TOML storage plugin #3292

bauhaus93 · 2019-11-27T21:40:31Z

Basics

These points need to be fulfilled for every PR:

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.
The PR is rebased with current master.

If you have any troubles fulfilling these criteria, please write
about the trouble as comment in the PR. We will help you.
But we cannot accept PRs that do not fulfill the basics.

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 for my code
I fully described what my PR does in the documentation
(not in the PR description)
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

Reviewers will usually check the following:

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

If you are already Elektra developer:

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 you also
say that everything is ready to be merged.

markus2330

Looks good: please concentrate now on rebasing, fixing build errors and docu. Other features can be added later in separated PRs (simply document limitations in the README.md)

markus2330 · 2019-11-28T08:34:17Z

src/plugins/toml/README.md

+        * Can't associate those info to array top key in the way like file ending comments are preserved
+        * Maybe add own metakey (eg. epilogue/comment/#1)?
+
+## TODOs


This TODOs are enough, no duplication of TODOs in the PR description are needed.

markus2330 · 2019-11-28T08:41:57Z

src/plugins/toml/README.md

+
+    - Don't know where/how exactly to store trailing array comments/newline info
+        * Can't associate those info to array top key in the way like file ending comments are preserved
+        * Maybe add own metakey (eg. epilogue/comment/#1)?


Can you give an example for the problem in an issue? Usually we collect meta-data not belonging to a specific key in the parentKey. Maybe you can simply add comment/# there? Please do not forget to document this, so that we do not get a mess with different behaviors between plugins.

markus2330 · 2019-11-28T08:42:39Z

src/plugins/toml/README.md

+## TODOs
+	- Document functions
+	- proper sorting -> arrays
+	- base64 for binary k/v


This should be simply adding the base64 plugin in infos/needs

markus2330 · 2019-11-28T08:50:55Z

src/plugins/toml/error.c

+		ELEKTRA_SET_INTERNAL_ERROR (root, msg);
+		break;
+	case ERROR_MEMORY:
+		ELEKTRA_SET_OUT_OF_MEMORY_ERROR (root, msg);


In master this takes only one argument, please rebase.

markus2330 · 2019-11-28T08:51:34Z

src/plugins/toml/table_array.c

+			snprintf (fraction, fracSize, "%s/%s", baseName, fracDup);
+			elektraFree (fracDup);
+			keyAddName (childDup, "..");
+		} while (keyRel (parent, childDup) != 0);


keyRel was removed, use keyIs(Direct)Below.

markus2330 · 2019-11-28T08:52:22Z

src/plugins/toml/testmod_toml.c

+
+static void testWriteReadBoolean (void)
+{
+	// can't check without active type plugin, must check how to add plugin dependency to tests


markus2330 · 2019-11-28T08:54:15Z

@sanssecours can you give some feedback about the parsing?

sanssecours

@sanssecours can you give some feedback about the parsing?

I added some comment. However, since this PR is still pretty much a work in progress I stopped the review process for now. I do not think it makes much sense to comment on a pull request that might be updated any time. Overall the stuff I have seen looks good though 😊.

sanssecours · 2019-11-28T12:50:47Z

src/plugins/toml/scalar.c

+	case SCALAR_STRING_BARE:
+		return elektraStrDup (scalar->str);
+	default:
+		ELEKTRA_ASSERT (0, "All possible scalar enums must be handeled, but got into default branch");


Suggested change

ELEKTRA_ASSERT (0, "All possible scalar enums must be handeled, but got into default branch");

ELEKTRA_ASSERT (0, "All possible scalar enums must be handled, but got into default branch");

src/plugins/toml/CMakeLists.txt

sanssecours · 2019-11-28T13:22:17Z

src/plugins/toml/README.md

+
+# TODO: Documentation
+
+## NULL keys


It might make sense to use the null plugin to store these types of key values instead.

src/plugins/toml/utility.h

markus2330 · 2019-12-07T10:22:30Z

doc/METADATA.ini

@@ -1295,3 +1295,14 @@ status = implemented
 usedby/plugins = cache
 type = boolean
 description = tells the cache plugin to remove all cache files during the next `kdbGet` in a safe way.
+
+[tomltype]


I would prefer if it is directly integrated in the types of Elektra (type, enum, array) and only for the tables there is some internal/toml/type or we even add the "tables" to standardtypes. I would prefer the name "section" though (similar to array as extra meta-data).

markus2330 · 2020-01-14T04:52:27Z

@bauhaus93 It would be great if we can finally go towards merging this. Once the build-server says it is okay, we can merge it (at least as experimental plugin).

markus2330 · 2020-01-26T05:40:14Z

src/plugins/toml/node.c

@@ -110,6 +112,9 @@ void destroyTree (Node * node)
 {
 	if (node != NULL)
 	{
+		if (node->type == NT_LIST_ELEMENT) {


mpranj · 2020-03-01T21:03:08Z

@bauhaus93 how is the toml plugin progressing? Regarding the build failures it seems that some problems should vanish if you rebase, but seems like there are some memleaks.

markus2330 · 2020-04-12T10:26:07Z

@bauhaus93 how is the TOML plugin doing? I closed now bugs related to INI as TOML will replace it.

bauhaus93 · 2020-04-14T12:10:23Z

@markus2330 I tried to track down the remaining memory leak(s), but I couldn't figure out where exactly they reside.

The leaks do not appear with valgrind, but only with ASAN. If I compile with -DENABLE_DEBUG=ON, to find the location where the leaked memory was allocated, the leaks disappear.

It seems to me, that the leaks happen exactly once for each test case/invocation of the parser:

1 direct leak of 8 byte
1 indirect leak of 64 byte,
1 indirect leak of 16384 byte

Even if I let the lexer drop every read char or modify the parser to not match anything, the leaks persist. They disappear only, when not calling yyparse at all.

markus2330 · 2020-04-14T12:34:31Z

@sanssecours can we maybe blacklist these leaks? Did you have similar experiences?

sanssecours · 2020-04-14T14:16:07Z

Did you have similar experiences?

As far as I remember, no. However, I did not use Flex and only used the C++ interface of Bison, which makes handling of dynamic memory much easier, since you can just use RAII.

can we maybe blacklist these leaks?

We probably can, but I think it would make sense to check if these memory problems show up with a very simple parsing example that uses Flex and Bison beforehand.

mpranj · 2020-05-04T15:19:40Z

@bauhaus93 can you please rebase and push again? It's weird but it seems that we are missing debug symbols for some packages. Maybe we get more output this time.

markus2330 · 2020-05-07T16:01:28Z

Great to see activity here 💖

scripts/docker/debian/buster/Dockerfile

scripts/docker/debian/sid/Dockerfile

scripts/docker/debian/stretch/Dockerfile

mpranj · 2020-05-07T22:29:00Z

@bauhaus93 thank you for rebasing. Unfortunately it seems the issue was not fixed. From the addresses of the backtrace I would guess its inside libc. I took the liberty to add the corresponding debug symbols on top of your branch. I hope we will see what's going on now.

scripts/docker/debian/stretch/Dockerfile

mpranj · 2020-05-08T10:54:54Z

@bauhaus93 unfortunately we were still missing the correct debug symbols. I now added a fedora 32 asan build in #3434 with bison and flex debug symbols among others. Can you please pull and rebase again.

mpranj · 2020-05-08T13:33:00Z

Thank you for rebasing again. The fedora 32 GCC asan build failed as well with apparently the same problems. Debug symbols for libc, bison, flex are definitely available. Is it possible it uses another lib? If you find the correct lib please add debug symbols to the fedora 32 docker image until we find the underlying problem. Debug symbols can be added in the section below dnf debuginfo-install -y.

The plugin now fills in missing comment metakeys (start/spaces) when writing. Previously, if a comment was created via kdb meta-set, it would miss the start and space metakeys, resulting in the comment written without it's starting symbol, creating invalid TOML files. Fixed an infinte loop, which occurec when the root key was in the keyset. Fixed a test case, which didn't test file ending comments properly. Added some doc for comments.

markus2330 · 2020-08-31T15:59:53Z

Great to see an update here! 💖

bauhaus93 · 2020-08-31T16:43:02Z

@markus2330 I've also contacted the author of the page from which I took the UTF-8 samples and he said I may use the collection. Could there be an issue with the single texts themselves? They mostly are poems/texts 100+ years old.

I've also removed the directory value plugin from the needed plugins for now, otherwise there's a problem on writing (the tomltype metakeys get moved, resulting in an incorrect file) . I will create an issue for this tomorrow for more details.

I've got a question about the build servers: Some of them (eg the FreeBSD ones) exclude the plugin from being built, because flex is missing. Where can I add this dependency? I couldn't find related Dockerfiles in the scripts/docker folder.

markus2330 · 2020-08-31T16:54:40Z

I've also contacted the author of the page from which I took the UTF-8 samples and he said I may use the collection. Could there be an issue with the single texts themselves? They mostly are poems/texts 100+ years old.

Thank you for asking! ❤️

If the author we took it from said it is okay, then we assume that he/she checked. If the text is old enough, the copyright expired anyway and it is public domain (okay to use for us).

I've also removed the directory value plugin from the needed plugins for now, otherwise there's a problem on writing (the tomltype metakeys get moved, resulting in an incorrect file) . I will create an issue for this tomorrow for more details.

Thank you 👍

I've got a question about the build servers: Some of them (eg the FreeBSD ones) exclude the plugin from being built, because flex is missing. Where can I add this dependency? I couldn't find related Dockerfiles in the scripts/docker folder.

It is probably .cirrus.yml for BSD or .travis.yml (both in the root directory as these CIs do not allow the files to be located anywhere else).

Adds the path of flex to the cmake paths, when on apple and using homebrew.

Dockerfile now installs flex and bison.

markus2330 · 2020-09-02T15:03:09Z

It finally passes the CI! I think we can merge it as this PR is already very long and it will be easier if new changes come in a new PR.

mpranj

This is definitely ready to be merged if you ask me. Minor things can always be polished/fixed, and this looks great to me as-is.

Good work @bauhaus93! Ping me if you agree with the merge and remove the draft status.

scripts/docker/cirrus/fedora/Dockerfile

Fixes ElektraInitiative#3474 The problem was, that when having comments associated to table array declarations in a TOML file, the table element root key (eg. tablearray/#0), which normally is not emitted in the keyset, has to be added to it. This caused trouble in the building of the file tree for writing. Fixed the table array index loop, to handle the element root key appropriatly.

Co-authored-by: Mihael Pranjić <[email protected]>

bauhaus93 · 2020-09-02T18:20:11Z

@mpranj Yeah, I think it's ready to merge if the tests pass now. I just fixed the issue with tablearrays @markus2330 pointed out, and if this didn't cause any new problems, it should be fine.

mpranj · 2020-09-03T08:24:06Z

@bauhaus93 thank you so much! It's great to finally have this merged so everyone can test it.

Can you open a new PR where dump is replaced by toml as the default storage plugin, to iron out the last things?

bauhaus93 · 2020-09-03T10:57:31Z

@mpranj Yeah, nice it's finally merged. Yes, I'll create a PR for that.

bauhaus93 · 2020-09-03T12:35:46Z

How would I set toml as the default storage plugin for checking?
Should I just set the proper environment variables (KDB_DEFAULT_STORAGE, KDB_DB_FILE, KDB_DB_INIT - like it's done for the mmap target) in .cirrus.yml for all kind of targets?

markus2330 · 2020-09-03T12:51:40Z

In scripts/cmake/ElektraCache.cmake line 137 you can change KDB_DEFAULT_STORAGE to toml.

kodebach · 2020-09-05T11:26:31Z

src/plugins/toml/utility.c

+const Key * findMetaKey (Key * key, const char * metakeyName)
+{
+	keyRewindMeta (key);
+	for (const Key * meta = keyNextMeta (key); meta != NULL; meta = keyNextMeta (key))
+	{
+		if (elektraStrCmp (keyName (meta), metakeyName) == 0)
+		{
+			return meta;
+		}
+	}
+	return NULL;
+}


@bauhaus93 Any particular reason, why you didn't just use keyGetMeta instead of looping through all metakeys?

Just asking, because it just caused me some confusing while rebasing #3447.

No, there wasn't any. I just somehow missed the existence of keyGetMeta. I'll replace all findMetaKeyoccurences with keyGetMeta, which does exactly what I want. Thanks for the hint!

bauhaus93 added the work in progress label Nov 27, 2019

markus2330 reviewed Nov 28, 2019

View reviewed changes

markus2330 requested a review from sanssecours November 28, 2019 08:54

sanssecours reviewed Nov 28, 2019

View reviewed changes

bauhaus93 force-pushed the plugin_toml branch from fa25438 to 5f2d4d1 Compare November 28, 2019 14:01

markus2330 reviewed Dec 7, 2019

View reviewed changes

bauhaus93 force-pushed the plugin_toml branch from b293494 to 434ed50 Compare January 16, 2020 12:26

markus2330 reviewed Jan 26, 2020

View reviewed changes

bauhaus93 force-pushed the plugin_toml branch from 125fd38 to 4e8ec78 Compare January 31, 2020 15:28

bauhaus93 force-pushed the plugin_toml branch from 3bec983 to e41479e Compare February 10, 2020 13:24

markus2330 mentioned this pull request Apr 14, 2020

elektraKeyCmpOrder with order = 0 #3274

Closed

bauhaus93 force-pushed the plugin_toml branch from 7e555b3 to 794fe19 Compare May 7, 2020 13:35

mpranj reviewed May 7, 2020

View reviewed changes

scripts/docker/debian/buster/Dockerfile Show resolved Hide resolved

mpranj reviewed May 7, 2020

View reviewed changes

scripts/docker/debian/sid/Dockerfile Show resolved Hide resolved

mpranj reviewed May 7, 2020

View reviewed changes

scripts/docker/debian/stretch/Dockerfile Show resolved Hide resolved

mpranj reviewed May 7, 2020

View reviewed changes

scripts/docker/debian/stretch/Dockerfile Outdated Show resolved Hide resolved

bauhaus93 force-pushed the plugin_toml branch from fc0f8e8 to a3a6120 Compare May 8, 2020 12:25

bauhaus93 force-pushed the plugin_toml branch from a3a6120 to 9a9c674 Compare June 9, 2020 13:36

bauhaus93 added 2 commits August 31, 2020 16:56

toml: Added documentation

7fd8a62

bauhaus93 force-pushed the plugin_toml branch from b267362 to 7fd8a62 Compare August 31, 2020 15:06

toml: Fixed readme, added release note

81bf64d

restyled-io bot mentioned this pull request Aug 31, 2020

Restyle TOML storage plugin #3471

Closed

toml: Fixed README

33028df

toml: Another README restyle

a8ae3d6

markus2330 mentioned this pull request Sep 1, 2020

Beginners Questions #3472

Closed

scripts: Added Flex to cirrus/travis targets.

a0f9585

bauhaus93 mentioned this pull request Sep 1, 2020

Toml/Directoryvalue problems #3473

Closed

bauhaus93 added 2 commits September 1, 2020 17:24

toml: Added homebrew flex handler in cmake

3326d32

Adds the path of flex to the cmake paths, when on apple and using homebrew.

docker: updated fedora dockerfile with flex/bison.

66cbf47

Dockerfile now installs flex and bison.

markus2330 mentioned this pull request Sep 2, 2020

Toml/tablearray problem #3474

Closed

mpranj approved these changes Sep 2, 2020

View reviewed changes

scripts/docker/cirrus/fedora/Dockerfile Outdated Show resolved Hide resolved

scripts/docker/cirrus/fedora/Dockerfile Outdated Show resolved Hide resolved

bauhaus93 and others added 3 commits September 2, 2020 19:38

Update scripts/docker/cirrus/fedora/Dockerfile

95a6bbf

Co-authored-by: Mihael Pranjić <[email protected]>

Update scripts/docker/cirrus/fedora/Dockerfile

100a34d

Co-authored-by: Mihael Pranjić <[email protected]>

bauhaus93 removed the work in progress label Sep 2, 2020

bauhaus93 marked this pull request as ready for review September 2, 2020 18:27

mpranj merged commit 15d8cf8 into ElektraInitiative:master Sep 3, 2020

kodebach reviewed Sep 5, 2020

View reviewed changes

	ELEKTRA_ASSERT (0, "All possible scalar enums must be handeled, but got into default branch");
	ELEKTRA_ASSERT (0, "All possible scalar enums must be handled, but got into default branch");

TOML storage plugin #3292

TOML storage plugin #3292

Conversation

bauhaus93 commented Nov 27, 2019 • edited Loading

Basics

Checklist

Review

Labels

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

markus2330 commented Nov 28, 2019

sanssecours 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

markus2330 commented Jan 14, 2020

Choose a reason for hiding this comment

mpranj commented Mar 1, 2020

markus2330 commented Apr 12, 2020

bauhaus93 commented Apr 14, 2020

markus2330 commented Apr 14, 2020

sanssecours commented Apr 14, 2020

mpranj commented May 4, 2020

markus2330 commented May 7, 2020

mpranj commented May 7, 2020

mpranj commented May 8, 2020

mpranj commented May 8, 2020 • edited Loading

markus2330 commented Aug 31, 2020

bauhaus93 commented Aug 31, 2020

markus2330 commented Aug 31, 2020

markus2330 commented Sep 2, 2020

mpranj left a comment

Choose a reason for hiding this comment

bauhaus93 commented Sep 2, 2020

mpranj commented Sep 3, 2020

bauhaus93 commented Sep 3, 2020

bauhaus93 commented Sep 3, 2020

markus2330 commented Sep 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bauhaus93 commented Nov 27, 2019 •

edited

Loading

mpranj commented May 8, 2020 •

edited

Loading