Codegen: Ignore missing required keys in help mode #2944

kodebach · 2019-09-09T16:56:21Z

If the user starts an application that has required keys with -h/--help, it would result in an error and the help message wouldn't be printed. This changes that.

It also adds the regenerated man page that is missing from #2942 (should've been part of
b18447e).

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)
References to issues, e.g. close #X, should be in the commit messages.

Checklist

I added unit tests for my code
I fixed all affected documentation
I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
I updated all meta data (e.g. README.md of plugins and METADATA.ini)
I mentioned every code not directly written by me in THIRD-PARTY-LICENSES

Review

Documentation is introductory, concise and good to read
Examples are well chosen and understandable
Code is conforming to our Coding Guidelines
APIs are conforming to our Design Guidelines
Code is consistent to our Design Decisions

markus2330 · 2019-09-11T19:04:28Z

doc/news/_preparation_next_release.md

@@ -80,6 +80,10 @@ The following section lists news about the [modules](https://www.libelektra.org/

 - We now treat relative paths as relative to `KDB_DB_SPEC` instead of the current working directory. _(Klemens Böswirth)_

+### Spec
+
+- There is now the config key `missing/log` that allows logging of all missing `require`d keys. _(Klemens Böswirth)_


Shouldn't we fail if a required key is missing? Why do we need the log as this should already be part of the error message?

spec can be configured to ignore some or all errors. I don't really know why, but it's always been the case.

This is needed for the case where a program using the codegen API is called in "help mode", i.e. app -h or app --help. In such a case, we want to ignore missing required keys and simply print the help message.
How the missing/log array helps detect this case, is documented in the relevant code:

libelektra/src/libs/highlevel/elektra.c

Lines 143 to 158 in ea74e58

Key * helpKey = ksLookupByName (config, "proc/elektra/gopts/help", 0);

const Key * missingKeys = keyGetMeta (parentKey, "logs/spec/missing");

if (ignoreRequireInHelpMode == 1 && helpKey != NULL && missingKeys != NULL)

{

// proc/elektra/gopts/help was set -> we are in help mode

// logs/spec/missing exists on parentKey -> spec detected missing keys

// we ensured that spec uses conflict/get = ERROR -> the error in kdbGet must be from spec

// --> we are in the error case that should be ignored

// BUT: anything other than helpKey may be incorrect

// and only helpKey should be used anyway

// so create a new config KeySet

Key * helpKeyDup = keyDup (helpKey);

ksClear (config);

ksAppendKey (config, helpKeyDup);

}

There could technically be other problems from the spec plugin as well, but since gopts runs first and was successful, there can't be any errors that would break the help message.

spec can be configured to ignore some or all errors. I don't really know why, but it's always been the case.

This is one of spec's anti-features. Thank you for documenting it but I would be in favor of removing these anti-features. If validation fails, the plugin should return an error.

This is needed for the case where a program using the codegen API is called in "help mode"

Ok. But wouldn't it be better to directly ask for a meta-key which says that we are in the help mode? We want the help to be printed no matter what errors happened in kdbOpen or kdbGet.

If validation fails, the plugin should return an error.

IMO the option to switch to warnings instead might be useful.

it allows spec to report more then one problem at once

sometimes ignoring a problem is fine or even intended (e.g. when fixing the problem)

I agree, however, that ignoring all problems by default is bad. I think it was done, so that problems can still be fixed via kdb, which would fail, if there are warnings or errors.

But wouldn't it be better to directly ask for a meta-key which says that we are in the help mode?

We are doing that. gopts creates the key proc/elektra/gopts/help, if we are in help mode. It is the key that was passed to elektraGetOpts, which contains data for the help message.

We want the help to be printed no matter what errors happened in kdbOpen or kdbGet.

I thought about that, but I am not sure this is possible. Ignoring all errors from spec should be fine, since gopts already ran at that point. But ignoring all errors (no matter from where) could be problematic. Ignoring only missing required keys was the safest solution.

Required keys must be provided by the user/admin and cannot come from another source (Elektra, app developer, etc.). Therefore they will be missing until the user makes changes to the KDB. Before that, no other error should occur (°). If a user runs app for the first time and receives an error about a missing required key, will:

Know what to do and add the key, thereby fixing the problem.

Try app -h/app --help to find out more. The help message may or may not contain useful information. If not they may try 3.

Read some other documentation to find out more. Ideally this leads them to 1.

In any case after this the user definitely know how to interact with the KDB. Since we assumed that there won't be any errors before the KDB was changed, we can assume that the user caused other errors by changing the KDB.

Of course it would be nice to always show the help message, but since it depends on the specification being readable, that is somewhat impossible. One way around that (at least in applications using codegen) is to generate a fallback help message that is used, if we couldn't read the spec. This fallback help message would be based on the spec provided to the code generator. It would therefore not reflect any changes to the spec (e.g. description or opt/help) that the user made.

(°) assuming a correct installation. But broken installations cannot be ignored anyway, since they may interfere with the help message that is generated from the spec.

it allows spec to report more then one problem at once

This would also be possible if the first error is as error and the others as warning.

sometimes ignoring a problem is fine or even intended (e.g. when fixing the problem)

We usually only add warnings during kdbGet() and errors only in kdbSet(). I would also expect that behavior in the spec plugin. We should document this behavior in a decision.

I thought about that,

Very nice write-up. Can you make a decision of what you wrote here?

We usually only add warnings during kdbGet()

There are many cases, where kdbGet() should return an error. The best example would be a broken storage file. The storage plugin should return an error and kdbGet() should abort.

For the spec plugin, it probably makes sense to have to option of switching between warnings and errors. In kdb (and other configuration tools) warnings would be preferred, as a kdbGet() is required before a kdbSet() could fix the problems. In applications on the other hand errors are the better solution. They are easier to detect, since kdbGet() uses a different return code for errors. The handling of actual errors is also easier in the high-level API (see #2952).

Can you make a decision of what you wrote here?

Will do.

There are many cases, where kdbGet() should return an error.

I agree. I made a different proposal: #2955

For the spec plugin, it probably makes sense to have to option of switching between warnings and errors.

I hope this will not be needed once we have #2955 implemented

Will do.

💖

kodebach · 2019-09-15T16:00:36Z

Side note: The fallback help message only works with app -h or app --help, but not with combined short options like app -fh. The reason is simply, that without the spec we don't know which option takes arguments and which doesn't (app -fh might be the same as app -f -h or app -f h). I think this limitation is acceptable.

markus2330

Thank you for the PR! I am not happy about hardcoded -h and --help but otherwise it is a nice PR.

markus2330 · 2019-09-16T19:39:46Z

doc/decisions/highlevel_help_message.md

+## Assumptions
+
+- We assume that the application in question was correctly installed.
+- We assume `gopts` was mounted.


Is this already default?

gopts is not mounted by default no. However, the file generated by the highlevel template, include a kdbEnsure contract, that mounts it, if it is missing.

Please copy this sentence into the decision.

markus2330 · 2019-09-16T19:39:54Z

src/libs/ease/symbols.map


+libelektra_0.9 {


Why this change?

It should have been part of #2916. The functions were not part of any 0.8.x release, so they should not be versioned as libelektra_0.8.

If we won't have another 0.9.x release and go straight to 1.0.0, they should actually be in libelektra_1.0.

We will definitely have a 0.9 release before 1.0.

markus2330 · 2019-09-16T19:41:10Z

src/plugins/gopts/gopts.c

@@ -43,6 +72,7 @@ int elektraGOptsGet (Plugin * handle ELEKTRA_UNUSED, KeySet * returned, Key * pa
 			ksNew (30, keyNew ("system/elektra/modules/gopts", KEY_VALUE, "gopts plugin waits for your orders", KEY_END),
 			       keyNew ("system/elektra/modules/gopts/exports", KEY_END),
 			       keyNew ("system/elektra/modules/gopts/exports/get", KEY_FUNC, elektraGOptsGet, KEY_END),
+			       keyNew ("system/elektra/modules/gopts/exports/ishelpmode", KEY_FUNC, elektraGOptsIsHelpMode, KEY_END),


isinhelpmode or simply helpmode?

Since you already merged this PR: Do you still want the change?

It was only a question. If you like ishelpmode best, we can keep it.

markus2330 · 2019-09-16T19:44:59Z

src/plugins/gopts/gopts.c

+
+	for (int i = 0; i < argc; ++i)
+	{
+		if (strcmp (argv[i], "-h") == 0 || strcmp (argv[i], "--help") == 0)


I do not like that this is hardcoded here. -H is very common (and also used in the kdb tool). -h has man other meanings.

Furthermore this is not properly parsed, e.g. -vh would not be recognized.

I do not like that this is hardcoded here. -H is very common (and also used in the kdb tool). -h has man other meanings.

Using -h/--help for help mode is already hard-coded in elektraGetOpts (there it is properly parsed).

This function is not actually intended to accurately detect help mode (for that one should still check for the presence of proc/elektra/gopts/help. It is only meant in case kdbGet() failed.

Apart from kdb tool, I have never seen -H used in favour of -h. The kdb tool also takes a very unusual approach, since it opens the man-page instead of printing to stdout/stderr.
In fact most tools I know, use only --help (if they support long options).

Furthermore this is not properly parsed, e.g. -vh would not be recognized.

See above #2944 (comment).

Apart from kdb tool, I have never seen -H used in favour of -h.

Because of how the cmdline options in Elektra are implemented every option needs to have a short option. -H was the most natural one as -h is already "human readable".

The kdb tool also takes a very unusual approach, since it opens the man-page instead of printing to stdout/stderr.

It makes a lot of sense to not have a --help output and a separated man page. E.g. git also does it this way.

In fact most tools I know, use only --help (if they support long options).

I would prefer if we only hardcode --help and drop support for -h.

markus2330 · 2019-09-16T19:50:29Z

Thank you!

markus2330 · 2019-09-20T08:49:45Z

Side note: The fallback help message only works with app -h or app --help, but not with combined short options like app -fh. The reason is simply, that without the spec we don't know which option takes arguments and which doesn't (app -fh might be the same as app -f -h or app -f h). I think this limitation is acceptable.

Please make sure that such notes are documented in the documentation. Here nobody will read it.

kodebach · 2019-09-20T10:10:42Z

It is documented in the doxygen comment for elektraGOptsIsHelpMode, I just added the note here, in case you miss the comment.

markus2330 · 2019-09-20T18:04:24Z

Sorry, I do not see it. You mean src/plugins/gopts/gopts.c line 37-44?

kodebach · 2019-09-20T18:14:39Z

You mean src/plugins/gopts/gopts.c line 37-44?

Yes

kodebach added 5 commits September 9, 2019 18:17

doc: regenerate man pages

c8a7fd7

spec: add option to log missing keys

d70254f

codegen: ignore missing required keys in help mode

9b6e1b4

codegen: add release notes

d353b2f

spec: add release notes

ea74e58

This comment has been minimized.

Sign in to view

markus2330 reviewed Sep 11, 2019

View reviewed changes

kodebach added 4 commits September 15, 2019 16:49

doc: add decision about help message

ca78207

ease: fix symbol versions

8bd7e8b

gopts: add function to detect help mode directly

dae86a2

codegen: implement help message fallback

5238a26

kodebach added the ready to merge label Sep 16, 2019

kodebach requested a review from markus2330 September 16, 2019 19:32

markus2330 approved these changes Sep 16, 2019

View reviewed changes

markus2330 merged commit 9c73c76 into ElektraInitiative:master Sep 16, 2019

kodebach deleted the codegen branch September 17, 2019 12:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Codegen: Ignore missing required keys in help mode #2944

Codegen: Ignore missing required keys in help mode #2944

kodebach commented Sep 9, 2019 •

edited

Loading

This comment has been minimized.

markus2330 Sep 11, 2019

kodebach Sep 11, 2019

markus2330 Sep 12, 2019

kodebach Sep 12, 2019

markus2330 Sep 12, 2019

kodebach Sep 13, 2019

markus2330 Sep 14, 2019

kodebach commented Sep 15, 2019

markus2330 left a comment

markus2330 Sep 16, 2019

kodebach Sep 16, 2019

markus2330 Sep 17, 2019

markus2330 Sep 16, 2019

kodebach Sep 16, 2019

markus2330 Sep 17, 2019

markus2330 Sep 16, 2019

kodebach Sep 16, 2019

markus2330 Sep 17, 2019

markus2330 Sep 16, 2019

kodebach Sep 16, 2019

markus2330 Sep 17, 2019

markus2330 commented Sep 16, 2019

markus2330 commented Sep 20, 2019

kodebach commented Sep 20, 2019

markus2330 commented Sep 20, 2019

kodebach commented Sep 20, 2019

	Key * helpKey = ksLookupByName (config, "proc/elektra/gopts/help", 0);
	const Key * missingKeys = keyGetMeta (parentKey, "logs/spec/missing");
	if (ignoreRequireInHelpMode == 1 && helpKey != NULL && missingKeys != NULL)
	{
	// proc/elektra/gopts/help was set -> we are in help mode
	// logs/spec/missing exists on parentKey -> spec detected missing keys
	// we ensured that spec uses conflict/get = ERROR -> the error in kdbGet must be from spec
	// --> we are in the error case that should be ignored

	// BUT: anything other than helpKey may be incorrect
	// and only helpKey should be used anyway
	// so create a new config KeySet
	Key * helpKeyDup = keyDup (helpKey);
	ksClear (config);
	ksAppendKey (config, helpKeyDup);
	}


		libelektra_0.9 {

Codegen: Ignore missing required keys in help mode #2944

Codegen: Ignore missing required keys in help mode #2944

Conversation

kodebach commented Sep 9, 2019 • edited Loading

Basics

Checklist

Review

This comment has been minimized.

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

kodebach commented Sep 15, 2019

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

markus2330 commented Sep 16, 2019

markus2330 commented Sep 20, 2019

kodebach commented Sep 20, 2019

markus2330 commented Sep 20, 2019

kodebach commented Sep 20, 2019

kodebach commented Sep 9, 2019 •

edited

Loading