libelektra
libelektra copied to clipboard
Basic implementation of "gopts" hook
This PR aims to provide a basic implementation for the "gopts" hook. In theory it works, but there is still one assert in the test cases that does not, as mentioned in #4412.
This PR should also provide an opportunity to discuss the architecture of hooks in general.
Basics
- [x] Short descriptions of your changes are in the release notes
(added as entry in
doc/news/_preparation_next_release.mdwhich contains_(my name)_) Please always add something to the release notes. - [x] Details of what you changed are in commit messages
(first line should have
module: short statementsyntax) - [x] 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
- [ ] add a line in
- [ ] The PR is rebased with current master.
Checklist
- [x] 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
- [x] 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.
testkdb_error
testkdb_nested
testkdb_simple
Those seem to expect 0 as return from kdbGet. The only way this could happen is if there is no backend left. I don't think that has anything to do with my changes.
testshell_markdown_blacklist
testshell_markdown_conditionals
testshell_markdown_email
testshell_markdown_ipaddr
testshell_markdown_length
testshell_markdown_tutorial_validation
I can't run these for whatever reason (probably missing dependency), but if I interpret it correctly, these tests just execute the shell commands included in the README of those plugins?
I can't run these for whatever reason (probably missing dependency),
I think some of these plugins don't have dependencies. You probably haven't enabled them in CMake. When you run cmake, set -DPLUGINS=ALL that enables all plugins for which you have the dependencies.
these tests just execute the shell commands included in the README of those plugins?
Yes, all the testshell_markdown_* tests run shell scripts from some markdown file. If the suffix is a plugin name, then the script is the plugin's README. testshell_markdown_tutorial_validation is from another file. Of the top of my head I'm not sure which one...
I don't think that has anything to do with my changes.
In some way it must be your changes, since the test worked in the latest run on new-backend. But like I said, if you don't find the problem, just leave it.
In some way it must be your changes, since the test worked in the latest run on new-backend. But like I said, if you don't find the problem, just leave it.
Yes, the problem lies in goptsActive. Previously, it was checked like this:
bool goptsActive = handle->globalPlugins[PROCGETSTORAGE][MAXONCE] != NULL;
Which doesn't really tell you if gopts is really activated, just if there is some plugin loaded there. Spoiler: the list plugin is. This with the fact that both backends are in the proc:/ namespaces led to the following block executing:
https://github.com/ElektraInitiative/libelektra/blob/3e9f5ed10725627405fb36c7a1ea33fa7eb67a1e/src/libs/elektra/kdb.c#L1780-L1794
So this is more or less a bug/works-by-accident behaviour of the new-backend branch IMHO.
Hm yeah you're right. Let's leave it the way it is. We'll fix it in another PR at some point.
Although something needs to change, because the failing tests haven't changed for a long time. For example this one
https://github.com/ElektraInitiative/libelektra/blame/5ebc9a4ba5215950f990f7dca948ce310f764079/tests/kdb/testkdb_simple.cpp#L482
hasn't changed for at least 7 years. That's a lot older than gopts itself.
@markus2330 can we merge this so I can continue with spec and the other hooks?
@atmaxinger Thank you, great job! Is there already something to test for @Eiskasten?