Binding/xfconf

[eiskasten]

trafficstars

Basics

• [ ] 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. If not, fix in this order:
  • [ ] add a line in doc/news/_preparation_next_release.md
  • [ ] reformat the code with scripts/dev/reformat-all
  • [ ] make all unit tests pass
  • [ ] fix all memleaks
• [ ] 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 reuse syntax

Review

• [ ] Documentation is introductory, concise, good to read and describes everything what the PR does
• [ ] Examples are well chosen and understandable
• [ ] Code is conforming to our Coding Guidelines
• [ ] APIs are conforming to our Design Guidelines
• [ ] Code is consistent to our Design Decisions

Labels

• [ ] Add the "work in progress" label if you do not want the PR to be reviewed yet.
• [ ] Add the "ready to merge" label if the basics are fulfilled and no further pushes are planned by you.

[markus2330]

Great to see this!

Please do not forget issues/PRs concerning new-backend :rocket:

[eiskasten]

@kodebach Do you know why the jenkins CI fails in this case? What does the jenkins state "unstable" mean? I cannot find any failing jobs or tests.

[kodebach]

No idea, @lukashartl or @markus2330 might know, but I'd just rerun the pipeline and see if the problem goes away.

[eiskasten]

Ok, I will try that

[eiskasten]

jenkins build libelektra please

[eiskasten]

jenkins build libelektra please

[eiskasten]

jenkins build libelektra please

[eiskasten]

jenkins build libelektra please

[eiskasten]

jenkins build libelektra please

[eiskasten]

@lukashartl currently the jenkins CI fails for this PR. Interestingly, when I trigger a rebuild for the same commit, the failure was triggered by a different job. You can see that by clicking the arrows on top left and compare the commit checksums. Do you have an idea what can cause this? Are other PRs affected as well?

[mpranj]

@Eiskasten no idea what causes this. A silly way that could trigger a proper rebuild: rebase from master and force push.

We once had a very rare circumstance where we had to close the PR and reopen a new one, but that's a last resort.

[eiskasten]

jenkins build libelektra please

[eiskasten]

@mpranj Thank you for your advise, it works now. However, it still fails randomly sometimes.

[eiskasten]

@markus2330 @tucek I know the review week did not start yet, but testing the Xfce session might not be straight forward. Please just follow the Using the binding as a replacement in Xfce section in the src/bindings/xfconf/README.md and let me know if you have problems.

[tucek]

@Eiskasten the build does not finish, please address these issues. i willl have a look at the changes in the meanwhile

[tucek]

jenkins build libelektra please

[tucek]

@mpranj why does this link check fail?

if test -s broken_links.txt; then
  printf >&2 'Broken Links:\n'
  printf >&2 '—————————————\n'
  cat >&2 broken_links.txt
  printf >&2 '—————————————\n'
  false
fi
Broken Links:
—————————————
src/plugins/crypto/README.md:64:0 https://gpgtools.org

[tucek]

@Eiskasten please rebase branch to current https://github.com/ElektraInitiative/libelektra master

[kodebach]

why does this link check fail?

If a link works locally for you, always attempt re-running the CI job (with the button on GitHub). Link checker issues are often temporary.

[eiskasten]

Done, the failing macOS builds should not affect anything. Building these bindings is disabled on macOS.

[tucek]

LGTM, after some trougbles i could build and install it, also read the documentation and found it useful, but suggested some improvements. Since this is a C implementation, i would like someone else to have a second look. @kodebach thx

[eiskasten]

I didn't try to run it and only had a quick look a the code, but based on that the PR looks mostly fine to me.

The only issue I have is that you seem to do a lot of key name manipulation with the escaped names. Since you don't handle the escapes properly that could go wrong. However, maybe because of the interaction with xfconf the escape stuff is irrelevant anyway. For example, if xfconf only allows something like [a-zA-Z0-9_-] your code should be fine, but I don't know if that's actually the case.

This is according to the official docs: https://docs.xfce.org/xfce/xfconf/start#introduction indeed the case

[eiskasten]

Besides the tutorial I think this could be merged. @markus2330 as we discussed in the meeting I would open a separate issue regarding the tutorial.

[eiskasten]

Looking forward to see this merged! Please ensure proper packaging (so that I can easily test it once it gets merged) and update documentation a bit.

Thank you for the review. The most points should be resolved. However, a few of them are still open. Hopefully I am able to fix them soon.

[eiskasten]

Everything should be corrected as you requested now.