Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added tutorial for newcomers contributing to libelektra using CLion #2687

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@e01306821
Copy link
Contributor

commented May 9, 2019

I've written a tutorial to help newcomers contributing to libelektra using CLion as proposed in #2623. Instead of solely focusing on development using CLion, I've described the whole process of adding new code to this project. If therefore anything important is missing especially regarding the development-procedure, just leave me a note.

Closes #2623

Basics

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins and doc/METADATA.md)

Review

@markus2330 please review my pull request
Tagging @kodebach too, if you have time to spare and want to, your input is also very welcome (of course this applies to everyone) =)

@sanssecours
Copy link
Member

left a comment

It looks like the spelling check fails on the build server. Please run

scripts/fix-spelling

and then commit the changes.

- doc<br/>
This folder contains mainly all documentation of the project, including
almost all pages of the [homepage](https://www.libelektra.org) of this
project. One important note is, that all markdown-pages can also be used

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 10, 2019

Member
Suggested change
project. One important note is, that all markdown-pages can also be used
project. One important note is, that all Markdown pages can also be used
the directory you have to work in. For further information on how to
develop your own plugin, please visit [this](/doc/tutorials/plugins.md)
tutorial. - tools<br/>
In this folder the source of all tools for interacting with Electra's

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 10, 2019

Member
Suggested change
In this folder the source of all tools for interacting with Electra's
In this folder the source of all tools for interacting with Elektra's
@kodebach
Copy link
Contributor

left a comment

Very nice guide, but there are somethings that could be improved. Especially the part about simply running as root is not a good recommendation for a tutorial.

Please also add information on how to enable clang-format support in newer versions of Clion.


## Setting up the project

It is important to start CLion as root-user, since some functionalities of

This comment has been minimized.

Copy link
@kodebach

kodebach May 12, 2019

Contributor

I actually wouldn't recommend that...

Instead I would recommend changing the CMake configurations such that you don't need root access.

To do that open File | Settings | Build, Execution, Deployment | CMake. There you should see all the different CMake configurations created (and automatically refreshed) by CLion. For most configurations, the CMake options field should contain (click the icon on the right to see one option per line) the following to remove need for root access:

-DKDB_DB_SYSTEM="~/.config/kdb/[xyz]/system"
-DKDB_DB_SPEC="~/.config/kdb/[xyz]/spec"
-DKDB_DB_USER=".config/kdb/[xyz]/user"
-DCMAKE_INSTALL_PREFIX=install
-DINSTALL_SYSTEM_FILES=OFF

Replace [xyz] by some unique name for the configuration e.g. dbg for debug. We use this, so that the different configurations don't clash with each other. The CMAKE_INSTALL_PREFIX changes the destination of make install to the install folder inside the CMake build directory. The advantage of using these lines, is that the resulting build of Elektra is completely isolated and shouldn't interfere with existing Elektra installations (even if make install is called).

Since this changes some of Elektra's behaviour (e.g. spec and system namespace are now also user specific), I don't recommend using these options for release builds. If you want to build Elektra, for use outside of development (of Elektra), I recommend just building via command line and sudo.

I would also recommend adding the options -DENABLE_DEBUG=ON and -DENABLE_LOGGER=ON for any debug builds, to enable some additional checks and logging capabilities. Additional, you may improve build speeds by setting the build options field to e.g. -j 8 (or more depending on your processor).

Users/developers can of course also add their own options.

Another note: While you may add as many different configurations as you want, Clion automatically maintains them all. This means everytime you change CMake files Clion will run cmake for all these configurations in parallel. Too many configurations and your system will grind to a halt for a few seconds everytime Clion decides to reload the CMake project.

Usually the folders you have to work in to add functionality or
documentation are as follows:

- doc<br/>

This comment has been minimized.

Copy link
@kodebach

kodebach May 12, 2019

Contributor

Don't use <br />, a newline should also do the job. To continue the enumeration after the line break, indent the text.

- doc
  This folder contains mainly all documentation of the project, including

This comment has been minimized.

Copy link
@markus2330

markus2330 May 13, 2019

Contributor

a newline should also do the job

Not every markdown renderer accepts the newline as new line, <br> should be fine here.

Almost all functionality-code resides here.
- bindings<br/>
Here is all the code of available [Bindings](/src/bindings/README.md)
of libelektra. - plugins<br/>

This comment has been minimized.

Copy link
@kodebach

kodebach May 12, 2019

Contributor

There are a few newlines missing here (before plugins, and tools)


The most thorough way to test your changes is to run all tests. Therefore
navigate to your run-configurations (Run -> Edit Configurations...) and look
for the entry "run*all". There, "run_all" should be selected as \_Executable*,

This comment has been minimized.

Copy link
@kodebach

kodebach May 12, 2019

Contributor
Suggested change
for the entry "run*all". There, "run_all" should be selected as \_Executable*,
for the entry "run_all". There, "run_all" should be selected as \_Executable*,

Not sure whether \_Executable* was intended either...

The most thorough way to test your changes is to run all tests. Therefore
navigate to your run-configurations (Run -> Edit Configurations...) and look
for the entry "run*all". There, "run_all" should be selected as \_Executable*,
"kdb" as _Target_. Now you can execute this run-configuration which will run

This comment has been minimized.

Copy link
@kodebach

kodebach May 12, 2019

Contributor

Actually for run_all you should probably select All targets (right at the top of the list) to ensure everything is built. Although I never tried running run_all this way. I simply use the integrated terminal and run make run_all in the cmake-build-xyz folder.

top left of the "Edit Configurations..."-window and name it. Here both
_Executable_ and _Target_ should have "kdb" selected. If you for example
want to test `kdb info dump`, write "info dump" next to _Program arguments_.
That's it, now you can just test this part of _kdb_.

This comment has been minimized.

Copy link
@kodebach

kodebach May 12, 2019

Contributor

You can also use a run configuration with executable and target set to any of the testmod_* or testkdb_* targets to run these tests.

Additionally for any of the tests that use Google Test, Clion's integrated Test runner may be used. The easiest way to create those run configurations is to just open the test source and click the icon next to the class.

For the testscr_* and testshell_* tests the best way of running is via the integrated terminal and ctest (after ensuring the correct parts of Elektra are built).

your changes. By default, you are in the master-branch of your repository.
First, you should never directly work on the master branch, since only
working code is expected to be there. This means, we now create a branch in
our repository where our code changes will be published into. To do this,

This comment has been minimized.

Copy link
@kodebach

kodebach May 12, 2019

Contributor

In a Clion tutorial you should really describe how to do this using the built in Git (and GitHub) support.

Also add:
Ensure that "Reformat code" and "Rearrange code" are disabled in the commit dialog. Otherwise Clions formatter might produce files that don't adhere to our formatting guidelines.
If you installed the pre-commit-check-formatting pre-commit-hook from the scripts directory. Ensure that "Run Git hooks" is enabled in the commit dialog.


## Creating a Pull Request

This step is most easily done using a browser. Open the web page of the

This comment has been minimized.

Copy link
@kodebach

kodebach May 12, 2019

Contributor

The built in Github support of Clion covers creating PRs, but I never tried it myself

This comment has been minimized.

Copy link
@markus2330

markus2330 May 13, 2019

Contributor

This can be added as extra information but it is good to also write that it is possible via web-interface.

- Especially for not that well-versed programmers don't forget that when
debugging e.g. `KeySet`, which can contain many `Key`-objects, you can only
see that there may be more than one key stored in this set by it's
_size_-attribute. Just the foremost `Key` will be visible directly using the

This comment has been minimized.

Copy link
@kodebach

kodebach May 12, 2019

Contributor
Suggested change
_size_-attribute. Just the foremost `Key` will be visible directly using the
_size_-attribute. Just the first `Key` will be visible directly using the

Also you can add a new watch with an expression like ks->array[9] (where ks is a KeySet and ks->size > 9) to monitor the element at index 9. You can also just use the evaluate expression or the GDB debug console to check the elements of ks->array.

Clion just doesn't now the size of the array, so it can't show you all elements without risking buffer overflows.

see that there may be more than one key stored in this set by it's
_size_-attribute. Just the foremost `Key` will be visible directly using the
debugger.
- Sometimes CLion does not rebuild changed modules accordingly by just running

This comment has been minimized.

Copy link
@kodebach

kodebach May 12, 2019

Contributor

Please add something like:

If you think, that the modules should have been rebuilt (e.g. if running testmod_abc doesn't rebuild abc), please report the issue under https://issues.libelektra.org, so we can fix the CMake dependencies.

@sanssecours
Copy link
Member

left a comment

Thank you for the pull request. I added some review suggestions.


- Git ([link](https://git-scm.com/download)) or similar software
- CLion ([link](https://www.jetbrains.com/clion/)) or similar software
- GitHub-account ([link](https://github.com/))

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 13, 2019

Member
Suggested change
- GitHub-account ([link](https://github.com/))
- [GitHub account](https://github.com)
## Prerequisites

- Git ([link](https://git-scm.com/download)) or similar software
- CLion ([link](https://www.jetbrains.com/clion/)) or similar software

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 13, 2019

Member
Suggested change
- CLion ([link](https://www.jetbrains.com/clion/)) or similar software
- [CLion](https://www.jetbrains.com/clion) or similar software

## Prerequisites

- Git ([link](https://git-scm.com/download)) or similar software

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 13, 2019

Member
Suggested change
- Git ([link](https://git-scm.com/download)) or similar software
- [Git](https://git-scm.com/download) or similar software
- CLion ([link](https://www.jetbrains.com/clion/)) or similar software
- GitHub-account ([link](https://github.com/))

## Forking the repository

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 13, 2019

Member

Please use title case for headings.

Suggested change
## Forking the repository
## Forking the Repository
- https://github.com/ElektraInitiative/libelektra

To be able to make pull requests, you need a copy of this repository inside
your GitHub-account. You can find a tutorial about how to do this

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 13, 2019

Member
Suggested change
your GitHub-account. You can find a tutorial about how to do this
your GitHub account. You can find a tutorial about how to do this
convention, such commits shouldn't be too large, otherwise it will become
difficult to revert some small changes if they are not working as intended.
To be able to commit code to your repository, you have to configure your
local git-installation to use your GitHub-credentials. You can find

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 13, 2019

Member
Suggested change
local git-installation to use your GitHub-credentials. You can find
local git-installation to use your GitHub credentials. You can find
information about how to do this
[here](https://help.github.com/en/articles/set-up-git).

After you've set up your git-configuration, you can continue with uploading

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 13, 2019

Member
Suggested change
After you've set up your git-configuration, you can continue with uploading
After you've set up your Git configuration, you can continue with uploading
[here](https://help.github.com/en/articles/set-up-git).

After you've set up your git-configuration, you can continue with uploading
your changes. By default, you are in the master-branch of your repository.

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 13, 2019

Member
Suggested change
your changes. By default, you are in the master-branch of your repository.
your changes. By default, you are in the master branch of your repository.
## Creating a Pull Request

This step is most easily done using a browser. Open the web page of the
git-repository of libelektra (https://github.com/ElektraInitiative/libelektra)

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 13, 2019

Member
Suggested change
git-repository of libelektra (https://github.com/ElektraInitiative/libelektra)
Git repository of libelektra (https://github.com/ElektraInitiative/libelektra)
Please read the template in this form and include the information stated there
if possible. Finally by clicking "Create pull request" you've successfully
created a pull request to merge your changes into the official repository! Now
maintainers of libelectra will review your code and, if everything is fine,

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 13, 2019

Member
Suggested change
maintainers of libelectra will review your code and, if everything is fine,
maintainers of libelektra will review your code and, if everything is fine,
@markus2330
Copy link
Contributor

left a comment

Thank you, this is a very much needed tutorial!

Usually the folders you have to work in to add functionality or
documentation are as follows:

- doc<br/>

This comment has been minimized.

Copy link
@markus2330

markus2330 May 13, 2019

Contributor

a newline should also do the job

Not every markdown renderer accepts the newline as new line, <br> should be fine here.

actually commit it to your online repository. Therefore type:

```sh
git commit -m "Added functionallity to kdb get. Closes #1234"

This comment has been minimized.

Copy link
@markus2330

markus2330 May 13, 2019

Contributor

Please use a less generic commit message as example. See .github/PULL_REQUEST_TEMPLATE.md:

first line should have module: short statement syntax

And please use a less generic statement as example.


## Creating a Pull Request

This step is most easily done using a browser. Open the web page of the

This comment has been minimized.

Copy link
@markus2330

markus2330 May 13, 2019

Contributor

This can be added as extra information but it is good to also write that it is possible via web-interface.

([shortcut](https://github.com/ElektraInitiative/libelektra/compare)), you
can now create a pull request referencing your forked repository and the
branch,the modified code resides in. Click on "compare across forks" so that
you can find and select your branch. Choose "<username>/libelectra" as the

This comment has been minimized.

Copy link
@markus2330

markus2330 May 13, 2019

Contributor
Suggested change
you can find and select your branch. Choose "<username>/libelectra" as the
you can find and select your branch. Choose "<username>/libelektra" as the
@e01306821

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Thank you so much for your detailed reviews! I hope I've addressed all of your requests.

@e01306821

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

How can I prevent the following change?
-for the entry "run_all". There, "run_all" should be selected as Executable,
+for the entry "runall". There, "run_all" should be selected as _Executable,

@kodebach

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

If you put run_all between backticks (`) to mark it as a code segment the text will be taken as is without markdown formatting.

@e01306821 e01306821 force-pushed the e01306821:tutorial branch from 20456e0 to 37a3f1f May 18, 2019

@e01306821 e01306821 force-pushed the e01306821:tutorial branch 2 times, most recently from b31872b to 4d7fb4a May 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.