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

KConfig Set #3277

Merged
merged 5 commits into from Dec 1, 2019
Merged

KConfig Set #3277

merged 5 commits into from Dec 1, 2019

Conversation

@darddan
Copy link
Contributor

darddan commented Nov 24, 2019

Basics

These points need to be fulfilled for every PR:

  • Short descriptions of your changes are in the release notes
    (added as entry in doc/news/_preparation_next_release.md which
    contains _(my name)_)
    Please always add something to the release notes.
  • Details of what you changed are in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, are in the commit messages.
  • The buildservers are happy.
  • The PR is rebased with current master.

Checklist

  • I added unit tests for my code
  • I fully described what my PR does in the documentation
    (not in the PR description)
  • I fixed all affected documentation
  • I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
  • I updated all meta data (e.g. README.md of plugins and METADATA.ini)
  • I mentioned every code not directly written by me in THIRD-PARTY-LICENSES

Review

@darddan

This comment has been minimized.

Copy link
Contributor Author

darddan commented Nov 24, 2019

For some reason I cannot add a Work in progress label.
Also I have a question. @FelixResch noticed that if you add a key into the GUI backend with brackets (example [key name]) it will be saved so into the file. Should I throw an error if I encounter such keys or should it be implemented in other ways.

@darddan darddan changed the title Implement first version of KConfig plugin set function KConfig Set Nov 24, 2019
@darddan darddan force-pushed the darddan:kconfig_set branch from 3bf58fb to 9891e6d Nov 24, 2019
@restyled-io restyled-io bot mentioned this pull request Nov 24, 2019
@darddan darddan force-pushed the darddan:kconfig_set branch from 9891e6d to 1ec5aa6 Nov 24, 2019
@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Nov 25, 2019

Also I have a question. @FelixResch noticed that if you add a key into the GUI backend with brackets (example [key name]) it will be saved so into the file. Should I throw an error if I encounter such keys or should it be implemented in other ways.

I think it is a rather low-priority issue. You can stay compatible to how it was before. If it is not too much work, proper escaping would be the best way. But then you need to somehow distinguish which [] are for locales (unescaped) and which `[]``are to be escaped.

@darddan darddan force-pushed the darddan:kconfig_set branch 3 times, most recently from 79c6649 to 88a70d8 Nov 25, 2019
@mpranj

This comment has been minimized.

Copy link
Member

mpranj commented Nov 26, 2019

Yep. In the jenkins blueocean UI click on the artifacts tab. The output is part of artifacts/debian-buster-full/DynamicAnalysis.xml.

If something fails only on the buildserver and you can't reproduce it locally you can also add more artifacts by editing /scripts/jenkins/Jenkinsfile to debug the situation. Basically you can add anything from the build directory. Do this only when needed as it can store tons of data on the buildserver.

@darddan

This comment has been minimized.

Copy link
Contributor Author

darddan commented Nov 26, 2019

Thanks I also noticed that the report was being shown in another CI that also failed

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Nov 27, 2019

jenkins build libelektra please

Copy link
Contributor

markus2330 left a comment

Looks good, I like it!

src/plugins/kconfig/README.md Outdated Show resolved Hide resolved
src/plugins/kconfig/README.md Show resolved Hide resolved
src/plugins/kconfig/testmod_kconfig.cpp Outdated Show resolved Hide resolved
@restyled-io restyled-io bot mentioned this pull request Nov 29, 2019
@darddan darddan force-pushed the darddan:kconfig_set branch 6 times, most recently from 1a52eb6 to bef6194 Nov 29, 2019
@mpranj

This comment has been minimized.

Copy link
Member

mpranj commented Nov 30, 2019

@darddan you need to fix the formatting, otherwise the jenkins tests will abort early. This is not a temporary jenkins failure.

If you have the all the formatting tools in the correct versions installed locally, you can reformat using ctest -VV -R testscr_check_formatting. If not, the buildserver also gives a patch including instructions on how to apply the patch.

@darddan darddan force-pushed the darddan:kconfig_set branch 5 times, most recently from 9afd54b to 7f9eac9 Nov 30, 2019
@darddan darddan force-pushed the darddan:kconfig_set branch 2 times, most recently from 6c26f7c to d7c5805 Dec 1, 2019
Static cast int to char in kconfig and fix failing tests
Merge master into this branch
@darddan

This comment has been minimized.

Copy link
Contributor Author

darddan commented Dec 1, 2019

@markus2330 @mpranj I reran the FULL job, it builds correctly, but the unsuccessful one still shows up here (as duplicate). could you tell me what to do next, is there something I'm overlooking?

@darddan

This comment has been minimized.

Copy link
Contributor Author

darddan commented Dec 1, 2019

The last one timed out. But looking at the report here, this passed 6 times already.

@mpranj
mpranj approved these changes Dec 1, 2019
Copy link
Member

mpranj left a comment

I like the clear and readable style of the code very much. There's not much to say about that so here are some other hints:

  1. When you mark a PR ready to merge, don't push new changes (someone could already be reviewing it).
  2. The code coverage could maybe be improved, but it's not critical in this phase. You can check it locally but the buildserver already does it for you here. You will find the listing of all PRs and the master branch here.
@mpranj mpranj merged commit 3c53b19 into ElektraInitiative:master Dec 1, 2019
17 checks passed
17 checks passed
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 62.078%
Details
restyled No differences
Details
🍎 Clang Task Summary
Details
🍎 Clang ASAN Task Summary
Details
🍎 FULL Task Summary
Details
🍎 MMap Task Summary
Details
🐧 Fedora Task Summary
Details
📚 Check Task Summary
Details
🔗 Check Task Summary
Details
😈 ASAN Task Summary
Details
😈 FreeBSD 11 Task Summary
Details
😈 FreeBSD 12 Task Summary
Details
@mpranj

This comment has been minimized.

Copy link
Member

mpranj commented Dec 1, 2019

Thank you very much for implementing the write functionality!

Looks like the FULL job was stuck (maybe a cirrus-ci bug) and you resolved it by pushing another commit.

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Dec 2, 2019

I added the kconfig plugin in the Debian package 0c83880

@mpranj did you test the plugin with your (old) KDE config? I plan to do it once the plugin is in the Debian package.

@mpranj

This comment has been minimized.

Copy link
Member

mpranj commented Dec 2, 2019

I did not have time to do any manual tests. I don't think I have any interesting KDE config, I basically only use KDevelop in gnome.

I'll have to test it with a fresh KDE installation.

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