Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

Highlevel API #2293

Merged
merged 197 commits into from
Jan 8, 2019
Merged

Highlevel API #2293

merged 197 commits into from
Jan 8, 2019

Conversation

kodebach
Copy link
Member

@kodebach kodebach commented Nov 7, 2018

Basics

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains *(my name)*)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)

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
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins)

@kodebach kodebach self-assigned this Nov 7, 2018
@kodebach
Copy link
Member Author

kodebach commented Nov 7, 2018

documentation, code comments and tests are still WIP
API and functionality is more or less done

@markus2330
Copy link
Contributor

markus2330 commented Dec 10, 2018

Seems like there are still memleaks (tests fail). It would be great if we can get this merged soon.

@kodebach
Copy link
Member Author

I fixed as much as I could, but there are still some problems left, which I wasn't able to solve:

  • Jenkins Build debian-stable-full-xdg fails in testcpp_kdb because of a conflict
  • Travis Builds 🍏 Clang and 🍏 MMap fail because of timeouts in testio_ev
  • Travis Build 🍏 Clang ASAN fails because of a memleak (I will investigate this, but the memleak does not occur under Linux and I don't own a Mac, so someone else might be quicker)
  • Both Travis FULL Builds fail because of Linker Errors. I can reproduce the error locally, but I don't now which part of the CMake config I need to change.

@kodebach
Copy link
Member Author

  • Travis Build 🍏 Clang ASAN fails because of a memleak (I will investigate this, but the memleak does not occur under Linux and I don't own a Mac, so someone else might be quicker)

e9019c3 may have fixed this

@kodebach
Copy link
Member Author

kodebach commented Dec 12, 2018

@markus2330 I tried to change the CMake configuration some more, but I wasn't able to fix the problems with the FULL builds. I think someone else has to take a look at the problem

Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good! Finally we approach the final API!

The linker problems seems to be fixed by @sanssecours already. The failing test seem to be caused by a missing tag, see review comments.

tests/ctest/test_highlevel.c Outdated Show resolved Hide resolved
tests/ctest/CMakeLists.txt Outdated Show resolved Hide resolved
tests/ctest/test_highlevel.c Outdated Show resolved Hide resolved
tests/cframework/tests.h Show resolved Hide resolved
src/tools/gen/support/c_elektra.py Show resolved Hide resolved
src/include/elektra.h Show resolved Hide resolved
src/include/elektra.h Outdated Show resolved Hide resolved
@@ -0,0 +1,58 @@
number:0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept for (highlevel) errors?

@Piankero maybe you can also help here.

src/error/exporterrors_highlevel.cpp Outdated Show resolved Hide resolved
doc/decisions/high_level_api.md Outdated Show resolved Hide resolved
@sanssecours
Copy link
Member

The linker problems seems to be fixed by @sanssecours already.

Unfortunately no 😢. I think the latest commit fixed the linker problem of the full build on macOS, but also introduced multiple problems in the Linux build. I might work again on the PR later, but for now I have to go to work…

@kodebach
Copy link
Member Author

@sanssecours Thanks for your help, I might be able to fix it myself now. I didn't think the order of the add_subdirectory commands would matter.

@kodebach
Copy link
Member Author

I finally made some progress with the linker error....

I first built the CMake target elektra-highlevel with add_subdirectory (highlevel) before add_subdirectory (elektra) (variant A) in the relevant CMake file and then the other way (variant B) arround.

For both versions I ran nm libelektra-highlevel.so | cut -c 18-. I then compared the output und got the following diff (shortened):

41,42c41,42
< T _elektraGetBooleanArrayElementByTag
< T _elektraGetBooleanByTag
---
> t _elektraGetBooleanArrayElementByTag
> t _elektraGetBooleanByTag
45,46c45,46
< T _elektraGetCharArrayElementByTag
< T _elektraGetCharByTag
---
> t _elektraGetCharArrayElementByTag
> t _elektraGetCharByTag
[...]
222,235c222,235
< D KDB_TYPE_BOOLEAN
< D KDB_TYPE_CHAR
< D KDB_TYPE_DOUBLE
< D KDB_TYPE_ENUM
< D KDB_TYPE_FLOAT
< D KDB_TYPE_LONG
< D KDB_TYPE_LONG_DOUBLE
< D KDB_TYPE_LONG_LONG
< D KDB_TYPE_OCTET
< D KDB_TYPE_SHORT
< D KDB_TYPE_STRING
< D KDB_TYPE_UNSIGNED_LONG
< D KDB_TYPE_UNSIGNED_LONG_LONG
< D KDB_TYPE_UNSIGNED_SHORT
---
> d KDB_TYPE_BOOLEAN
> d KDB_TYPE_CHAR
> d KDB_TYPE_DOUBLE
> d KDB_TYPE_ENUM
> d KDB_TYPE_FLOAT
> d KDB_TYPE_LONG
> d KDB_TYPE_LONG_DOUBLE
> d KDB_TYPE_LONG_LONG
> d KDB_TYPE_OCTET
> d KDB_TYPE_SHORT
> d KDB_TYPE_STRING
> d KDB_TYPE_UNSIGNED_LONG
> d KDB_TYPE_UNSIGNED_LONG_LONG
> d KDB_TYPE_UNSIGNED_SHORT

It seems all of the symbols that result in undefined reference errors in variant B, are not exported from libelektra-highlevel.so in this configuration.

I am not sure what needs to change in order for theses symbols to be exported, but maybe @sanssecours or somebody else knows what to do with this information.

@sanssecours sanssecours force-pushed the elektra_api branch 2 times, most recently from 26d7be2 to 84ceb62 Compare December 25, 2018 05:13
@sanssecours
Copy link
Member

I just pushed a few commits that should fix the linker problems. I am not sure that adding KDB* and _elektra* to libelektra-symbols is that great of a solution though…

@sanssecours sanssecours force-pushed the elektra_api branch 4 times, most recently from 96eb88c to fe98762 Compare December 25, 2018 18:03
Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the great work! Now everything seems to be ok, further changes in the next PR please!

return -1;
}

// use API
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add at least one getter or do not close elektra here.

No need to specify the base path `/sw/org/myapp/#0/current` anymore, as the high-level API keeps track of that for you.
The API supports the CORBA types already used by some plugins. The high-level API should also be used in combination
with a specification (`spec-mount`). When used this way, the API is designed to be error and crash free while reading values.
Writing values, can of course still produces errors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

produce

*
**************************************/

const char * elektraGetValue (Elektra * elektra, const char * name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be const void *

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function calls keyString() internally, so it has the same return type.

If the function should support binary data, I would propose this signature instead:

const void * elektraGetValue (Elektra * elektra, const char * name, size_t * size);

Passing NULL to size would be allowed, that way one could use (const char *) elektraGetValue (elektra, "mykey", NULL); to get the current behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markus2330 any comment?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay.

I would recommend to not support binary data for now. So simply remove the *Value* functions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But your proposal is also ok. (If you have already implemented, documented and tested it.)

*
**************************************/

void elektraSetValue (Elektra * elektra, const char * name, const char * value, KDBType type, ElektraError ** error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also void *

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as for elektraGetValue, if we want to support binary data, we need to add a size parameter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

* @param DISABLE_UNDEF_PARAMETERS define to disable undefining of parameters after the macro. Use if parameters
* are used within another supermacro.
* @param CODE_ONLY optional, defaults to 0. Set to 1 to only generate to function body. This is useful, if you want to create a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generate the?

key-values, create a separate handle for each thread to avoid concurrency issues.

#### Configuration
Currently there is only one way to configure an `Elektra` instance:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too early in document?

recommended you either use `atexit()` in you application or set a custom callback, to make sure you won't leak memory.

The callback should interrupt the thread of execution in some way (e.g. by calling `exit()` or throwing an exception in C++). It should
not return to the calling function. If it does, the behaviour is generally undefined, getter-functions will, however, most likely return 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It must not return to the calling function. (No further explanation needed.)

void elektraFatalErrorHandler (Elektra * elektra, ElektraErrorHandler fatalErrorHandler)
```
This allows you to set the callback called by Elektra, when a fatal error occurs. Technically a fatal error could occur any time, but
the most common use case for this callback is inside of functions that do not take a separate `ElektraError` argument. For example,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also consider to use the fatal error handler if the user passed 0 to the functions that take an ElektraError argument.

## Reading and writing values

### Key names
When using `KDB` and `KeySet` directly you would have to specify the whole key name to access a value. In the high-level API you do not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simply say how it is for the high-level API.

passing them only `"message"` as the name for the configuration value.

### Read values from the KDB
A typical application will want to read some configuration values at start. This should be made as easy as possible for the developer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be much earlier.

@markus2330 markus2330 merged commit 0b862e9 into ElektraInitiative:master Jan 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants