Basic implementation of "gopts" hook

[atmaxinger]

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

• 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.

[kodebach]

The general idea looks great (it's pretty much what I had in mind when I started the new-backend branch). However, the way you changed struct _KDB requires quite a lot of allocations. I'm pretty sure we can avoid those. There are also some other minor issues.

[markus2330]

Beautiful coding style!

[markus2330]

Link to the decision.

[atmaxinger]

Is there a decision file specifically for the hardcoding part?

[markus2330]

No, we only have doc/decisions/global_plugins.md

And you need to adapt the decision with what we are actually doing now, e.g. calling it hooks. So the file also needs to be renamed.

[atmaxinger]

I have modified and renamed it now.

[markus2330]

Use new API, keyMeta and meta:/ (keyGetMeta will probably stay as alias but will not accept keys without meta:/ prefix)

[kodebach]

IMO that's more of a nice-to-know. It's good if @atmaxinger knows that in future the recommended way to access meta keys is ksLookupByName (keyMeta (errorKey), "meta:/error", 0) (or possibly keyGetMeta (errorKey, "meta:/error")), but in the end it really doesn't matter if my tree-surgeon scripts have to replace one more call.

So maybe always use meta:/ in future, but IMO there is really no need to change this now.

[markus2330]

@kodebach if your tool can do these changes automatically it is of course also fine for now. But at some point we need to learn writing correct code (not relying on legacy code being in place), otherwise we will introduce wrong code again...

[atmaxinger]

So we keep it for now and let the tool of @kodebach handle it.

[kodebach]

Yes, you can keep this. But for future PRs it's probably better to always use meta:/ for meta keys. Just to get into the habbit and because there are already cases, where you need to meta:/.

[atmaxinger]

Do we have documentation for keyMeta? Because the API Doc still tells me that I should use keySetMeta and keyGetMeta. Or is it just that I have to use the meta:/ prefix for all keys I use with those functions?

[kodebach]

Beyond the API docs, I don't think keyMeta is documented yet. The docs are definitely outdated. keyGetMeta/keySetMeta might be removed (uncertain), but keyMeta definitely will be the main API for metadata in future, since you can already do everything necessary with it.

Or is it just that I have to use the meta:/ prefix for all keys I use with those functions?

When you use keyMeta then you definitely have to provide the meta:/ namespace yourself. The fact that keyGetMeta and keySetMeta work without the meta:/ prefix is a backwards compatibility thing. Nobody has yet wanted to update all uses, and my semi-automatic tool isn't ready yet.

The actual meta keys always use the meta:/ prefix, because otherwise it wouldn't be a valid keyname. For example:

printf ("%s\n", keyName (keyGetMeta (keyNew ("/foo", KEY_META, "type", "string", KEY_END), "type")));
// prints meta:/type

What you should do to get in the habit of things is write code with meta:/ instead of without:

Key * k = keyNew ("/foo", KEY_META, "meta:/type", "string", KEY_END);
keyGetMeta (k, "meta:/type");
keySetMeta (k, "meta:/type", "long");

This also makes it far less likely that you run into problems with one of the cases, where the meta:/ is actually needed, e.g. things like:

Key * k = keyNew ("/foo", KEY_META, "type", "string", KEY_END);

if (strcmp ("type", keyName (keyGetMeta (k, "type"))) == 0) {
   // doesn't work
}

if (strcmp ("meta:/type", keyName (keyGetMeta (k, "type"))) == 0) {
   // works
}

[kodebach]

Great work, LGTM. I really like the way you wrote the tests. Very clean. I have a few other comments, but IMO the PR could be merged as-is.

I also noticed that there are 10 new tests that fail compared to new-backend (see also):

testshell_markdown_blacklist
testshell_markdown_conditionals
testshell_markdown_email
testshell_markdown_ipaddr
testshell_markdown_length
testshell_markdown_tutorial_validation
testkdb_error
testkdb_nested
testkdb_simple

Please have a quick look, if you find any obvious problems that are easy to fix. If you don't find anything or it's too much work, just leave it for another PR. I think the issue might be that you disable all "global plugins" including spec.

[kodebach]

Actually, you can probably just return -1, if there is an existing error and avoid the if (!existingError) checks. If there was already an error, kdbOpen should never have called this function.

[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?

[kodebach]

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.

[atmaxinger]

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:

libelektra/src/libs/elektra/kdb.c

Lines 1780 to 1794 in 3e9f5ed

	if (ksGetSize (backends) == 0 || (goptsActive && keyGetNamespace (ksAtCursor (backends, 0)) == KEY_NS_PROC &&
	keyGetNamespace (ksAtCursor (backends, ksGetSize (backends) - 1)) == KEY_NS_PROC))
	{
	// TODO (kodebach): name not needed, once lock is in place
	keyCopy (parentKey, initialParent, KEY_CP_NAME | KEY_CP_VALUE);
	keyDel (initialParent);
	keyCopy (parentKey, keyGetMeta (backendsFindParent (allBackends, parentKey), "meta:/internal/kdbmountpoint"),
	KEY_CP_STRING);
	ksDel (backends);
	ksDel (allBackends);
	errno = errnosave;
	return 0;
	}

So this is more or less a bug/works-by-accident behaviour of the new-backend branch IMHO.

[kodebach]

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.

[atmaxinger]

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

[markus2330]

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