[FLOSS H3] simplify infos/status

[Janldeboer]

Reopening the pull request #3590 after rebasing to the current master.

fixes #666
fixes #3504

Basics

• Short descriptions of your changes are in the release notes
  (added as entry in doc/news/_preparation_next_release.md which
  contains _(my name)_)
  Please always add something to the release notes.
• Details of what you changed are in commit messages
  (first line should have module: short statement syntax)
• References to issues, e.g. close #X, are in the commit messages.
• The buildservers are happy. If not, fix in this order:
  • add a line in doc/news/_preparation_next_release.md
  • reformat the code with scripts/dev/reformat-all
  • make all unit tests pass
  • fix all memleaks
• The PR is rebased with current master.

Checklist

• I added unit tests for my code
• I fully described what my PR does in the documentation
  (not in the PR description)
• I fixed all affected documentation (see Documentation Guidelines)
• I fixed all affected decisions (see Decision Process)
• 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 reuse syntax

Review

• Documentation is introductory, concise, good to read and describes everything what the PR does
• 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

Labels

• Add the "work in progress" label if you do not want the PR to be reviewed yet.
• Add the "ready to merge" label if the basics are fulfilled and no further pushes are planned by you.

[Janldeboer]

There are currently two tests that fail:

The following tests FAILED:
877
17 - testtool_specreader (Failed)
878
181 - testscr_check_kdb_internal_check (Failed)

I'm trying to fix them, but I have difficulties understanding what they even check and how that relates to this changes.

[kodebach]

testtool_specreader tests the code that reads the infos/status data. This includes

libelektra/src/libs/tools/tests/testtool_specreader.cpp

Line 132 in fda27d8

mpd->data[PluginSpec ("c")]["status"] = "productive tested";

But AFAICT tested no longer exists as a status.

The testscr_check_kdb_internal_check runs Plugin::check

libelektra/src/libs/tools/src/plugin.cpp

Line 157 in fda27d8

void Plugin::check (vector<string> & warnings)

on (almost) every plugin. That also includes some checks for infos/status. You changed the allowed infos/status values (PluginDatabase::statusMap), but didn't update the READMEs of the plugins.

[Janldeboer]

Okay, that makes sense, thank you for your help!
As I didn't come up with the changes myself but just reopened an old PR, I'm unsure how exactly this was planned to be implemented.

I currently see two options to resolve the issues:

1. Change the READMEs of all plugins in question
2. Change the allowed values of infos/status so that all old values are still supported

Personally I think the 2. option is more appropriate, do you agree with that?

[kodebach]

@markus2330 What do you say?

[markus2330]

Small updates from changed terminology and decisions.

[markus2330]

Change the READMEs of all plugins in question

Changing all READMEs is needed to fix #666, which purpose is to update/simplify what is allowed in infos/status. So obviously the outdated parts of infos/status must be get rid of.

Please fix scripts/dev/update-infos-status, then most of these updates should be quite easy.

Replacing "nodep" and adding the storage features is something bigger, please start with applying the changes of infos/status to the plugins. I updated #666 with the tasks. So for this PR, it is better to keep nodep (add it back to src/libs/tools/include/plugincontract.h).

[Janldeboer]

Parsing the contract to retrieve all available tags is not working anymore, as the CONTRACT.ini isn't compiled before running the regex, so the tags are available at that point in time.
Shall I just parse the plugincontract.h directly or do we have to compile the CONTRACT.ini so that we can keep using it for the test?

Here is the parsing function for reference, hopefully it gives you a better understanding of what I mean:

# parse CONTRACT.ini and return as dict tag:order
def parseContract ():
	contract = open (os.path.join('doc', 'CONTRACT.ini'), 'r')
	start = re.compile ("\s*\[infos/status\]\s*")
	end = re.compile ("\s*\[\S*\]\s*")
	info = []
	inInfo = False
	for line in contract:
		if end.search(line):
			inInfo = False
		if start.search(line):
			inInfo = True
		if inInfo:
			info.append (line)
	enum = dict ()
	p1 = re.compile ("\".*\"")
	p2 = re.compile ("-?[0-9]+")
	for tag in re.findall("\{\s*\".*\"\s*,\s*-?[0-9]+\s*\}", "".join (info)):
		t = p1.search(tag).group(0)[1:-1]
		v = p2.search(tag).group(0)
		enum[t] = int(v)
	contract.close()
	return enum

[Janldeboer]

Looking at the tags which aren't available anymore (except nodep), I wonder how to replace which ones and which to remove:
concept -> experimental
coverage -> ?
limited -> experimental (?)
readonly -> delete (?)
nodoc -> delete (?)
writeonly -> delete (?)
tested -> tested/unit or tested/shell (?)
conformant -> ?
difficult -> experimental (?)
unfinished -> preview
final -> productive
memleak -> preview (?)
reviewed -> documented (?)
unittest -> tested/unit
specific -> ?
shelltest -> tested/shell
global -> ?
libc -> ?

I adjusted the scripts/dev/update-infos-status to allow deleting and replacing multiple tags with one command, so once we're settled on how the tags shall be changed it can quickly be applied.

For this, I removed the restraint that --del, --set and --add are mutual exclusive and I introduced a --replace flag, to be used like this:
--replace oldtag1 newtag1 oldtag2 newtag2 ...

[Janldeboer]

Another thing I don't fully understand is where the pluginfeatures.h are being used.
The tags are included in the CONTRACT.ini in [infos/features/storage], but part of the contract seems unused.

[Janldeboer]

Looking at the tags which aren't available anymore (except nodep), I wonder how to replace which ones and which to remove

@markus2330 I would appreciate any thoughts on this, as I don’t want to make this decision on my own. Thank you very much!

[markus2330]

You are doing a great job here! Thanks for diving in!

Parsing the contract to retrieve all available tags is not working anymore, as the CONTRACT.ini isn't compiled before running the regex, so the tags are available at that point in time.

This is a problem to be fixed in CMake.

Shall I just parse the plugincontract.h directly or do we have to compile the CONTRACT.ini so that we can keep using it for the test?

For update-infos-status it actually makes sense to parse what we have in the repo. (But if you already implemented otherwise, it is fine, too)

Another thing I don't fully understand is where the pluginfeatures.h are being used.
The tags are included in the CONTRACT.ini in [infos/features/storage], but part of the contract seems unused.

The plan is to use it in src/plugins/*/README.md of the storage plugins. But it might be too much work for you to find out for all the storage plugins which features they have.

About the replacements (unchanged lines are okay as you suggested):

concept -> experimental
+ coverage -> tested/unit // can be cross-checked by looking at https://doc.libelektra.org/coverage/master/debian-bullseye-full/
+ limited -> pluginfeatures.h // this is replaced by pluginfeatures.h
+ readonly -> pluginfeatures.h
+ nodoc -> documented
+ writeonly -> pluginfeatures.h
+ tested -> tested/unit or tested/shell (?) // tested/shell can be determined by grepping for TEST_README in the plugin's CMakeLists.txt
- conformant -> ? // no replacement
+ difficult -> experimental
unfinished -> preview
final -> productive
+ memleak -> memleak // memleak is unchanged, still exists
- reviewed -> documented // reviewed can be discarded
unittest -> tested/unit
+ specific -> pluginfeatures.h limited
shelltest -> tested/shell
+ global -> hook
- libc -> ? // discard

[Janldeboer]

How should I handle the tags that are now covered by pluginfeatures.h?
As far as I see, the script doesn't allow be to bulk edit them.
Instead of the tags readonly and writeonly we now have the features readand write, so I suppose every plugin which is tagged with neither readonly or writeonly should have both features read and write?

Would it be a good approach to remove them first and add them back in a new PR, as we have to check the features used by each plugin anyway?

Checking it the features of all plugins is indeed too much work for me, at least until the deadline tomorrow.

Edit: An alternative approach would be to to create another script that migrates the feature tags to be in infos/features, I'm currently implementing the script.

[Janldeboer]

I successfully migrated all the tags that are features using my script.
However, I wasn't yet able to apply the remaining deletions / replacements:
First, in the list of removed tags I forgot old, orphan and preview.
I decided to remove old and orphan and replace preview by concept

For some reason, the script is completely deleting the READMEs from some of the plugins and I couldn't figure out why. I will join tomorrows consultation hour, so that hopefully we can manage to solve that issue.

[Janldeboer]

old -> obsolete
orphan -> obsolete
preview -> concept

[Janldeboer]

Update on how I will change the tags:
Deleting:

conformant
reviewed
libc
tested
nodoc

Replacing:

coverage -> tested/unit
difficult -> experimental
unfinished -> experimental
final -> productive
unittest -> tested/unit
shelltest -> tested/shell
global -> hook
preview -> experimental
orphan -> obsolete
old -> obsolete

[Janldeboer]

I'm currently running in to a new problem:
Now that the status may contain a /, the generate_readme function in scripts/cmake/modules/LibAddMactos.cmake doesn't work correctly anymore, as the / isn't parsed by the regex.
My attempts to fix the regex failed so far.
Edit: I was able to fix it

[Janldeboer]

@markus2330 @flo91
Unfortunately I wasn't able to finish the PR yesterday, as new problems keep coming up the further the CI gets and it takes a lot of time to fix the issues because I always need to wait for the CI.

I am also unhappy about the way I solved one particular issue:
The plugins spec, c and quickdump had the tag preview before, which was replaced by experimental.
As all experimental plugins are excluded by default, the two were excluded as well, even though they are required for some tests.
I manually added them to the default plugin selection:

ALL;-EXPERIMENTAL;spec;c;quickdump

Removing or changing the experimental tag (maybe to concept?) would probably be the cleaner solution.
What is your opinion on this?

Another more general problem that I noticed is that the check "FULL" is always failing, as Intel x86 builds on macOS are generally not supported anymore by the CI.

[Janldeboer]

So far, two issue still occur that I have trouble to fix:

(Edit: solved now) libelektra-c.so not found

``` running test commands with parent key spec:/tests/script/gen/highlevel/commands error: kdb gen failed: The command /home/jenkins/workspace/libelektra_PR-4758@3/build directory/bin/kdb gen terminated unsuccessfully with the info: Was not able to load such a plugin!

Maybe you misspelled it, there is no such plugin or the loader has problems.
You might want to try to set LD_LIBRARY_PATH, use kdb-full or kdb-static.
Errors/Warnings during loading were:
Sorry, 1 warning was issued ;(
1: Module kdb issued the warning C01200:
Installation: Dlopen failed. Could not load module libelektra-c.so. Reason: libelektra-c.so: cannot open shared object file: No such file or directory
Mountpoint: /
Configfile:
At: /home/jenkins/workspace/libelektra_PR-4758@3/src/libs/loader/dl.c:90

Please report the issue at https://issues.libelektra.org/

This appeared before I added the `c` plugin to the default plugins manually and I hope that it won't appear anymore on the current run of the CI.
Edit: This problem is solved now

selected default ... is not selected in PLUGINS

``` CMake Error at src/plugins/CMakeLists.txt:73 (message): selected default storage (dump) is not selected in PLUGINS, please change KDB_DEFAULT_STORAGE or PLUGINS

CMake Error at src/plugins/CMakeLists.txt:81 (message):
selected default resolver (resolver_fm_hpu_b) is not selected in PLUGINS,
please change KDB_DEFAULT_RESOLVER or PLUGINS

It seems to me like again the plugins are excluded due to changed tags, but in this case both plugins don't have the `experimental` tag. Maybe I'm able to fix this by digging deeper into why the plugins aren't available, but any help would be appreciated.
</details>

[Janldeboer]

Something went wrong with the rebase.

Yes, I still have trouble rebasing, but now the commit history looks fine again

[markus2330]

There are many tests failing: https://build.libelektra.org/blue/organizations/jenkins/libelektra/detail/PR-4758/59/tests

I try to retrigger the build.

Did you try to reproduce it locally?
https://www.libelektra.org/tutorials/run-all-tests-with-docker

[markus2330]

jenkins build libelektra please

[Janldeboer]

There are many tests failing: https://build.libelektra.org/blue/organizations/jenkins/libelektra/detail/PR-4758/59/tests

Yes, as far as I can see they all came down to the same problem, so one fix should be able to fix them all.

Did you try to reproduce it locally? https://www.libelektra.org/tutorials/run-all-tests-with-docker

I will try that now

[Janldeboer]

Testing it locally doesn't work for me.
When running make -j 10 I get this:

/home/jenkins/workspace/src/plugins/kconfig/file_utility.cpp: In member function ‘bool kconfig::FileUtility::isNextCharToken()’:
/home/jenkins/workspace/src/plugins/kconfig/file_utility.cpp:44:2: error: case label value is less than minimum value for type [-Werror=switch-outside-range]
   44 |  case EOF:
      |  ^~~~
/home/jenkins/workspace/src/plugins/kconfig/file_utility.cpp: In member function ‘void kconfig::FileUtility::skipLine()’:
/home/jenkins/workspace/src/plugins/kconfig/file_utility.cpp:76:3: error: case label value is less than minimum value for type [-Werror=switch-outside-range]
   76 |   case EOF:
      |   ^~~~
/home/jenkins/workspace/src/plugins/kconfig/file_utility.cpp: In member function ‘void kconfig::FileUtility::readUntilChar(std::ostream&, const char&)’:
/home/jenkins/workspace/src/plugins/kconfig/file_utility.cpp:130:9: error: comparison is always false due to limited range of data type [-Werror=type-limits]
  130 |   if (c == EOF && isNextCharEOF ())
      |         ^
/home/jenkins/workspace/src/plugins/kconfig/file_utility.cpp: In member function ‘void kconfig::FileUtility::readUntilChar(std::ostream&, const char&, const char&)’:
/home/jenkins/workspace/src/plugins/kconfig/file_utility.cpp:156:9: error: comparison is always false due to limited range of data type [-Werror=type-limits]
  156 |   if (c == EOF && isNextCharEOF ())
      |         ^

[Janldeboer]

@markus2330 I've been looking into this for over an hour now but I honestly have no idea where these problems come from. Especially because the problems just came up after fixing the tag parsing.
Do you have any idea how I can continue troubleshooting?

[markus2330]

Looks like nodep is missing in some core plugin (probably backend plugin). Probably some bug in the parsing was introduced. You could, e.g., print what CMake parses and compare with how it should be.

[kodebach]

I don't think backend is missing. The first kdb mount fails with the message:

The command /home/jenkins/workspace/libelektra_PR-4758/build directory/bin/kdb mount terminated unsuccessfully with the info:
You entered a bad name (<empty>) for a plugin!
A valid name of a plugin is either
modulename or modulename#refname
where both modulename and refname must start with a-z
and then a-z, 0-9 and underscore (_) only
Please report the issue at https://issues.libelektra.org/

I don't know why that happens, but if backend was missing the message would contain something about "Bootstrapping failed", since backend is already used during the bootstrap in kdbOpen.

An even better proof of this is that there are testshell_markdown_ tests that do succeed:

testshell_markdown_blacklist .................   Passed

I think the issue might actually be in the C++ code of the tools library not in CMake. It's probably because of the --with-recommends of the test for hosts. That probably parses some additional data from the infos/* of and encounters a bug somewhere, which leads to it trying to load a plugin with an empty name.

[kodebach]

Also you should only focus on the testshell_markdown_hosts test. All the tests afterwards fail only because the test destroyed the KDB (see also #4826).

[markus2330]

The C++ parsing code is in src/libs/tools/src/plugin.cpp line 169-. Maybe it has problems with spaces at end or similar?

It should now be impossible that the data is out-of-date, as it directly includes plugincontract.h.

@kodebach is there actually some code you know that directly works with "global" and might have problems that it is now called "hook"? The code in src/libs/tools/src/backend.cpp GlobalPlugins::serialize seems to broken anyway?

[kodebach]

is there actually some code you know that directly works with "global"

I know nearly nothing about the C++ tools code. I only did the bare minimum to make kdb mount work in the new-backend PR.

Maybe it has problems with spaces at end or similar?

AFAIK spaces at the end of lines in Markdown files would make the formatting check fail, so there shouldn't be any.

[Janldeboer]

That probably parses some additional data from the infos/* of and encounters a bug somewhere, which leads to it trying to load a plugin with an empty name.

I looked into this before, but couldn't see anything wrong so far.
What I did find suspicious, is that the infos/status and infos/provides get mixed up before being put into split_plugin_providers

Here they are being combined:

file (READ ${CMAKE_CURRENT_SOURCE_DIR}/README.md contents)
	string (REGEX MATCH "- +infos/status *= *([-a-zA-Z0-9 ]*)" CATEGORIES "${contents}")
	string (REGEX REPLACE "- +infos/status *= *([-a-zA-Z0-9 ]*)" "\\1" CATEGORIES "${CATEGORIES}")
	string (REGEX REPLACE " " ";" CATEGORIES "${CATEGORIES}")

	string (REGEX MATCH "- +infos/provides *= *([a-zA-Z0-9/ ]*)" PROVIDES "${contents}")
	string (REGEX REPLACE "- +infos/provides *= *([a-zA-Z0-9/ ]*)" "\\1" PROVIDES "${PROVIDES}")
	string (REGEX REPLACE " " ";" PROVIDES "${PROVIDES}")
	list (APPEND CATEGORIES "ALL" "${PROVIDES}")

	split_plugin_providers (CATEGORIES)

and than the providers are being split by this macro:

macro (split_plugin_providers PROVIDES)
	foreach (PROVIDER "${${PROVIDES}}")
		string (REGEX MATCH "([a-zA-Z0-9]+)/([a-zA-Z0-9]+)" PROVIDER_PARTS "${PROVIDER}")
		string (LENGTH "${PROVIDER_PARTS}" PROVIDER_PARTS_LENGTH)
		if (PROVIDER_PARTS_LENGTH GREATER 0)
			string (REGEX REPLACE "([a-zA-Z0-9]+)/([a-zA-Z0-9]+)" "\\1;\\2" PROVIDER_PARTS "${PROVIDER_PARTS}")
			list (APPEND ${PROVIDES} "${PROVIDER_PARTS}")
		endif ()
	endforeach ()
endmacro ()

While this seems to be a potential source of errors, I couldn't see why exactly it would be:
No category should be parsed as a provider, as all the / from the categories are being replaced by ; now.

The prints that I put in there might help to check that.
Edit: I removed the prints as they were (probably) falsely implement and just led to more problems.
Here is the pipeline with the prints inside.

[kodebach]

The more I look at it the less I understand about this bug...

It happens in both debian-buster-minimal and debian-bullseye-minimal, which are identical jobs except for the OS version. But it does not happen in debian-bullseye-minimal-nodep. That leads me to conclude the issues is with one of the plugin that are not marked as nodep. But no idea, how or why...

On a random guess: Have you tried undoing the changes to CONTRACT.ini that use #include? I didn't find any code that would read CONTRACT.ini directly, but maybe the # is what's confusing things.

[Janldeboer]

The more I look at it the less I understand about this bug...

I'm glad that I'm not the only one feeling like this 🥲

On a random guess: Have you tried undoing the changes to CONTRACT.ini that use #include? I didn't find any code that would read CONTRACT.ini directly, but maybe the # is what's confusing things.

Worth a try, I'll do so now.

[Janldeboer]

I just noticed one other thing in the contract:

[infos/status]
type = vector<int enum #include "src/libs/tools/include/plugincontract.h">

Integers might not big enough the handle the value of the default tag (64000).
I'm not sure what kind of integers we got here, but long was used before so I switched back to that now.

[Janldeboer]

On a random guess: Have you tried undoing the changes to CONTRACT.ini that use #include? I didn't find any code that would read CONTRACT.ini directly, but maybe the # is what's confusing things.

Unfortunately, the same error persists.

[github-actions]

I mark this stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping by writing a message here or create a new PR with the remainder of this PR.
Thank you for your contributions 💖

[github-actions]

I closed this now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new PR with the remainder of this PR.
Thank you for your contributions 💖