Is kdb.h minimal?

[kodebach]

In #3606 @markus2330 mentioned that kdb.h is supposed to be minimal. I don't think this is the case.

IMO the minimal API for Key would be:

Key * keyNew (const char * name, ...);

const char * keyName (Key * key);
const char * keyUnescapedName (Key * key);
ssize_t keySetName (Key * key, const char * name);

const void * keyValue (const Key * key);
ssize_t keySetString (const Key * key, const char * value);
ssize_t keySetBinary (const Key * key, const void * value, size_t length);

KeySet * keyMeta (Key * key);

int keyDel (Key * key);

Possibly keyLock/keyIsLocked (or a more general keyFlags/keySetFlags) could be considered minimal as well. Also replacing keySetString and keySetBinary with keySetValue with an additional flag would be more minimal.

Anything else regarding Key is at least as much a convenience function as the keyDup alias would be.

The KeySet part of the API is more minimal, but ksLookupByName, ksHead and ksTail are definitely not minimal.

The only part that is actually minimal IMO is the KDB part.

I think this would be a good task for Stefan. Basically answering the question: "What would be a minimal API to replace kdb.h?" Which parts of the answer we want to implement will then be up to us.

[mpranj]

In general I agree that we could/should clean up the API where possible, but I do not agree with all points. I'm having a hard time to select functions which I would throw out of kdb.h.

Also replacing keySetString and keySetBinary with keySetValue with an additional flag would be more minimal.

So you suggest having:

ssize_t keySetValue (const Key * key, const void * value, size_t length, someflag_t flags);

where then according to the flag the length parameter would be ignored? In that particular case I prefer the current API, it seems more convenient to me.

On the other hand I mostly agree with your proposal for the Key API. The question is: how do we handle the rest of the functions which are thrown out of kdb.h?

[kodebach]

ssize_t keySetValue (const Key * key, const void * value, size_t length, someflag_t flags);

It wasn't a very serious suggestion. But I would say, the most convenient API (for this signature) would be: If flags indicates a binary key, we use length, otherwise we use min (strlen (value), length) if length > 0 or strlen(value) if length == 0.

[kodebach]

A general clarification: I'm am of course in favour of keeping the API clean. I'm however, not sure kdb.h should be minimal.

To me "minimal" means the absolute minimum to make the API usable (not useful). For Key that means create, destroy, get/set name, get/set value and access to the metadata keyset. Everything else inside of struct _Key is currently either internal implementation details or could be done based on these functions.

To keep the API clean, I think we should restrict kdb.h to the set of functions that a large majority of the user base will need. This means including things like keyCopy and keyGetMeta, even though they could be replaced (maybe less efficiently) by calling other functions as well.

An alternative (especially to make migration easier) would be: kdbcore.h contains the absolute minimal API, kdb???.h contains often used, but not minimal things like keyCopy and keyGetMeta and kdb.h includes both kdbcore.h and kdb???.h.

[markus2330]

Minimal is related to working with the data structure: every operation the data structure supports efficiently should be available. What can be build from these operations is not minimal anymore and should be moved out of the core (e.g. to libease).

In doc/todo/FUTURE there is already a list of which function might be removed. Removing the meta-functions makes sense to me. They got obsolete with the introduction of keyMeta.

Merging functions by giving them more parameters does not make the API more minimal.

kdbcore.h contains the absolute minimal API, kdb???.h contains often used, but not minimal things like keyCopy and keyGetMeta and kdb.h includes both kdbcore.h and kdb???.h.

Also do not forget about #3098. So maybe elektra/core.h for KeySet (libelektra-core), elektra/kdb.h for KDB (libelektra-kdb).

[kodebach]

supports efficiently

That is where our definitions of minimal differ. To me efficiently doesn't have anything to with minimal.

But even with your definition, I don't think "minimal" shouldn't necessarily be a goal for kdb.h as long as the API is clean.

Now what is "clean"? I think it is very similar to "minimal", but a bit more flexible. For example, I think adding short static inline aliases and helpers (like the new keyDup in #3606) is still "clean" even though it is probably not "minimal".

In that sense, I think keeping keyGetMeta and keySetMeta in kdb.h can work, if they are defined like this:

static inline Key * keyGetMeta (Key * k, const char * meta) {
    return ksLookupByName (keyMeta (k), meta, 0);
}

static inline void keySetMeta (Key * k, const char * meta, const char * value) {
    ksAppendKey (keyMeta (k), keyNew (meta, KEY_VALUE, value, KEY_END));
}

But currently they are more complicated, partly because they translate e.g. type into meta:/type (we should probably remove that for 1.0).

---

Right now we are also just discussing headers, but we should also discuss the shared libraries. I really dislike the idea that I have to link against another library (e.g. libease) for very basic convenience functions. Maybe there needs to be a step in between.

So let's say elektra/core.h for the basic clean/minimal API without any additional convenience functions. Another header e.g. elektra/helpers.h for the static inline aliases and simple non-static inline helper functions. The non-static inline functions declared in elektra/helpers.h are still defined in libelektra-core.so so you don't have to link against another library for a 3 line helper function. Anything more complex or less often used (like the type conversion stuff) will still be part of the separate libelektra-ease.so and declared in elektra/ease.h.

Basically, I want to avoid a situation, where you could use libelektra-core.so on its own, but practically you always use libelektra-ease.so as well, because otherwise things are just too complicated.

BTW I think some of the stuff currently in libelektra-ease.so should be moved into libelektra-core.so. Things like the very basic array name stuff and elektraReadArrayNumber and also elektraKeyGetRelativeName, which a lot of plugins need and which could be done more efficiently with direct access to the struct.

[markus2330]

That is where our definitions of minimal differ. To me efficiently doesn't have anything to with minimal.

This definition is used in C++. They have quite a lot of containers and only offer the methods that can be implemented efficiently. Elektra uses this definition quite some time but not forever. E.g. in the beginning there was also a case-insensitive search of key names which needed O(n). I removed this as it is against minimalism: everyone can iterate over the KeySet and make a case-insensitive comparison themselves (or we would add it to libease if it would be an important functionality for many customers).

Now what is "clean"? I think it is very similar to "minimal", but a bit more flexible. For example, I think adding short static inline aliases and helpers (like the new keyDup in #3606) is still "clean" even though it is probably not "minimal".

But currently they are more complicated, partly because they translate e.g. type into meta:/type (we should probably remove that for 1.0).

Yes, I fully agree.

So let's say elektra/core.h for the basic clean/minimal API without any additional convenience functions. Another header e.g. elektra/helpers.h for the static inline aliases and simple non-static inline helper functions. The non-static inline functions declared in elektra/helpers.h are still defined in libelektra-core.so so you don't have to link against another library for a 3 line helper function. Anything more complex or less often used (like the type conversion stuff) will still be part of the separate libelektra-ease.so and declared in elektra/ease.h.

Yes, I like that idea. Maybe "elektra/aliases.h" states clearer what it is, "elektra/helpers.h" is good, too. If there are no objections I would add it in doc/DESIGN.md.

Basically, I want to avoid a situation, where you could use libelektra-core.so on its own, but practically you always use libelektra-ease.so as well, because otherwise things are just too complicated.

I fully agree with this goal. static inline helper-functions are not a bad idea. But still we have to be careful with them, as they also need to be consistent with everything else.

BTW I think some of the stuff currently in libelektra-ease.so should be moved into libelektra-core.so. Things like the very basic array name stuff and elektraReadArrayNumber and also elektraKeyGetRelativeName, which a lot of plugins need and which could be done more efficiently with direct access to the struct.

This is against the minimalism: if there is any possibility to remove it from the core, we should do so. The core otherwise gets too bloated and not everyone needs arrays or relative names.

[kodebach]

If there are no objections I would add it in doc/DESIGN.md.

I think we can at least start a PR and start a more concrete discussion to decide which functions will be in what header/library.

[markus2330]

I fully agree. Unfortunately #3098 is not yet merged and our ideas are basically on top of this.

[kodebach]

if there is any possibility to remove it from the core, we should do so

Then maybe there should be step between libcore and libease as well. Not everyone needs arrays, but many plugins do support them. Pulling in all of libease just for basic array support seems a bit silly.

[markus2330]

Yes, we need several convenience or higher-level libraries. Designing them without anyone working on them does not make much sense, though.

That some of the libraries are a mess is a direct consequence that most people do not want to share code at all. So I tried to push them to share at least some useful function used at several places. It has advantages (less duplicated code) but also disadvantages (the libs are a mess as they are not designed as a single API).

[kodebach]

I also think the Repo and CMake setup is not really ideal for libraries that are only used in a few places. If you have just one or maybe a handful of functions that are shared between two places, the effort involved in creating a new library hardly seems worth it. Also libease is already such a mess of unrelated things that it is very tempting to just put your shared code there.

[kodebach]

Addendum:

Designing them without anyone working on them does not make much sense, though.

This actually plays well with my idea of having one stable and another unstable convenience library. The stable one has a properly thought out and tested API, while the unstable one is more for things that we needed to share internally (e.g. between plugins), but didn't have time to design a proper API for.

I think we can design at least a stable version of: 1) a limited API for arrays (just read/write array name and validation of array names) 2) maybe the type conversion stuff (the API is very basic, just two kinds of functions repeated for all types.

[markus2330]

I also think the Repo and CMake setup is not really ideal for libraries that are only used in a few places.

What do you mean? You only need to call add_lib with the source files as argument. There is a dedicated folder for libraries.

Too many libraries is also not a solution: then there is a problem about how to find the right library.

I think we can design at least a stable version

For 1.0 I am happy if we get a stable version of Key name, value+metadata and KeySet's basic operations. Still a long way to go there.

[kodebach]

What do you mean? You only need to call add_lib with the source files as argument.

Well that's kind of the first hurdle. We rely heavily on custom CMake code and in my experience that quickly reaches it's limits (happend multiple times to me) and then you suddenly need to either a) extend the custom functions for your use case or b) know every little detail that happens in e.g. add_lib so you can properly add your library in a way that works with elektra-full and elektra-static.

Regarding the structure: There maybe plugins that could share code, but that code shouldn't really be public API. The only way to do that would be to create a library and hope nobody else starts using it. If we don't split by type of component (library, plugin, binding, tool), but instead allow some logical grouping (e.g. one group for all the notification stuff) then creating private libraries would be easier. If its just a few functions and you only create the library for code reuse, it might even make sense to create statically linked libraries (currently we have not support for that).

I admit all that is definitely a very minor issue, but I think that to new contributors our setup can seem very intimidating and adding a new library seems like a big deal.

Too many libraries is also not a solution: then there is a problem about how to find the right library.

Yes, too many public libraries is a problem. If you just want to deal with internal code duplication the way to go are probably private maybe even statically linked libraries. That's what we're missing right now. Currently everything in src/libs is designed to be at least somewhat useful to external users.

[markus2330]

If its just a few functions and you only create the library for code reuse, it might even make sense to create statically linked libraries (currently we have not support for that).

CMake provides support for it, we do not need to add anything for it. Elektra uses it at several places, e.g.: fcrypt uses code from the crypto plugin; adapters of the IO libs; crypto had the
CRYPTO_COMMON_FILES before we removed the different variants.

Currently everything in src/libs is designed to be at least somewhat useful to external users.

I agree, we could use private/static libs more, e.g. the utility library does not need to be public.

[kodebach]

CMake provides support for it

Of course there is CMake support. CMake supports lots of things, but with Elektra it's often a question of: "Can I just use this standard CMake function or is there some special magic somewhere that will break, if I do?"

But yes, for static libraries I think just using standard CMake stuff should work.