Integrate kdbEnsure into kdbOpen #3651

kodebach · 2021-02-11T15:45:32Z

Implements #2764

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

If you have any troubles fulfilling these criteria, please write
about the trouble as comment in the PR. We will help you.
But we cannot accept PRs that do not fulfill the basics.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

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

Reviewers will usually check the following:

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

If you are already Elektra developer:

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 you also
say that everything is ready to be merged.

kodebach · 2021-02-11T15:49:58Z

I think the new implementation will work something like this:

Contract keys are below system:/elektra/contract
Each part below system:/elektra/contract/<contract-type> is a separate contract
<contract-type> can be anything but, just a few pre-defined values will have an effect
The contract-type globalkeyset copies data into the global keyset
All keys system:/elektra/contract/globalkeyset/... will renamed to system:/elektra/... and copied into the global keyset.
This will always be the first contract that is handled, since it may contain configuration for later ones.
The contract-type mountglobal mounts global plugins
system:/elektra/contract/mountglobal/<plugin>/ mounts the plugin <plugin> globally with the config below
Note: the first implementation will delegate to the list plugin here, once global plugins are reworked and list is removed, this will happen directly in kdbOpen
Everything else below system:/elektra/contract/ will be ignored for now. Detailed documentation will be in doc/dev/kdb-contracts.md

This is should be enough for both gopts and notifications. The new functions elektraGOptsContract() and elektraNotificationContract() will add contracts for the relevant data (e.g. argv and the ioBinding) below system:/elektra/contract/globalkeyset/ and basic contracts for mounting the plugins below system:/elektra/contract/mountglobal/gopts/ and system:/elektra/contract/mountglobal/internalnotification/ respectively.

kodebach · 2021-02-11T20:35:05Z

There is now a rough sketch of the implementation, to implementing the comment above.

lgtm-com · 2021-02-11T21:07:47Z

This pull request introduces 3 alerts when merging f2a2b1e into 77aec77 - view on LGTM.com

new alerts:

3 for FIXME comment

src/plugins/cache/cache.c

kodebach · 2021-02-13T19:01:38Z

src/libs/elektra/keyname.c

+ *            i.e. there is no prefix to replace
+ * @retval  1 if the prefix was sucessfully replaced
+ */
+int keyReplacePrefix (Key * key, const Key * oldPrefix, const Key * newPrefix)


I added this function to kdb.h since it is

quite useful to some plugins

much easier to use than manually calling keyName, keySetName, keyNameSize etc.

heavily optimized compared to the alternative

uses many internals of struct _Key that really shouldn't be modified outside the core

kodebach · 2021-02-13T19:09:27Z

src/libs/elektra/keyset.c

+ * @retval  0 if @p ks contains no keys below @p root (and also not @p root itself)
+ * @returns   otherwise, the number of keys that have been renamed
+ */
+ssize_t ksRename (KeySet * ks, const Key * root, const Key * newRoot)


We could add this function to kdb.h as well, but I put in in kdbprivate.h for now.

Mainly, because of the fact that it modifies keys with key->ksReference == 1 in-place. This is nice, because you can avoid allocating new keys and sometimes even completely avoids re-sorting the KeySet, but it can easily lead to memory bugs. For example in a section like this:

libelektra/src/libs/elektra/kdb.c

Lines 316 to 322 in 1e7ba55

const char * pluginName = keyBaseName (cur);

KeySet * pluginConfig = ksCut (contract, cur);

// increment ref count, because cur is part of pluginConfig and

// we hold a reference to cur that is still needed (via pluginName)

keyIncRef (cur);

ksRename (pluginConfig, cur, pluginConfigRoot);

However, apart from this the function is completely safe to use and could be useful in plugins that move keys around. So maybe putting it into kdb.h could make sense.

kodebach · 2021-02-13T19:13:45Z

src/libs/elektra/keyset.c

+ *         in @p ks, the size of @p ks is returned. The snippet above
+ *         shows why this is useful.
+ */
+elektraCursor ksFindHierarchy (const KeySet * ks, const Key * root, elektraCursor * end)


I found this neat alternative to the ksExtract function we discussed in #2992. I put it into kdbprivate.h for now, but I really think it should be in kdb.h since it will be incredibly useful.

I'll also rewrite elektraKsFindCutpoint (used by ksCut) to use this, instead of just looking for the root and then looping over the following keys.

src/bindings/swig/python/tests/test_gopts.py

kodebach · 2021-02-13T19:38:26Z

@markus2330 The actual integration of kdbEnsure into kdbOpen, i.e. the contracts part, should now be done. I just need to rewrite the notifications stuff and then replace the kdbOpenOld calls (and fix bugs).

kodebach · 2021-02-14T19:33:15Z

src/bindings/rust/elektra/src/kdb.rs

@@ -26,8 +26,10 @@ impl Drop for KDB {
 impl KDB {
    /// Opens the session with the Key database.
    pub fn open<'a>() -> Result<Self, KDBError<'a>> {
+        // FIXME: binding


@PhilippGackstatter Should I keep anything in mind here, you can I just add the new KeySet parameter? Also would using std::option be a good idea, since the KeySet can be NULL in C?

I think we can just add it. We could do the signature like this:

pub fn open<'a, C>(contract: C) -> Result<Self, KDBError<'a>> where C: Into<Option<Contract>>;

This way, we can call KDB::open(None) and KDB::open(Contract { ... }) respectively, without having to specify Some(Contract {}) in the second case, so it's a bit nicer than using Option directly.

I tried it here on the Rust playground.

kodebach · 2021-02-14T19:38:36Z

src/bindings/swig/lua/tests/test_gopts_kdb.lua

@@ -0,0 +1,87 @@
+--[[
+-- TODO: cannot call std::vector<std::string> function from lua


The std::vector<std::string> solution I used for Ruby and Python doesn't work for Lua (it seems Lua Tables can't easily be converted into std::vector). Do we have someone who knows more about Lua and SWIG, or should we just leave elektraGOptsContract unsupported in Lua?

@manuelm as you are our best Lua hacker, do you have some idea how this can be done?

This should work out of the box:

local custom_argv = kdb.StringVector() custom_argv:push_back("test") custom_argv:push_back("get") custom_argv:push_back("-v") custom_argv:push_back("user:/")

I already found a different solution, but thanks for the help

kodebach · 2021-02-14T19:38:41Z

@markus2330 Apart from the updates to the Java and the Rust binding (and the public API questions) this PR should be done, please Review (or delegate to the appropriate person)

markus2330

Amazing work, I really like your attention to many details and the many other small things that you fixed.

The main issues are (maybe not so obvious from the individual comments):

there is a bit chaos about the contract functions: in kdb.h they are called elektra.* but in other places it is in KDB namespace/class.
modularization not ideal: *Contract functions should be (from modularity point of view) ideally be part of the module, not of the core. Ideally applications should even link against libelektra-notification (in this case they actually already do) or libelektra-opts, then the contract could also be part of that lib/plugin.
global keyset is not documented but with this PR many semantics of the global keyset were actually assumed/fixed.

Btw. are opts/gotps really the ideal name? Why do we have a src/libs/opts/opts.c library if it is not user-facing? Is this something that actually should be static? (see also point 2. about modularization)

doc/dev/kdb-contracts.md

doc/news/_preparation_next_release.md

src/bindings/jna/libelektra5j/src/main/java/org/libelektra/KDB.java

src/include/kdb.h.in

src/libs/elektra/kdb.c

src/libs/highlevel/elektra.c

doc/news/_preparation_next_release.md

kodebach · 2021-02-17T01:03:53Z

there is a bit chaos about the contract functions: in kdb.h they are called elektra.* but in other places it is in KDB namespace/class.

C++ uses the kdb namespace for everything (including KeySet and Key). SWIG uses a global kdb object to wrap this namespace. I'm not sure where you'd want to put the function? A new elektra namespace (AFAIK we already use that for the highlevel API)?

In Java everything has to be in a class. We could create a new KDBContracts class, but why not just leave it in KDB? After all you will use these functions together with kdbOpen.

I didn't call it e.g. kdbGOptsContract or kdbContractGOpts in C, because it's not part of the core KDB API and our helper functions normally use the prefix elektra.

modularization not ideal: *Contract functions should be (from modularity point of view) ideally be part of the module, not of the core. Ideally applications should even link against libelektra-notification (in this case they actually already do) or libelektra-opts, then the contract could also be part of that lib/plugin.

Unless I made a mistake, the elektraIoContract and elektraNotificationsContract functions should be part of libelektra-io and libelektra-notification respectively. However, elektraGOptsContract is part of the core, because it actually works without explicitly linking against libelektra-opts (since the gopts plugin does the linking). Another major reason why I put it into the core, is that it makes the SWIG bindings much easier. AFAIK we would either always have to link against libelektra-opts in SWIG or create a separate binding just for elektraGOptsContract. The C++ binding with it's header only setup could also be problematic.

global keyset is not documented but with this PR many semantics of the global keyset were actually assumed/fixed.

Yes, we should actually fix that. I didn't change any of the global keyset behaviour AFAIK (apart from not caching everything). Basically, everything this PR depends on related to the global keyset, is stuff the cache and mmapstorage plugin also depend on.

Btw. are opts/gotps really the ideal name? Why do we have a src/libs/opts/opts.c library if it is not user-facing? Is this something that actually should be static? (see also point 2. about modularization)

The name is just a name. If you have a better idea, feel free to change it.

src/libs/opts/opts.c is part of libelektra-opts which can be used without the gopts plugin as well, by calling elektraGetOpts directly. It doesn't work very well with specifications (other than the opts stuff), since you can't call it before validation plugins are executed, but for simple applications that just want to use the command line parser, it is an option.

kodebach · 2021-02-17T01:35:04Z

BTW I have no idea what is going on with the failing macOS build jobs. I assume it's the same problem as with the failing Jenkins build, but somehow it doesn't segfault on macOS. However, I have absolutely no clue how to reproduce this. The only thing I'm pretty sure about is that only the Java test has a problem and the other failed tests are because the Java test aborted and didn't clean up properly.

markus2330 · 2021-02-18T16:38:13Z

Yes, we should actually fix that. I didn't change any of the global keyset behaviour AFAIK (apart from not caching everything).

And this is a huge change and must be documented. And afaik this is the only semantic the content of the global keyset has so far, so the docu does not need to include much else. Nobody can read your mind, so please write down how you were thinking how it should be used. (I could only copy texts from your discussion but this leads to very poor docu.)

The name is just a name. If you have a better idea, feel free to change it.

You are obviously very efficient in renaming, most other people, including me, are not. And once users start using it at wider scale, we can forget all about renaming. I already stated that ideally there would be only one interface, which could be called "proc" (like the namespace), so there would be no need for "opts" or "gopts".

Btw. it is not like that I cannot live with the current situation, I understand you if you say my suggestions are too much effort to implement. I just gave the feedback the the API situation of opts/gopts looks to me hackish and historically grown.

kodebach · 2021-02-18T17:56:11Z

Nobody can read your mind, so please write down how you were thinking how it should be used.

I can't read @mpranj's mind either. So I don't actually know what the original intention behind the global keyset was. I'll write down what I know, but I can't guarantee it will be 100% correct.

I already stated that ideally there would be only one interface,

Yes, ideally both the plugin and the library would use the same name. But that's not possible right now. There is definitely a name collision somewhere in CMake, but there might be other things that prevent this.

The actual interface is pretty much the same already. The command-line argument and environment variable parsing is configured via the metadata in the spec:/ namespace, no matter whether you use elektraGetOpts directly or the gopts plugin.

which could be called "proc" (like the namespace), so there would be no need for "opts" or "gopts".

I think proc is too general, maybe args but that's basically the same as opts.

markus2330 · 2021-02-18T18:03:11Z

I can't read @mpranj's mind either. So I don't actually know what the original intention behind the global keyset was. I'll write down what I know, but I can't guarantee it will be 100% correct.

Thank you very much 💖

I think proc is too general, maybe args but that's basically the same as opts.

If the name proc is too general we also have a problem with the namespace proc. I do not see how args/arguments/variables/options are less general, though.

kodebach · 2021-02-18T18:13:42Z

If the name proc is too general we also have a problem with the namespace proc.

The namespace proc is the reason why "proc" is too general as a new name for the opts library or the gopts plugin. The proc namespace encompasses much more than just command-line arguments and environment variables. Sure right now that's the only thing we use it for, but the proc namespace is intended for process internal data. That could be a lot more.

I do not see how args/arguments/variables/options are less general, though.

args and opts are still very general, but at least the are limited to what the library/plugin actually does.

mpranj · 2021-02-18T18:15:24Z

I can't read @mpranj's mind either. So I don't actually know what the original intention behind the global keyset was.

Unfortunately I forgot to document that the global keyset is cached. Other than that the intention and usage is documented (in multiple places).

I did not think anybody would want to store pointers in the global keyset. Actually I think the way we store pointers in KeySets (at several places) is abuse of the API and not recommended for any external user/developer.

On the other hand it made sense to cache the global keyset. With the cache we try to store the complete/relevant state of the KDB. The Split for example is also restored from the cache.

kodebach · 2021-03-03T11:52:23Z

The Jenkins build should™ now work. The failing job didn't build the gopts plugin and the SWIG and JNA binding tests didn't check for that. (Note: the solution for JNA is a bit more janky, because our integration between CMake and Maven is not great)

I still don't know why the 🍎 MMap job failed, but maybe it now works too. But looking at the Cirrus config, it should build gopts and therefore probably has a different problem.

PS. No, I don't know why the tests segfaulted because of a missing plugin instead of returning the proper error.

kodebach · 2021-03-03T16:00:35Z

@mpranj I found out why the "🍎 MMap" job doesn't work. It actually has nothing to do with macOS and happens on Linux as well (with the CMake config from the Cirrus job). The immediate cause is that this line

libelektra/src/plugins/mmapstorage/mmapstorage.c

Line 1136 in ae419e6

ksClose (global); // TODO: if we have a global keyset, maybe use ksAppend?

clears the KeySet and removes the data that gopts expects. There is already a TODO here that seems to anticipate the problem, so how should I solve this?

EDIT: I tried a simple solution with ksNew and ksAppend, but immediately got some memory errors. I'll wait for suggestions before investigating further.

mpranj · 2021-03-03T18:52:28Z

There is already a TODO here that seems to anticipate the problem, so how should I solve this?

I can make a PR tomorrow at latest. If it does not break anything else it should be quick.

kodebach · 2021-03-03T18:55:29Z

You should be able to push directly to this branch. Then we'd know right away that it fixes the issue as well as not breaking anything else.

markus2330 · 2021-03-04T06:16:34Z

@mpranj thank you very much for helping out here!

…re lost

mpranj · 2021-03-04T14:54:06Z

@kodebach sorry you hit this bug/nasty leftover TODO, but thank you very much for reporting and pinning down the exact problem.

I've pushed a fix, let's hope the build servers are happy. Works on my machine(tm).

kodebach · 2021-03-04T16:59:33Z

From my side this PR is done and the build servers are happy as well. @markus2330 Do still have any input or can this be merged?

markus2330 · 2021-03-05T08:07:48Z

Thank you so much for this amazing work! So let us test this and release it asap.

@tucek please make sure to rebase to master so that your work is based on top of this (it contains changes to the Java binding).

tucek · 2021-03-05T14:19:36Z

After rebase (and rebuilding elektra) i get the following test error:
jna_test_failure.txt

Does anyone recognize the Problem?

kodebach · 2021-03-05T14:52:57Z

Have you tried doing a clean build? i.e. deleting the whole build directory and calling cmake again (just deleting the src/bindings part of the build directory might work too).

The JNA setup is a bit funky and doesn't really detect, if the C API changed and it should rebuild the JNA bindings. It then tries to call functions that no longer exist, when you run the tests.

tucek · 2021-03-05T18:26:55Z

i had some so mismatch... i had to first uninstall and then reinstall elektra... thx

kodebach commented Feb 13, 2021

View reviewed changes

src/plugins/cache/cache.c Outdated Show resolved Hide resolved

kodebach commented Feb 13, 2021

View reviewed changes

src/plugins/cache/cache.c Outdated Show resolved Hide resolved

kodebach commented Feb 13, 2021

View reviewed changes

src/bindings/swig/python/tests/test_gopts.py Outdated Show resolved Hide resolved

kodebach requested a review from markus2330 February 13, 2021 19:22

kodebach force-pushed the kodebach/issue2764 branch from 50af897 to e95fd91 Compare February 13, 2021 19:38

kodebach commented Feb 14, 2021

View reviewed changes

kodebach added the work in progress label Feb 14, 2021

kodebach marked this pull request as ready for review February 14, 2021 19:39

kodebach mentioned this pull request Feb 15, 2021

gopts: directly override config #3608

Closed

20 tasks

markus2330 linked an issue Feb 16, 2021 that may be closed by this pull request

inject argc, argv, environ #3607

Closed

This was referenced Feb 16, 2021

Implement fuse-prototype including context-mocking and README.md #3657

Merged

Scope of implementation work done within FUSE Thesis #3599

Closed

kodebach mentioned this pull request Feb 16, 2021

Update kdbOpen ElektraInitiative/go-elektra-archive#11

Closed

markus2330 requested changes Feb 16, 2021

View reviewed changes

kodebach added 9 commits March 3, 2021 11:13

fix external codegen examples

3ad0102

improve kdb open error messages

dc56296

another attempt to fix JNA

69bfa0a

more detailed error messages in JNA

72bd95e

fix jna

0028e15

mark GOptsTest.java as needing KDB

04caf7e

Fix keyCopy/keyDup after rebase

c406517

Update release notes

3aa62c9

exclude the gopts tests if gopts plugin is not included

1c22489

kodebach force-pushed the kodebach/issue2764 branch from a7f2695 to 1c22489 Compare March 3, 2021 11:45

This was referenced Mar 3, 2021

remove cursor API #3189

Closed

CLI binding for option parsing #2492

Closed

mpranj added 2 commits March 4, 2021 15:38

mmapstorage: do not replace non-empty global keyset, otherwise keys a…

189c051

…re lost

mmapstorage: refactor keyset replacement

4cb8b36

kodebach removed the work in progress label Mar 4, 2021

kodebach mentioned this pull request Mar 4, 2021

backend plugin #2969

Closed

14 tasks

markus2330 mentioned this pull request Mar 5, 2021

document global keyset #3687

Closed

markus2330 merged commit a80240f into ElektraInitiative:master Mar 5, 2021

kodebach mentioned this pull request Mar 20, 2021

Elektra Core-API Reviews #3718

Merged

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate kdbEnsure into kdbOpen #3651

Integrate kdbEnsure into kdbOpen #3651

kodebach commented Feb 11, 2021

kodebach commented Feb 11, 2021

kodebach commented Feb 11, 2021

lgtm-com bot commented Feb 11, 2021

kodebach Feb 13, 2021

kodebach Feb 13, 2021

kodebach Feb 13, 2021

kodebach commented Feb 13, 2021

kodebach Feb 14, 2021

PhilippGackstatter Feb 15, 2021

kodebach Feb 14, 2021

markus2330 Feb 16, 2021

manuelm Feb 16, 2021

kodebach Feb 16, 2021

kodebach commented Feb 14, 2021

markus2330 left a comment

kodebach commented Feb 17, 2021

kodebach commented Feb 17, 2021

markus2330 commented Feb 18, 2021

kodebach commented Feb 18, 2021

markus2330 commented Feb 18, 2021

kodebach commented Feb 18, 2021

mpranj commented Feb 18, 2021

kodebach commented Mar 3, 2021

kodebach commented Mar 3, 2021 •

edited

mpranj commented Mar 3, 2021

kodebach commented Mar 3, 2021

markus2330 commented Mar 4, 2021

mpranj commented Mar 4, 2021

kodebach commented Mar 4, 2021

markus2330 commented Mar 5, 2021

tucek commented Mar 5, 2021

kodebach commented Mar 5, 2021

tucek commented Mar 5, 2021 •

edited

	const char * pluginName = keyBaseName (cur);
	KeySet * pluginConfig = ksCut (contract, cur);

	// increment ref count, because cur is part of pluginConfig and
	// we hold a reference to cur that is still needed (via pluginName)
	keyIncRef (cur);
	ksRename (pluginConfig, cur, pluginConfigRoot);

		@@ -0,0 +1,87 @@
		--[[
		-- TODO: cannot call std::vector<std::string> function from lua

Integrate kdbEnsure into kdbOpen #3651

Integrate kdbEnsure into kdbOpen #3651

Conversation

kodebach commented Feb 11, 2021

Basics

Checklist

Review

Labels

kodebach commented Feb 11, 2021

kodebach commented Feb 11, 2021

lgtm-com bot commented Feb 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kodebach commented Feb 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kodebach commented Feb 14, 2021

markus2330 left a comment

Choose a reason for hiding this comment

kodebach commented Feb 17, 2021

kodebach commented Feb 17, 2021

markus2330 commented Feb 18, 2021

kodebach commented Feb 18, 2021

markus2330 commented Feb 18, 2021

kodebach commented Feb 18, 2021

mpranj commented Feb 18, 2021

kodebach commented Mar 3, 2021

kodebach commented Mar 3, 2021 • edited

mpranj commented Mar 3, 2021

kodebach commented Mar 3, 2021

markus2330 commented Mar 4, 2021

mpranj commented Mar 4, 2021

kodebach commented Mar 4, 2021

markus2330 commented Mar 5, 2021

tucek commented Mar 5, 2021

kodebach commented Mar 5, 2021

tucek commented Mar 5, 2021 • edited

kodebach commented Mar 3, 2021 •

edited

tucek commented Mar 5, 2021 •

edited