Elektra High Level API #1412

domhof · 2017-03-13T11:14:27Z

Purpose

First draft of Elektra High Level API.

Checklist

Please only check relevant points.
For docu fixes, spell checking and similar nothing
needs to be checked.

commit messages are fine (with references to issues)
I ran all tests and everything went fine
I added unit tests
affected documentation is fixed
I added code comments, logging, and assertions
meta data is updated (e.g. README.md of plugins)

@markus2330 please review my pull request

markus2330 · 2017-03-13T16:30:34Z

Thank you!

Can you put elektra.c into a separate library? It won't compile (and does not make sense) to put it into the core.

@tom-wa Can you do a review, too?

markus2330

Thank you!

markus2330 · 2017-03-13T16:30:50Z

src/include/elektra.h

+#include "kdbhelper.h"
+#include "kdbtypes.h"
+
+struct _ElektraError


Structs should be hidden!

markus2330 · 2017-03-13T16:31:28Z

src/include/elektra.h

+
+struct _ElektraError
+{
+    int errorNr;


Why is this needed? Do you want to change elektraHasError to return a kdb_long_t?

Yes, we could do something like that (maybe with another function). The idea is that the developer can identify an error-type without parsing the string. Maybe it is not needed, I would say we will see which error-types we will have and can then decide upon that if we keep it or remove it.

The idea is that the developer can identify an error-type without parsing the string.

Yes, sounds useful. For the code generator option we could even provide an enum.

I would say we will see which error-types we will have

They are already defined in src/error/specification.

markus2330 · 2017-03-13T16:32:03Z

src/include/elektra.h

+struct _ElektraError
+{
+    int errorNr;
+    const char * msg;


Shouldn't this be an array if you can hold errorNr errors?

errorNr just identifies the error-type. msg is a readable string that describes it.

markus2330 · 2017-03-13T16:32:48Z

src/libs/elektra/elektra.c

+void elektraClearError (Elektra * elektra)
+{
+    elektraFree (elektra->error);
+    elektra->error = NULL;


Nice and simple!

markus2330 · 2017-03-13T16:33:32Z

src/libs/elektra/elektra.c

+
+    keyDel (nameKey);
+
+    const kdb_long_long_t value = ELEKTRA_LONG_LONG_S (string, NULL, 10);


use keyGetLongLong here?

As far as I know we only have getString and getValue in the layer below. (we decided to remove all typed getters)

markus2330 · 2017-03-13T16:33:53Z

src/libs/elektra/elektra.c

+#include <stdlib.h>
+#include <memory.h>
+
+Elektra * elektraOpen (const char * application)


Please add error handling here!

It is important to understand which guarantees we can give to libraries.

see #1412 fix #1359

markus2330

Nicely done!

markus2330 · 2017-03-17T21:37:02Z

src/libs/highlevel/elektra.c

+    READ_KEY
+
+    kdb_boolean_t value = 0;
+    if (!strcmp(string, "true") || !strcmp(string, "1") || !strcmp(string, "on")) {


Here you introduce the same issue as discussed with hex values in #1388

We already have defined booleans to be 0 or 1.

Furthermore, you need to check here if check/type isn't something else other than boolean.

markus2330 · 2017-03-17T21:37:16Z

src/include/elektra.h

+typedef struct _ElektraError ElektraError;
+typedef struct _Elektra Elektra;
+
+Elektra * elektraOpen (const char * application);


error parameter missing

markus2330 · 2017-03-17T21:37:22Z

src/include/elektra.h

+Elektra * elektraOpen (const char * application);
+void elektraClose (Elektra * elektra);
+
+kdb_boolean_t elektraHasError (const Elektra * elektra);


markus2330 · 2017-03-17T21:37:56Z

src/include/elektraprivate.h

+    KeySet * config;
+    Key * parentKey;
+
+    struct _ElektraError * error;


markus2330 · 2017-03-17T21:38:17Z

src/libs/CMakeLists.txt

@@ -15,3 +15,5 @@ add_subdirectory (plugin)
 add_subdirectory (elektra)

 generate_manpage (elektra-libs FILENAME ${CMAKE_CURRENT_SOURCE_DIR}/README.md SECTION 7)
+
+add_subdirectory (highlevel)


please add newline

markus2330 · 2017-03-17T21:38:49Z

src/libs/highlevel/elektra.c

+keyDel (nameKey); \
+
+#define RELEASE_KEY_AND_RETURN_VALUE \
+keyDel (resultKey); \


is this macro really needed?

markus2330 · 2017-03-17T21:40:12Z

src/libs/highlevel/elektra.c

+#include "elektraprivate.h"
+
+#define READ_KEY \
+Key * const nameKey = keyDup (elektra->parentKey); \


Why duplicate? What about having a searchKey in Elektra and only modify its name?

markus2330 · 2017-03-17T21:41:44Z

src/libs/highlevel/elektra.c

+{
+    READ_KEY
+
+    char * value = elektraMalloc (keyGetValueSize(resultKey));


Do not return allocated memory to the user.

markus2330 · 2017-03-17T21:44:34Z

src/tools/gen/support/c.py

@@ -13,13 +13,13 @@ def funcname(self, key):
 			raise Exception("invalid keyname " + key)

 	def getfuncname(self, key):
-		return "get_"+self.funcname(key)
+		return "elektraGet"+self.funcname(key)


This change would need a fix in the tests. It might be better to have a new py file which inherits from c.py.

Please cut off basename here, or fix specification of your user study example to not contain Sw.

markus2330 · 2017-04-09T18:28:23Z

Thanks for working on it!

Seems you need to rebase (conflicing file .gitignore)

Please remove "work in progress" when I should do a review.

markus2330

There seem to be some serious issues in the tests, unrelated to the resolver.

markus2330 · 2017-04-09T20:42:02Z

tests/ctest/test_elektra.c

+ * @copyright BSD License (see doc/LICENSE.md or http://www.libelektra.org)
+ */
+
+#include <tests_internal.h>


Not needed, tests should do.

markus2330 · 2017-04-09T20:42:44Z

tests/ctest/test_elektra.c

+    ksAppendKey(config, key);
+
+    // Check
+    Key *checkKey = ksLookupByName (config, name, 0);


you are searching for the wrong key here: name != parentKeyName+name

This file was never meant to be added to git, I tried different things until I found out that the KDB could not be loaded in the first place. I'm aware of the inconsistencies, please ignore them, the code will get removed anyway after we have found out why the resolver cannot be loaded (or about the real cause).

Okay, for me it worked except of this.

markus2330 · 2017-04-09T20:43:02Z

tests/ctest/test_elektra.c

+    KDB * handle = kdbOpen (parentKey);
+    kdbGet (handle, config, parentKey);
+
+    printf ("Error occurred: %s\n", keyString (keyGetMeta (parentKey, "error/description")));


How do you know it failed? You need to check the return value of kdbGet.

lldb tells me that the handle is NULL ;-)

Okay, something seems to be very broken, then.

But in this case the error 37 with the description handle or ks null pointer should be set?

markus2330 · 2017-04-09T20:46:21Z

tests/ctest/test_elektra.c

+
+int main (int argc, char ** argv)
+{
+	printf ("KEY      TESTS\n");


Wrong test name.

See comment above. I initially copied test_key.c. This is just a leftover.

markus2330 · 2017-04-09T20:54:25Z

tests/ctest/test_elektra.c

+    kdbGet (handle, config, parentKey);
+
+    // Check
+    Key *checkKey = ksLookupByName (config, name, 0);


Also wrong lookup here.

see comment above

markus2330 · 2017-04-09T21:07:20Z

tests/ctest/test_elektra.c

+
+    // Close
+    kdbClose (handle, parentKey);
+    keyDel (parentKey);


not everything cleaned up, e.g. config

see comment above

markus2330 · 2017-04-09T21:09:03Z

tests/ctest/test_elektra.c

+
+static void test_development ()
+{
+    const char * parentKey = "user/sw/elektra/kdb/#0/current";


Only write in /test for test cases, e.g. user/test/...

Btw. the test cases redirect user to /tmp, so you cannot use kdb get to check the key afterwards.

Thank you! I didn't know about user being redirected to tmp. Is there a documentation for writing tests etc.?

Only doc/TESTING.md. For non-ABI tests it is okay to use gtest, which is much more comfortable and better documented. (And according to @fberlakovich CLion has support for it.)

We can also consider to write ABI tests with gtest. Until now nobody checked what would happen in the case of gtest upgrades. Maybe it will work without issues anyway. (Because gtest is compiled in after all.)

We can also consider a gtest upgrade, we still have 1.7.0 but 1.8.0 was already released.

Improvements of doc/TESTING.md and how we use gtest is of course very welcomed!

markus2330 · 2017-05-03T09:10:39Z

src/error/exporterrors.cpp

@@ -113,7 +113,29 @@ ostream & operator<< (ostream & os, parse_t & p)
 		os << p[i]["macro"] << " " << i << endl;
 	}

+    os << endl << endl;
+
+	/* Temporarily disable enum generation.


Please revert that change, there is little use of passing all enums to the user.

markus2330

Thank you, looks much better now!

Issues should be easy to fix, would be great to have the fixes soon so that we can merge this.

markus2330 · 2017-05-03T16:32:41Z

tests/ctest/test_highlevel.c

+	init (argc, argv);
+
+	test_primitiveGetters ();
+    test_arrayGetters ();


Please reformat. (clang-format, see scripts/reformat-source)

markus2330 · 2017-05-03T16:34:04Z

tests/ctest/test_highlevel.c

+
+    succeed_if (elektraArraySize(elektra, "longDoubleArrayKey") == 2, "Wrong array size");
+    succeed_if (elektraGetLongDoubleArrayElement(elektra, "longDoubleArrayKey", 0) == 1.1L, "Wrong key value.");
+    succeed_if (elektraGetLongDoubleArrayElement(elektra, "longDoubleArrayKey", 1) == 2.1L, "Wrong key value.");


What happens on out-of-array access?

markus2330 · 2017-05-03T16:35:19Z

tests/ctest/test_highlevel.c

+    elektraSetBooleanArrayElement(elektra, "booleanArrayKey", 0, 0);
+    elektraSetBooleanArrayElement(elektra, "booleanArrayKey", 1, 1);
+
+    elektraSetCharArrayElement(elektra, "charArrayKey", 'c', 0);


Please do boundary tests (smallest and largest char, long,...)

markus2330 · 2017-05-03T16:40:19Z

src/libs/highlevel/README.md

+
+int main(int argc, char ** argv)
+{
+  ElektraError * error = NULL;


Please also format examples nicely.

markus2330 · 2017-05-03T16:41:04Z

src/libs/highlevel/README.md

+int main(int argc, char ** argv)
+{
+  ElektraError * error = NULL;
+  Elektra * elektra = elektraOpen ("/sw/yourApplication", &error);


Please use only correct examples (org+version missing)

Changed it to "/sw/org/myapp/#0/current".

markus2330 · 2017-05-03T17:09:23Z

src/libs/highlevel/elektra.c

+
+    if (kdb == NULL)
+    {
+        *error = elektraErrorCreateFromKey(parentKey);


error fatal?

markus2330 · 2017-05-03T17:10:51Z

src/libs/highlevel/elektra.c

+            Key * problemKey = ksCurrent (elektra->config);
+            if (problemKey != NULL)
+            {
+//                ELEKTRA_LOG_DEBUG("problemKey: %s\n", keyName (problemKey));


Why comment out?

markus2330 · 2017-05-03T17:11:17Z

src/libs/highlevel/elektra.c

+
+            kdbGet (elektra->kdb, elektra->config, elektra->parentKey);
+        }
+    } while (ret == -1);


Might not terminate.

markus2330 · 2017-05-03T17:13:53Z

src/libs/highlevel/elektra.c

+//                ELEKTRA_LOG_DEBUG("problemKey: %s\n", keyName (problemKey));
+            }
+
+            kdbGet (elektra->kdb, elektra->config, elektra->parentKey);


Unnecessary kdbGet if kdbSet was successful.

Please take a look at examples/kdbset.c or qt-gui for how to do this correctly. You also need to do a 3-way merge considering who changed the value (for the one key to set).

markus2330 · 2017-05-03T17:14:51Z

src/libs/highlevel/elektra.c

+            Key * problemKey = ksCurrent (elektra->config);
+            if (problemKey != NULL)
+            {
+//                ELEKTRA_LOG_DEBUG("problemKey: %s\n", keyName (problemKey));


Why comment out? Seems to be duplication of code here?

markus2330 · 2017-05-07T17:39:48Z

src/bindings/cpp/benchmarks/benchmark_thread.cpp

@@ -67,8 +67,9 @@ class Environment : public ThreadBoolean
 	: ThreadBoolean (ks, context_, Key ("/test/%layer1%", KEY_CASCADING_NAME, KEY_META, "default", "1", KEY_END)),
 	  nested (ks, context_), person (ks, context_),
 	  profile (ks, context_, Key ("/%layer1%/profile", KEY_CASCADING_NAME, KEY_META, "default", "default", KEY_END)),
-	  bm (ks, context_, Key ("/%layer1%/%layer2%/%layer3%/%layer4%/%layer5%/%layer6%/%layer7%/%layer8%/%layer9%/", KEY_CASCADING_NAME,


Did you use clang-format-3.8? Maybe @sanssecours can give you a tip how to get it (on Mac).

If we would use different versions of clang-format we would keep reverting our changes.

Did you use clang-format-3.8? Maybe @sanssecours can give you a tip how to get it (on Mac).

You can install clang-3.8 via brew install clang-format@3.8.

If we would use different versions of clang-format we would keep reverting our changes.

We could also just use the current version (5.0.0) of the software.

Thanks for giving the tip!

We could also just use the current version (5.0.0) of the software.

Installing clang on all build agents take some time, so I would prefer to wait for @BernhardDenner's build server case study. Furthermore, it needs to be coordinated with all developers. We can discuss it in the next meeting, but for now @domhof please use 3.8.

Thanks, I used the current version (5.0.0). Reverted back to 3.8 for now.

markus2330 · 2017-05-16T18:43:19Z

Btw. before merging please rebase. And it would be great if you can put the module you are working on as short message before the commit messages. (kdbhl or similar)

And as long term perspective we should get rid of the python dep for the simple parts of the code generator (generating the key names and default config).

markus2330 · 2017-06-16T21:31:20Z

What is the status of this PR? Still seems to have a lot of wrong reformatting? Please check the PR at github.

…entation

markus2330 · 2017-07-20T14:29:48Z

Btw. last time we discussed you will start a new PR. What is the status? Please also make sure to work in all feedback you got so far and self-review what you have done.

markus2330 requested a review from tom-wa March 13, 2017 16:29

markus2330 reviewed Mar 13, 2017

View reviewed changes

markus2330 pushed a commit that referenced this pull request Mar 17, 2017

doc: some updates in high-level-api

0a0e7f9

see #1412 fix #1359

markus2330 requested changes Mar 17, 2017

View reviewed changes

markus2330 added the work in progress label Apr 9, 2017

markus2330 mentioned this pull request Apr 9, 2017

Resolver not loaded in test #1458

Closed

domhof force-pushed the elektra_api branch from 5281eaa to 00a4c41 Compare April 9, 2017 20:28

markus2330 reviewed Apr 9, 2017

View reviewed changes

domhof force-pushed the elektra_api branch from 9b7b906 to 45841e6 Compare April 28, 2017 21:11

markus2330 mentioned this pull request May 3, 2017

Split source location of elektra libs #1484

Closed

markus2330 reviewed May 3, 2017

View reviewed changes

markus2330 requested changes May 3, 2017

View reviewed changes

domhof force-pushed the elektra_api branch from a2c08b1 to a99d757 Compare May 7, 2017 14:41

markus2330 reviewed May 7, 2017

View reviewed changes

markus2330 mentioned this pull request May 17, 2017

how to avoid emergent misbehavior in high-level API? #1497

Closed

domhof force-pushed the elektra_api branch from d29ede5 to ce961eb Compare May 18, 2017 07:37

domhof removed the work in progress label May 19, 2017

domhof force-pushed the elektra_api branch 2 times, most recently from 2208e21 to 64f10af Compare June 1, 2017 08:24

markus2330 added the work in progress label Jun 16, 2017

domhof added 6 commits June 21, 2017 09:29

Elektra API interface, elektraGetString and elektraGetLongLong implem…

b55e2ca

…entation

Free error and elektra structs

581b85a

Fix code style

7bab88c

Move high level API to separate module

8448982

Hide implementation of _Elektra and _ElektraError

1b89d8a

Include elektra.h

95530ff

domhof added 17 commits June 21, 2017 09:30

Use lower case key names to adhere to convention

09f2c05

Require C11 for test_highlevel

19fbeb1

NULL check in elektraErrorReset

86b6c94

Implement generic accessors

05d1267

Add generic tests

183ff42

Generate defines

f0ff6ef

Add setters and getters

b779f96

Remove old code

88af167

Apply reformat-source

52195b4

Add array defines and docs

8cdc8bf

Update README.md

5989dfd

Update README.md

da64afa

Apply reformat-source

3dddd2f

Update README.md

1329a90

Simplify type tag generation code

a4dada1

codegen: Favor gen/type over type

e4f7b4b

codegen: Add stringkey entry

2506c77

domhof force-pushed the elektra_api branch from 64f10af to 2506c77 Compare June 21, 2017 07:30

domhof added 4 commits July 18, 2017 17:47

highlevel: Fix macro errors

00a42f7

codegen: Generate generic code for enums

527cd7d

codegen: Move declarations part to elektra header

db55b18

codegen: Generate typed tag and string setters/getters

72b2764

domhof added 6 commits July 23, 2017 20:48

codegen: Add elektra_generic.h to CMakeLists.txt

4c86ed8

highlevel: Prefix defaults with application

571f2ca

highlevel: Init default KeySet with number of parameters

3712553

codegen: Add defines for enum type and conversions

4419d86

highlevel: Prefix internals with __

4d9094d

highlevel: Eliminate elektraSetByString

5b307bf

domhof closed this Aug 2, 2017


		keyDel (nameKey);

		const kdb_long_long_t value = ELEKTRA_LONG_LONG_S (string, NULL, 10);

Elektra High Level API #1412

Elektra High Level API #1412

Conversation

domhof commented Mar 13, 2017

Purpose

Checklist

markus2330 commented Mar 13, 2017

markus2330 left a 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

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

markus2330 left a 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

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

markus2330 commented Apr 9, 2017

markus2330 left a 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

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

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

markus2330 Apr 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markus2330 left a 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

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

Choose a reason for hiding this comment

markus2330 May 7, 2017 • edited

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

markus2330 commented May 16, 2017

markus2330 commented Jun 16, 2017

markus2330 commented Jul 20, 2017

markus2330 Apr 10, 2017 •

edited

markus2330 May 7, 2017 •

edited