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

Integration of Highlevel API and libopts #2383

Open
kodebach opened this Issue Feb 7, 2019 · 13 comments

Comments

Projects
None yet
2 participants
@kodebach
Copy link
Contributor

kodebach commented Feb 7, 2019

The highlevel API doesn't expose its internal KeySet, so using it together with elektraGetOpts is impossible. Even if we were to expose the KeySet, that wouldn't help. Because of the way we handle conflicts an application would need to call elektraGetOpts after each elektraSet* call.

That means we either need to integrate elektraGetOpts directly into the highlevel API or implement the global plugin mentioned in #1252.

The global plugin seems like the better way, but it has some problems:

  • Getting the required values of argv and envp outside of main is totally different on each OS, so the implementation would need (reliable) #ifdefs. (see also)
  • Testing would be complicated. And rely heavily on the build server for the different OS versions. Also non-shell tests would be hard to do, because we need a separate process.
  • The plugin would need to be disabled for the kdb tool and maybe other applications, because it would produce errors, if multiple conflicting specifications are found below the parent key.

Especially the last point is a huge problem. The only guaranteed solution would be, if we had a way to only enable the plugin, if the application requests it before calling kdbGet. It also doesn't help to mark the parent key of the opts specification with some kind of metadata, because it could still be another application that called kdbGet.

@kodebach kodebach added the question label Feb 7, 2019

@kodebach

This comment has been minimized.

Copy link
Contributor Author

kodebach commented Feb 7, 2019

If we decide against the global plugin, we should close #1252.

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Feb 8, 2019

Yes, I agree that the global plugin is the most beautiful solution. We should implement that but not for the next release.

Testing would be complicated.

This is not the first implementation which has #ifdef for different OS, e.g. the resolver or getenv-interception already has. Testing should not be complicated as the same test cases, should nearly identically work on all OS (unlike resolver, which produces vastly different results with different CMake configurations):

  1. argc/argv parsing is identical on every OS
  2. environment: only the PATH separator is different, but this only influences the input data, not the test case itself

Of course it would be good to have a Windows agent so that we get confirmation, that the Windows specific code is actually working.

Actually, we could also allow : separators on Windows (next to ; separators, which are necessary otherwise people would go mad with PATH and similar) and cmd-line options starting with /. But such features are currently low-priority and we should consult someone with Windows knowledge what the expected behavior is.

The plugin would need to be disabled for the kdb tool and maybe other applications, because it would produce errors, if multiple conflicting specifications are found below the parent key.

I don't think this problem has something to do with the kdb tool: the kdb tool does not have an env/opt specification. And if it would have, why should it differ from any other application? And even if there are problems, we can easily fix the kdb tool. (E.g. the legacy feature that the kdb tool also gets config from old-style paths might be a problem. See src/tools/kdb/cmdline.cpp line 290-300.) But we can remove this legacy feature with 0.9.)

But yes: if users pass too general parentKeys (like / or /sw) it is very likely that specifications are conflicting.

But there are solutions to this problem if we mark the top-level keys of the respective specifications. Then we could either:

  • Deactivate the plugin if multiple top-level keys are found.
  • Use argv[0] to derive the information which the correct top-level key is.
  • Use some external user-supplied knowledge about which specification to use (e.g. --elektra-parent-key or ELEKTRA_PARENT_KEY)
@kodebach

This comment has been minimized.

Copy link
Contributor Author

kodebach commented Feb 8, 2019

Testing should not be complicated as the same test cases

The test cases are quite easy to write, yes. But because we rely on getting argv from the OS, we need to run the test in a separate process. Additionally local testing is hard, because you can only test the version used for your OS.

I didn't mean to suggest the plugin is impossible, just that we shouldn't expect to be easy to implement.

I don't think this problem has something to do with the kdb tool

No kdb was just an example for an application that would likely access very general keys (e.g. spec/).

Deactivate the plugin if multiple top-level keys are found.

Even if we find just one specification, it might still be for a different application. i.e. kdb get --color=never spec/sw/org/myapp/#0/current may find a specification defining --color as an option without an argument.

Use argv[0] to derive the information which the correct top-level key is.

argv[0] is not very reliable.

Use some external user-supplied knowledge

Yes, we need a way for an application to tell kdbGet() "this is the parent key of my specification". If defined, this parent key will then be used in the elektraGetOpts call instead of the one passed to kdbGet(). I just don't know how we could achieve that.

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Feb 8, 2019

The test cases are quite easy to write, yes. But because we rely on getting argv from the OS, we need to run the test in a separate process.

Yes, but high-level integration tests are very useful in any case. We do not have enough of those.

Additionally local testing is hard, because you can only test the version used for your OS.

Within the #ifdef should be only a very few lines of codes. So once argc, argv, environ works for every OS, there should be little need for local testing. But of course, initially it is an effort. The effort, however, will only be on our side. All devs using Elektra will then appreciate that they do not have to care about argc, argv and environ at all.

I just don't know how we could achieve that.

Do we really need to achieve this?

Suppose we have two specifications for color:

[sw/myapp1/something]
opt/long = color
[sw/myapp2/something]
opt/long = color

When the executable is called with --color, we would simply add two proc keys:

proc/sw/myapp1/something
proc/sw/myapp2/something

For the kdb tool we might want to drop all proc/ keys after the configuration phase, so that configuration of kdb itself and the configuration to be manipulated does not get mixed up. For every other application it does not matter if some proc/ keys are added (which are never looked up anyway).

@kodebach

This comment has been minimized.

Copy link
Contributor Author

kodebach commented Feb 8, 2019

I thought of a solution:

Key * parentKey = keyNew("/sw/org/myapp/#0/current");
KDB * kdb = kdbOpen(parentKey);
elektraSetApplication(kdb, keyName(parentKey));
KeySet * ks = ksNew(0, KS_END);
kdbGet(kdb, ks, parentKey);
// ...

The function elektraSetApplication(KDB * kdb, const char * application) would set a key (e.g. proc/elektra/application) in the global KeySet introduced in #2307. The value of this key would then be used by the global plugin its call to elektraGetOpts. The highlevel API could call elektraSetApplication internally.

In this solution an application is identified by the (cascading) parent key of its configuration/specification (which should be unique anyway). An additional benefit would be that other plugins could use this information too, if the need ever arises.

Do we really need to achieve this?

Yes, we do. Your example is an invalid specification and will result in an OPTS_ILLEGAL_SPEC error.

As I explained in the documentation of libopts, if we were to allow multiple specifications of the same option, we would need a lot of additional validation to ensure the specifications are compatible.

Lets take the following specification.

[sw/myapp1/something]
opt/long = color
opt/arg = none
[sw/myapp2/something]
opt/long = color
opt/arg = required

Now suppose we call elektraGetOpts with the parent key /sw and "--color=green" in argv. It will find both specifications and currently throws an error because color is specified twice. We could allow that and check, if the specifications are compatible, which they wouldn't be in this case. We could also allow every duplicate specification no matter, whether they are compatible. elektraGetOpts would still throw an error, because you gave an argument to an option specified as opt/arg = none.

For the kdb tool we might want to drop all proc/ keys

Not if we want to use elektraGetOpts for the kdb tool, which I thought was the plan.

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Feb 9, 2019

In this solution an application is identified by the (cascading) parent key of its configuration/specification

Your solution works and is architectural sound. Some additional thoughts:

  • instead of providing the wrong parentKey, with your solution someone might provide a wrong elektraSetApplicationName.
  • In the profile plugin, we currently hard-code that the application configuration is within /sw/*/*/#*/current. For the options we could also assume that we only have an application, if we are below this path. (Alternatively, we would need to patch the profile plugin to also use elektraSetApplicationName.)
  • It adds one line of code in basically every application (if using the low-level API) but we strive that Elektra reduces the amount of code as much as possible. An alternative would be to have something like elektraDisableGetOpt(kdb), which would then only be needed for tools like kdb (see below).
  • it is hard-coded configuration for Elektra itself, which I would like to avoid. If we add something like this it is only a matter of time if someone comes up with elektraSetSomethingElse or even elektraSetGenericOption (key, value).

if we were to allow multiple specifications of the same option

Thank you for the example. Can this also happen for environment variables?

Not if we want to use elektraGetOpts for the kdb tool, which I thought was the plan.

Of course we want to use elektraGetOpts for all Elektra tools. kdb tool uses kdbGet twice: first to get its own configuration, and then later for the configuration to be manipulated. elektraGetOpts is only needed for the first kdbGet call. Luckily, the second kdb get call also uses another KDB instance, so your proposal should be work as-is.

But why not using the parentKey and disable the cmd-line functionality if the parentKey does not seem to be an application? Only because of the one counter-example where someone accesses the application root itself kdb get user/sw/org/myapp/#0/current?

As you know we want to change kdbGet semantics anyway, so that it always gets everything (because with mmap done by @mpranj it will hopefully not expensive). Then the parentKey is only informative anyway, and can be directly used as "application name".

The real question is actually if we want to force applications to use the /sw/*/*/#*/current hierarchy. E.g. @tom-wa is against that requirement. (Because the name is so long and the bookmarks, see kdb (1) do not work as good as I hoped they would.)

@kodebach

This comment has been minimized.

Copy link
Contributor Author

kodebach commented Feb 9, 2019

It adds one line of code in basically every application

We could still implement the plugin, so that it uses the value from elektraSetApplicationName and if that is not available falls back to the parent key.

Can this also happen for environment variables?

Currently the only way environment variables can be configured is via the key name. If it ends with /# the environment variable is split into an array. Technically one could therefore reuse the same environment variable for multiple keys, but for consistency's sake and in case we add configuration options in future it is also forbidden.

It adds one line of code in basically every application

It adds one line, if the application whats to use the global plugin for libopts. Alternatively a direct call to elektraGetOpts may be used instead.

something like elektraDisableGetOpt(kdb)

kdb tool uses kdbGet twice

Yes that would also be possible. Although I would go for a more general solution. IMO in this case we should allow application developers to tell Elektra which global plugins are required, and which are prohibited before calling kdbGet. Otherwise we may need additional elektraDisable*() functions in future. So basically, we are back at #868 again.

As you know we want to change kdbGet semantics anyway, so that it always gets everything

That is exactly why elektraGetOpts has to be called with the correct parent key.

But why not using the parentKey and disable the cmd-line functionality if the parentKey does not seem to be an application

The question is how would you detect that?

The real question is actually if we want to force applications to use the /sw/*/*/#*/current hierarchy.

I think these keys are quite long. The /sw/*/* part is a good idea because it ensures unique key names, similar to Java packages for example. The /#*/current part may be useful, but it probably ends up being /#0/current most of the time, and that it is just annoying. I don't think we should enforce this hierarchy, but we should strongly recommend it. Again similar to Java packages.

Also once shell completion works correctly for all commands, long key names together with kdb shouldn't be a problem and in code you can just use a macro for your parent key (or the highlevel API).

@markus2330 markus2330 referenced this issue Feb 10, 2019

Closed

command-line-options tutorial #2394

0 of 2 tasks complete
@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Feb 10, 2019

We could still implement the plugin, so that it uses the value from elektraSetApplicationName and if that is not available falls back to the parent key.

Then the plugin would still not know if it should use cmd-line parsing?

Otherwise we may need additional elektraDisable*() functions in future. So basically, we are back at #868 again.

Yes, I am afraid there is no good (*) way around #868. Sometimes applications need to tell what they expect from KDB. And this is the case for cmd-line options. But for cmd-line options we should consider a different default, if said nothing, the cmd-line options should be available.

(*) a workaround would be that applications reset their argv and environ, then the opts plugin could omit to do anything.

The question is how would you detect that?

If the parentKey matches /sw/*/*/#*/current.

I think these keys are quite long. [...]

I agree.

@kodebach

This comment has been minimized.

Copy link
Contributor Author

kodebach commented Feb 11, 2019

We could add elektraDisableOpts and as soon as we have implemented kdbEnsure we change elektraDisableOpts so that it just calls kdbEnsure and also mark it as deprecated. That way we wouldn't break existing applications, but could essentially get rid of the elektraDisableOpts workaround.

Also: Should the plugin actually be global plugin? Couldn't it just be mounted via spec-mount?

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Feb 15, 2019

Of course, it would be very nice if the plugin also works non-globally.

kdbEnsure() is needed nevertheless, as the application wants guarantees if it is mounted or not.

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Feb 15, 2019

Actually, it might be enough if the application check if spec-mount was called. (If everything is part of the specification).

@kodebach

This comment has been minimized.

Copy link
Contributor Author

kodebach commented Feb 15, 2019

kdbEnsure() is needed nevertheless, as the application wants guarantees if it is mounted or not.

Yes, of course, but there is no need to call the libopts plugin for every interaction with KDB. Most kdbGet calls won't even access any command line specifications.

Actually, it might be enough if the application check if spec-mount was called.

Is there already a way to do that or would that just be a different kind of use of kdbEnsure?

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Feb 15, 2019

No, unfortunately there is currently no startup check at all. But it is a must-have as we do not know how reliable the mounting (at installation time) will work. I updated the goal in #690

@markus2330 markus2330 referenced this issue Feb 15, 2019

Open

Improve Elektra's default behaviour #690

0 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment