libelektra icon indicating copy to clipboard operation
libelektra copied to clipboard

Basic implementation of "gopts" hook

Open atmaxinger opened this issue 3 years ago • 1 comments

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.md which 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 statement syntax)
  • [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
  • [ ] 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

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.

atmaxinger avatar Sep 18 '22 19:09 atmaxinger

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?

atmaxinger avatar Sep 22 '22 10:09 atmaxinger

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.

kodebach avatar Sep 22 '22 11:09 kodebach

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.

atmaxinger avatar Sep 22 '22 14:09 atmaxinger

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.

kodebach avatar Sep 22 '22 14:09 kodebach

@markus2330 can we merge this so I can continue with spec and the other hooks?

atmaxinger avatar Sep 24 '22 09:09 atmaxinger

@atmaxinger Thank you, great job! Is there already something to test for @Eiskasten?

markus2330 avatar Sep 24 '22 10:09 markus2330