CCode Updates & Other Minor Enhancements #2066

sanssecours · 2018-06-11T18:41:29Z

Purpose

This pull request adds support for key name escaping to the CCode plugin. The PR also contains other minor enhancements, including

relative storage support for the Tcl plugin, and
updates to info/status of various plugins

.

Checklist

I checked all commit messages.
I added a Markdown Shell Recorder test for the CCode plugin.
I ran all tests locally and everything went fine.
The PR contains some logging code for the CCode plugin.
The release notes should be up to date.

markus2330

Great maintenance work! Thank you so much!

markus2330 · 2018-06-11T20:35:24Z

doc/help/kdb-find.md

@@ -55,7 +55,7 @@ kdb find 'fizz'
 #> user/tests/find/tests/fizz/buzz
 #> user/tests/find/tostfizz

-sudo kdb umount /tests/find


We should umount nevertheless. The umount feature of the shell recorder should only be there to catch incorrectly written shell recorder scripts.

The umount feature of the shell recorder should only be there to catch incorrectly written shell recorder scripts.

I agree. I removed the auto unmount feature in the latest updates. This way an incorrectly written test will always fail.

Yes, in general tests should fail if they do not properly clean up everything.

I am currently thinking about writing a tutorial of how to setup a computer with Elektra. In this tutorial it would be wanted that everything stays mounted at the end. It is questionable, however, if this tutorial can be checked via the shell recorder because it has many local dependencies, so maybe it won't be run via the shell recorder anyway.

markus2330 · 2018-06-11T20:35:43Z

doc/help/kdb-ls.md

@@ -91,7 +91,7 @@ kdb ls /tests/examples/kdb-ls/ -v
 #> user/tests/examples/kdb-ls/tost
 #> user/tests/examples/kdb-ls/tost/level

-kdb umount /tests/examples


markus2330 · 2018-06-11T20:36:26Z

doc/man/man1/kdb-find.1

@@ -71,7 +71,8 @@ kdb find \'fizz\'
 #> user/tests/find/tests/fizz/buzz
 #> user/tests/find/tostfizz

-sudo kdb umount /tests/find
+kdb rm \-r /tests/find
+kdb umount /tests/find


Why is umount here but not above in the source? This seems to be correct.

Why is umount here but not above in the source?

I guess my script to remove those pesky man pages where only the date changed also reverted the (single-line) update to the man page.

The man page and the ReadMe for kdb-find should be in sync after the last update again.

markus2330 · 2018-06-11T20:37:28Z

doc/news/_preparation_next_release.md

 ### Crypto

 - The `crypto` plugin now uses Elektra's `libinvoke` and the `base64` plugin in order to encode and decode Base64 strings. This improvement reduces code duplication between the two plugins. *(Peter Nirschl)*

+### Haskell
+
+- An issue when building Haskell plugins with a cached sandbox is fixed in case


Thank you for reordering this. We should discuss it in the next meeting how to use your new organization.

markus2330 · 2018-06-11T20:37:57Z

doc/news/_preparation_next_release.md


 ### Regex Dispatcher

 - The plugin [regexdispatcher](https://www.libelektra.org/plugins/regexdispatcher) has been added. It calculates regex representations for
  commonly used specification keywords to be used with the [typechecker](https://www.libelektra.org/plugins/typechecker). Currently the
  keywords `check/range`, `check/enum` and `check/validation` are supported. *(Armin Wurzinger)*

+### Tcl
+
+- The [`tcl`](http://libelektra.org/plugins/tcl) plugin does not fail anymore, if its configuration file does not exist and you trie to


markus2330 · 2018-06-11T20:48:11Z

src/plugins/ccode/README.md

+
+```sh
+# We use `%` as escape character and map the space character (hex: `20`) to the character `_`.
+sudo kdb mount config.tcl user/tests/ccode tcl base64 ccode escape=`bash -c 'printf %x \"%'` chars= chars/20=`bash -c 'printf %x \"_'`


What about not requiring chars=?

That sounds like a good idea, especially since I did not add chars= at first and had to debug why only adding chars/20 did not work. However, for which key should we look in ksLookupByName then? As far as I know ksLookupByName does not support something like /chars* (glob expression) or /chars.* (regex) as argument.

I think the configuration settings passed to the plugin won't be too big. So simply iterate over everything and check if it contains /chars.

markus2330 · 2018-06-11T20:50:19Z

src/plugins/ccode/ccode.cpp

+		if (size > mapping->bufferSize)
+		{
+			mapping->bufferSize = size;
+			mapping->buffer = new unsigned char[mapping->bufferSize];


In modern C++ "new" is avoided. Here there is a big memory leak. You should use an array/vector instead. It avoids the memory leak, size is then implicitely available, and vectors have resize().

In modern C++ "new" is avoided.

I know. I just replaced malloc with new and free with delete.

Here there is a big memory leak.

I do not think so, since I did not change the way the plugin handles memory. If there is a memory leak, then the Linux Travis ASAN build should also fail, which is not the case.

You should use an array/vector instead. It avoids the memory leak, size is then implicitely available, and vectors have resize().

This certainly makes sense (except for the part about the memory leak of course 😊).

The memory leak only occurs if the size is > 1000 (especially in gradually increasing sizes there might be a memleak in every iteration), seems like this is not tested. I did not say you introduced the problem but in C++ it is now easier to fix 😄

And in general it is not safe to translate every malloc -> new because C++ additionally has exceptions which can cause memleaks not possible in C.

markus2330 · 2018-06-11T20:53:23Z

src/plugins/ccode/ccode.hpp

@@ -11,27 +11,35 @@

 #include <kdbplugin.h>

+#ifdef __cplusplus


Is there any reason to include this in C code?

Since C does not support using or extern "C" we have to remove the code below, if we include the header file in testmod_ccode.c.

markus2330 · 2018-06-11T20:55:23Z

src/plugins/ccode/testmod_ccode.c

-	CCodeData * d = get_data ();
-	char buf[1000];
-	d->buf = buf;
+	CCodeData * mapping = get_data ();


Does this function test every possible character?

For maintenance it would be nice if the check_reversibility can be shared between the different code plugins.

Does this function test every possible character?

Nope, not as far as I can tell. I did not write this function though 😊.

Yeah, seems like I was in a hurry finishing my master thesis 😄

markus2330 · 2018-06-11T20:57:41Z

tests/shell/shell_recorder/shell_recorder.sh

+	for line in $TOUMOUNT;
+	do
+		mp=$(printf '%s' "$line" | sed -n 's/.*with name \(.*\)/\1/p')
+		"$KDB" umount "$mp"


We should also warn the user that umount was forgotten here.

markus2330 · 2018-06-12T09:01:24Z

src/plugins/ccode/ccode.cpp

+/** @see elektraDocOpen */
+int elektraCcodeOpen (Plugin * handle, Key * key ELEKTRA_UNUSED)
+{
+	CCodeData * const mapping = new CCodeData ();


Should be unique_ptr, see vector discussion.

markus2330

Thank you, great work! Some things need to be changed, though.

markus2330 · 2018-06-17T20:27:37Z

doc/news/_preparation_next_release.md

+  retrieve the plugin contract. *(René Schwaiger)*
+- The plugin now uses relative key names. This update addresses issue [#51][]. *(René Schwaiger)*
+
+[#51]: https://github.com/ElektraInitiative/libelektra/issues/51


issues.libelektra.org/51 is preferred

markus2330 · 2018-06-17T20:28:33Z

doc/news/_preparation_next_release.md

@@ -211,6 +244,9 @@ Thanks to Michael Zronek and Vanessa Kos.
 - The Markdown Shell Recorder now also tests if a command prints nothing to `stdout` if you add the check `#>`. *(René Schwaiger)*
 - We fixed some problems in the [Markdown Shell Recorder](https://master.libelektra.org/tests/shell/shell_recorder/tutorial_wrapper) test
  of [`kdb ls`](https://master.libelektra.org/doc/help/kdb-ls.md). *(René Schwaiger)*
+- The Markdown Shell Recorder test for [`kdb find`](https://master.libelektra.org/doc/help/kdb-find.md) now removes the configuration file
+  at the end of the test. *(René Schwaiger)*
+- We removed the broken auto unmounting feature from the [Markdown Shell Recorder][]. *(René Schwaiger)*


What is the behavior now?

The Markdown Shell Recorder does not do any auto unmounting. That behavior did not change. I just removed dead code.

markus2330 · 2018-06-17T20:30:12Z

src/bindings/cpp/tests/tests.hpp

-	{                                                                                                                                  \
-		ASSERT_TRUE (x) << y;                                                                                                      \
-	} while (0)
+#define succeed_if(x, y) ASSERT_TRUE (x) << y;


This is not a good change, see https://stackoverflow.com/questions/1067226/c-multi-line-macro-do-while0-vs-scope-block

Oh, I forgot to remove the trailing semicolons. This should be fixed in the latest version of the PR.

markus2330 · 2018-06-17T20:31:10Z

src/plugins/ccode/CMakeLists.txt

-include (LibAddMacros)
+if (DEPENDENCY_PHASE)
+	include (LibAddTest)
+	add_gtest (testmod_ccode


why not add_plugin_test? Why in the dependency phase?

why not add_plugin_test?

I just copied this code from the type plugin. The documentation also states that you should use add_gtest. My favorite approach would be to just use add_plugin, but as far as I can tell this function and add_plugin_test do not support C++ tests for a C++ plugin.

Why in the dependency phase?

Adding the plugin without if (DEPENDENCY_PHASE)… does not work:

CMake Error at cmake/Modules/LibAddTest.cmake:28 (add_executable): add_executable cannot create target "testmod_ccode" because another target with the same name already exists. The existing target is an executable created in source directory "/Users/rene/Documents/Studium/Master Thesis/Configuration Parsing/Source/Elektra/src/plugins". See documentation for policy CMP0002 for more details. Call Stack (most recent call first): src/plugins/ccode/CMakeLists.txt:1 (add_gtest) CMake Error at cmake/Modules/LibAddMacros.cmake:80 (target_link_libraries): Attempt to add link library "elektra-core" to target "testmod_ccode" which is not built in this directory. Call Stack (most recent call first): cmake/Modules/LibAddTest.cmake:34 (target_link_elektra) src/plugins/ccode/CMakeLists.txt:1 (add_gtest) CMake Error at cmake/Modules/LibAddTest.cmake:40 (target_link_libraries): Attempt to add link library "gtest_main" to target "testmod_ccode" which is not built in this directory. Call Stack (most recent call first): src/plugins/ccode/CMakeLists.txt:1 (add_gtest) CMake Error at cmake/Modules/LibAddTest.cmake:43 (target_link_libraries): Attempt to add link library "gtest" to target "testmod_ccode" which is not built in this directory. Call Stack (most recent call first): src/plugins/ccode/CMakeLists.txt:1 (add_gtest) CMake Error at cmake/Modules/LibAddTest.cmake:51 (install): install TARGETS given target "testmod_ccode" which does not exist in this directory. Call Stack (most recent call first): src/plugins/ccode/CMakeLists.txt:1 (add_gtest)

.

add_gtest

Yes, seems like we miss a feature in add_plugin.

Adding the plugin without if (DEPENDENCY_PHASE)… does not work:

I mean that ADDTESTING_PHASE should be the correct phase.

I mean that ADDTESTING_PHASE should be the correct phase.

After I changed the phase add_gtest did not find the source files for the test at configuration time any more. I fixed this problem by adding the code

foreach (file ${ARG_SOURCES} ${source}.cpp) if (EXISTS "${file}") list (APPEND SOURCES "${file}") else () list (APPEND SOURCES "${CMAKE_CURRENT_SOURCE_DIR}/${file}") endif () endforeach ()

to add_gtest. However now the build fails printing an error message about kdb.hpp

In file included from ../src/plugins/ccode/coder.cpp:12: ../src/plugins/ccode/coder.hpp:17:10: fatal error: 'keyset.hpp' file not found #include <keyset.hpp> ^~~~~~~~~~~~

. For now I undid the changes to fix the problem.

markus2330 · 2018-06-17T20:33:25Z

src/plugins/ccode/coder.hpp

+
+#include <keyset.hpp>
+
+using CppKeySet = kdb::KeySet;


please limit scope (could be moved to class Coder)

markus2330 · 2018-06-17T20:37:23Z

src/plugins/ccode/testmod_ccode.cpp

+using CppKeySet = kdb::KeySet;
+using CppKey = kdb::Key;
+
+using ckdb::elektraModulesClose;


It is okay as is but you could also use "using ckdb;".

I know, I prefer to only explicitly “import” the functionality I need though.

markus2330 · 2018-06-17T20:38:28Z

src/plugins/ccode/testmod_ccode.cpp

+
+void testRoundTrip (string const decodedString, string const encodedString = "", CppKeySet config = defaultConfig ())
+#ifdef __llvm__
+	__attribute__ ((annotate ("oclint:suppress[empty if statement]"), annotate ("oclint:suppress[high cyclomatic complexity]"),


Does the code below really emits all these problems? Maybe we should create Macros with these attributes?

Does the code below really emits all these problems?

Yes. The main problems seem to be the macros succeed_if_same and exit_if_fail. Every time we use them OCLint prints the following two warnings:

“too few branches in switch statement”, and

“empty if statement”

.

Maybe we should create Macros with these attributes?

Nah, I think it is okay as it is. These annotations should not look pretty. Ideally we should fix the underlying problems, instead of using annotations to disable the warnings.

markus2330 · 2018-06-17T20:39:11Z

src/plugins/ccode/testmod_ccode.cpp

+			annotate ("oclint:suppress[high ncss method]"), annotate ("oclint:suppress[too few branches in switch statement]")))
+#endif
+{
+	CppKeySet modules{ 0, KS_END };


modern C++ style 👍

I wonder about the spacing of the formatter, though.

markus2330 · 2018-06-17T20:40:48Z

src/plugins/tcl/action.cpp

@@ -19,22 +19,25 @@

 #include "action.hpp"

+#include <kdbease.h>


please add a comment what it is used for (elektraKeyGetRelativeName)

markus2330 · 2018-06-17T20:43:24Z

src/plugins/tcl/tcl.cpp

 		errno = errnosave;
+		parent.release ();


If you happen to start a new C++ plugin: the type plugin solves this much more nicely with a delegate. Then you only have single place where you have to release the ownership.

If you happen to start a new C++ plugin: the type plugin solves this much more nicely with a delegate.

I know, I used the delegate approach for the CCode plugin updates (that are part of this PR).

This update should make failures less likely, since now - the download of the Travis cache, - the download of a Homebrew bottle, and - the download or build of a source package have to fail, for a Homebrew related issue to cause a failed build job.

Before this update the `tcl` plugin would report that it was unable to read the configuration file, if the `KeySet` managed by the plugin was empty.

This update addresses part of issue ElektraInitiative#51.

Reference: https://git.libelektra.org/pull/2066#discussion_r195940677

markus2330 · 2018-06-19T13:35:34Z

Thank you, great job!

sanssecours added bug enhancement work in progress labels Jun 11, 2018

sanssecours added this to the 0.8.24 milestone Jun 11, 2018

sanssecours self-assigned this Jun 11, 2018

sanssecours removed the work in progress label Jun 11, 2018

markus2330 reviewed Jun 11, 2018

View reviewed changes

sanssecours force-pushed the branch 2 times, most recently from d57694c to 4a870d9 Compare June 12, 2018 07:34

markus2330 reviewed Jun 12, 2018

View reviewed changes

sanssecours force-pushed the branch 3 times, most recently from 90b9925 to c08fead Compare June 13, 2018 11:04

sanssecours mentioned this pull request Jun 16, 2018

testshell_markdown should be disabled if bash is not available #2077

Closed

sanssecours force-pushed the branch 2 times, most recently from 8c295c1 to 0519819 Compare June 17, 2018 09:05

sanssecours added the ready to merge label Jun 17, 2018

markus2330 requested changes Jun 17, 2018

View reviewed changes

markus2330 removed the ready to merge label Jun 17, 2018

sanssecours added the work in progress label Jun 18, 2018

sanssecours force-pushed the branch from 0519819 to 2fd7555 Compare June 18, 2018 06:59

sanssecours added 9 commits June 18, 2018 15:40

CCode: Add basic MSR test

2573689

CCode: Simplify elektraHexcodeConvFromHex

cfbd06f

CCode: Simplify code

6a763d1

CCode: Rephrase plugin introduction

16b6935

Tcl: Use C++ Key type for parent key

55ceda1

Tcl: Return directly after contract retrieval

638d46b

Before this update the `tcl` plugin would report that it was unable to read the configuration file, if the `KeySet` managed by the plugin was empty.

Tcl: Use relative key names

a97f741

This update addresses part of issue ElektraInitiative#51.

CCode: Extend Markdown Shell Recorder test

9d173fb

sanssecours added 22 commits June 18, 2018 15:40

CCode: Update documentation of encodeValue

4690aa8

CCode: Update documentation of decodeValue

e6e2368

CCode: Update documentation of encodeName

98ea416

CCode: Update documentation of decodeName

721e7da

CCode: Add documentation for Coder

44106f9

CCode: Add documentation for encodeKeySet

2e80b3a

CCode: Add documentation for decodeKeySet

04c23d6

CCode: Add basic unit test

567d39f

Tests: Remove unused macro

2ed847c

Tests: Simplify macros

fa08d17

CCode: Add roundtrip test

e847a3b

CCode: Test plugin using % as escape character

ca3114f

CCode: Split definition and implementation

45ca39a

CCode: Fix description inside comment banners

e02e06a

CCode: Do not require /chars config key

922c7e7

mINI: Remove unnecessary configuration values

5827ba1

Release: Replace GitHub URL

20fc191

Tests: Remove trailing semicolons from macros

674449c

CCode: Limit scope of using directives

59223fa

CCode: Mark problematic code reported by OCLint

76a0e40

Tcl: Add comment about included header file

140484f

Reference: https://git.libelektra.org/pull/2066#discussion_r195940677

CCode: Fix warnings reported by OCLint

66072ac

sanssecours force-pushed the branch 3 times, most recently from d0370fb to 66072ac Compare June 18, 2018 16:22

sanssecours added ready to merge and removed work in progress labels Jun 18, 2018

markus2330 approved these changes Jun 19, 2018

View reviewed changes

markus2330 merged commit a1fd1ad into ElektraInitiative:master Jun 19, 2018

		@@ -19,22 +19,25 @@

		#include "action.hpp"

		#include <kdbease.h>

CCode Updates & Other Minor Enhancements #2066

CCode Updates & Other Minor Enhancements #2066

Conversation

sanssecours commented Jun 11, 2018 • edited

Purpose

Checklist

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

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 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

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 Jun 19, 2018

sanssecours commented Jun 11, 2018 •

edited