New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

examples/highlevel/README.md not working #2374

Open
markus2330 opened this Issue Feb 6, 2019 · 20 comments

Comments

Projects
None yet
3 participants
@markus2330
Copy link
Contributor

markus2330 commented Feb 6, 2019

Steps to Reproduce the Problem

Run commands as described in examples/highlevel/README.md

kdb stash
cd examples/highlevel
sudo kdb mount spec.ini 'spec/sw/example/highlevel/#0/current' ni
sudo kdb import spec/sw/example/highlevel/#0/current ni < spec.ini
sudo kdb spec-mount '/sw/example/highlevel/#0/current'
pkgconfig/application

Expected Result

Successful execution of application.

Actual Result

ERROR: The key '/sw/example/highlevel/#0/current/mystring' has the wrong type (expected 'string' but got '(null)').

System Information

  • Elektra Version: master

Further Log Files and Output

kdb getmeta /sw/example/highlevel/#0/current/mystring type
#> Metakey not found
kdb getmeta spec/sw/example/highlevel/#0/current/mystring type
#> string
kdb global-mount 
#>
kdb export /sw/example/highlevel/#0/current ni
;Ni1
; Generated by Nickel Plugin using Elektra (see libelektra.org).

myint = 
myfloatarray/# = 
print = 1
mystring = 
mydouble = 
 = 

[myint]
 default = 0
 type = long

[myfloatarray/#]
 default = 0.0
 type = float

[print]
 default = 0
 type = boolean

[mystring]
 default = 
 type = string

[mydouble]
 default = 0.0
 type = double

[]
 mountpoint = highlevelexamples.conf
kdb mount
#> highlevelexamples.conf on /sw/example/highlevel/#0/current with name /sw/example/highlevel/#0/current
#> spec.ini on spec/sw/example/highlevel/#0/current with name spec/sw/example/highlevel/#0/current
@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Feb 6, 2019

I can't reproduce this, the only thing I noticed is that kdb stash might be broken.
After executing kdb stash, kdb file still reported the same file as before and kdb get produced the same value as before, so the kdb is clearly not in a clean state. Also it removes things like system/info/elektra/constants that should clearly always be present.

The only reason for this bug I can think of is, that your shell treated the second part of the spec-mount line as a comment, because of the # (see syntax highlighting below).

sudo kdb import spec/sw/example/highlevel/#0/current ni < spec.ini

I changed the line to use '...' in #2375 to be on the safe side.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Feb 6, 2019

I can't reproduce this, the only thing I noticed is that kdb stash might be broken.

You are right, can you please report the bug? (If it is different to #1601)

Your comment lead me to the right track. The problems seems to be that system/elektra/globalplugins is not purged by kdb stash.

Try importing:

;Ni1
; Generated by Nickel Plugin using Elektra (see libelektra.org).

postcommit/user/placements/get = pregetstorage postgetstorage
postcommit/user/placements/error = prerollback postrollback
presetstorage = list
precommit = list
pregetstorage = list
postgetstorage = list
postcommit/user = list
 = 
prerollback = list
postrollback = list
postcommit = list
postcommit/user/plugins = /etc/kdb/elektra.ecf
postcommit/user/placements = 
postcommit/user/placements/set = presetstorage precommit postcommit

This seems to cause the problem. At least for me the problem vanished after I did kdb rm -r system/elektra/globalplugins.

@mpranj Do you have an idea why the global plugins influence Elektra in this way?

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Feb 6, 2019

Try importing:

Yes, that results in the error.

You are right, can you please report the bug?

Turns out I just didn't delete the mounted file properly.

Regarding kdb stash: see #1601

That aside, as far as I am concerned this is not a bug in the highlevel example, so we can close this issue.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Feb 6, 2019

No, it is not a bug in the example but nevertheless it is a bug in Elektra.

And maybe the highlevel API should check for the presence of necessary (global) plugins?

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Feb 6, 2019

And maybe the highlevel API should check for the presence of necessary (global) plugins?

Which plugins did you have in mind? Technically none are required. Defaults can be passed to elektraOpen, if one doesn't want to use the spec plugin.

Also the type plugin is currently written in C++ so that might be a problem for some users (LCDproc?). But it shouldn't be too hard to rewrite the plugin to check using the elektraKeyTo* functions from the highlevel API.
Although that would mean, if we start validating in kdbGet as well, we do the same conversion 2 twice in a row throwing away the result once when the user calls elektraGet*. (Using the type plugin right now also converts twice, but using different methods.)

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Feb 7, 2019

Which plugins did you have in mind?

Done properly we should check if the keys present in spec/ is a superset of the defaults keyset passed in elektraOpen. And based on this information, we would know which plugins should be present. Unfortunately, the spec-mount logic is in C++ and we should not pull this dependency in (or only optional). We also have the same problem (deps to C++) with 3-way merge.

elektraGet*

Yes, there should be no check in elektraGet* as every specified elektraGet* is supposed to always succeed.

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Feb 7, 2019

Done properly we should check if the keys present in spec/ is a superset of the defaults keyset passed in elektraOpen

We should either leave it as is or remove the defaults parameter from elektraOpen entirely. The parameter is intended as an alternative to a specification. If the specification is necessary anyway it is useless.

the spec-mount logic is in C++ and we should not pull this dependency in

Yes, that is a problem, especially for LCDproc. I think the best solution is to leave the highlevel API as is. The (optional) automatic mounting of plugins, checking for global plugins etc. can be done via code generation. That way it is also much easier to leave out code that has C++ dependencies, if the user wants to.

We also have the same problem with 3-way merge.

Not really. I don't see a situation where the highlevel API would require a 3-way merge. kdbSet is called immediately when the user calls elektraSet*, meaning ours and base differs (at most) in one key. If kdbSet reports a conflict, there can only be two situations: Either ours and theirs changed different keys, or they changed the same key. In the first case simply calling kdbGet changing the intended key and calling kdbSet again is always the correct solution. For the second case we could either take the value from ours or theirs in a 3-way merge. Because the ours value comes from the elektraSet* call currently executing, we can assume it is the more current value. Therefore both situations can be solved by simply trying again, which is what the highlevel API currently does.

there should be no check in elektraGet*

We still need to convert the string value to the requested type. The type plugin does the same, but ignores the value and just checks for success. So unless we store the conversion result from the type plugin somewhere, we can't avoid this redundant process.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Feb 7, 2019

The parameter is intended as an alternative to a specification. If the specification is necessary anyway it is useless.

Yes, it is an alternative. But the user should be safe against wrong changes in the specification, in particular incompatible changes in types.

Not really. I don't see a situation where the highlevel API would require a 3-way merge.

You are right: with a single key the 3-way merge is trivial.

we can't avoid this redundant process

The problem is not the redundant conversion but that the user might have messed up the specification. If we check the specification (the types) at startup, this problem could not happen.

So to be more concrete, given following application:

kdb_long_t myint = elektraGetLong (elektra, "myint");

the user should be safe against an exit() from Elektra when doing:

kdb setmeta /sw/org/myapp/#0/current/myint type float

Expected behavior: elektraOpen should fail, telling that the type of myint does not match with the build-in spec.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Feb 7, 2019

I copied the post above to #2382 as it is unrelated to this issue.

To close this issue: I think we should have at least some safety check, so that users do not run into this bug without getting a proper error.

The most simple check would be to make sure that globalplugins contains spec?

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Feb 7, 2019

The most simple check would be to make sure that globalplugins contains spec?

We could do that yes. But that would solve much would it? The specification could still be changed, types aren't validated for kdb set, etc. The only thing that would solve is the use of #, _ etc. in the specification. If the specification doesn't use these, the spec plugin is just a redundant iteration over the KDB.

We should either provide a way for the application to tell us exactly what needs to be present, or we leave it as is and say the developer/user has to check that everything is configured correctly. Personally I prefer the second version. My understanding was that the guarantees of the highlevel API only become binding when everything is used properly (i.e. specification present, plugins mounted correctly, etc.), and that the code generation will make that easier.

@mpranj

This comment has been minimized.

Copy link
Member

mpranj commented Feb 7, 2019

Your comment lead me to the right track. The problems seems to be that system/elektra/globalplugins is not purged by kdb stash.

Are you sure that it's not purged? When there is no global config at system/elektra/globalplugins then the default config (spec) is applied. (see mount.c elektraDefaultGlobalConfig()) After kdb stash you should therefore see this default global config.

If this was not what you meant, please clarify.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Feb 8, 2019

But that would solve much would it?

I tried now the high-level tests several times and I most often got no output and only some undocumented exit code. Thus I think it is quite relevant for end users that we check for known problems and inform the users what to do.

Better would be, of course that we either

  1. make sure that needed global plugins are always present (even with broken system/elektra/globalplugins configurations) or
  2. some #868 functionality, where global plugins are simply added on demand for applications that need it.

Are you sure that it's not purged?

Yes, it seems like it, see #1601

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Feb 8, 2019

I tried now the high-level tests several times and I most often got no output and only some undocumented exit code.

Do you mean the test cases or the example? What exit codes, and what did you try that didn't work?

The test cases should print all the error information available.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Feb 8, 2019

Do you mean the test cases or the example?

I tried the examples, and also copied code from the README into the examples.

What exit codes,

The exit code of the executable. (EXIT_FAILURE because onFatalError is invoked)

and what did you try that didn't work?

As described in the top-post of this issue. I was able to reproduce it by simply following the instructions of the README (and the wrong globalplugins config).

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Feb 8, 2019

EXIT_FAILURE because onFatalError is invoked

Then you should also see an error message that tells you what is wrong.

and the wrong globalplugins config

Wrong in what way? The example is designed to work with the default Elektra config, anything else might not work. The spec plugin is needed for the example, but that should be mounted by default.

I was able to produce a few errors, but none of them are problems with the highlevel API or the example:

  • spec-mount seems to be broken, because the type plugin isn't mounted. Instead the hexnumber plugin is mounted for some reason.
  • setmeta also seems to be broken. Calling kdb setmeta user/sw/example/highlevel/#0/current/myfloatarray array #4 completes without error and with exit code 0. However, it not only doesn't set the metadata it actually removes it, if present.
  • The spec plugin is only working partly. When I set /sw/example/highlevel/#0/current/print to "1" it copied the metadata into the user config file. However, specifying the default metakey on myfloatarray/# simply does nothing. kdb get /sw/example/highlevel/#0/current/myfloatarray/#0 still results in a key not found error.
@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Feb 8, 2019

Then you should also see an error message that tells you what is wrong.

Is it also shown if logging is turned off?

Wrong in what way? The example is designed to work with the default Elektra config, anything else might not work.

This is not very satisfactory. It should always work or print an error which leads you to how to fix the problem (e.g. typing kdb global-mount or to mount the specification). It should also always print which key is affected (e.g. in "ERROR: The value 'abc' could not be converted to type 'long'." it is not known which key is the problem).

Please improve the error messages.

setmeta also seems to be broken. Calling kdb setmeta user/sw/example/highlevel/#0/current/myfloatarray array #4 completes without error and with exit code 0. However, it not only doesn't set the metadata it actually removes it, if present.

This problem is because the shell treats # as comment if it is preceded by a space. setmeta with one argument removes the meta data. I agree that this is not very user-friendly. We should have a separate delmeta. Please create an issue.

The spec plugin is only working partly.

Yes, the spec plugin is full of bugs and currently unmaintained. Please report the bugs.

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Feb 8, 2019

Is it also shown if logging is turned off?

The example uses a custom fatal error handler and prints the error description to stderr:

static void onFatalError (ElektraError * error)
{
	fprintf (stderr, "ERROR: %s\n", elektraErrorDescription (error));
	exit (EXIT_FAILURE);
}

in "ERROR: The value 'abc' could not be converted to type 'long'." it is not known which key is the problem

That's true we should fix that...

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Feb 9, 2019

Do you need an issue for the left-overs?

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Feb 9, 2019

I created some issues... I will fix the ERROR messages together with #2381

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Feb 9, 2019

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment