Remove error code generation #2976

ghost · 2019-09-18T07:05:13Z

Basics

Check relevant points but please do not remove entries.
Do not describe the purpose of this PR in the PR description but:

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

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

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

ghost · 2019-09-18T07:06:51Z

@kodebach @markus2330 Can you please help me with the Cmake fixing? I try to remove the code generation but I cannot fix the Cmake dependencies correctly (without probably severe time investment)

Currently I receive the following error message:

In file included from /home/michiz/UniRepos/elektra/src/include/kdbprivate.h:12:0,
                 from /home/michiz/UniRepos/elektra/src/libs/elektra/global.c:10:
/home/michiz/UniRepos/elektra/src/include/elektra.h:13:10: fatal error: elektra/errors.h: No such file or directory
 #include <elektra/errors.h>
          ^~~~~~~~~~~~~~~~~~

kodebach · 2019-09-18T11:57:54Z

AFAIK the stuff that is generated from specification_highlevel (including elektra/errors.h) is not used anywhere and can be removed completely.

kodebach · 2019-09-18T12:04:26Z

Similarly elektra/errorcodes.h (those codes shouldn't be used, we decided to use the same codes everywhere) and elektra/errorsprivate.h (already part of kdbprivate.h) should be removed.

kodebach · 2019-09-18T12:05:25Z

src/include/CMakeLists.txt

@@ -170,6 +170,7 @@ install (FILES kdbextension.h
 	       "${CMAKE_CURRENT_BINARY_DIR}/kdbtypes.h"
 	       kdbhelper.h
 	       "${CMAKE_CURRENT_BINARY_DIR}/kdb.h"
+	       "${CMAKE_CURRENT_BINARY_DIR}/kdberrors.h"


kdberrors.h should not be installed, it shouldn't be used outside of Elektra. Why did you add it?

I removed it. I was not aware that this should not be installed

@kodebach are you sure about that? The file might be helpful for out-of-source-tree development of plugins. As this is currently not a priority, I think it is a good thing to not install any private files.

If we install private files, we should install in a subdirectory (private).

The old version of kdberrors.h wasn't suitable for installation IMO. The new version could be installed, but we should first decide, whether or not we want to remove the F variants of the macros (see #2490 (comment)).

I'll see how the new header file looks like.

kodebach · 2019-09-18T12:06:38Z

src/include/CMakeLists.txt

@@ -190,7 +191,7 @@ install (FILES kdbextension.h

 add_custom_target (elektra_config_headers ALL
 		   DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/kdb.h ${CMAKE_CURRENT_BINARY_DIR}/kdbconfig.h
-			   ${CMAKE_CURRENT_BINARY_DIR}/kdbversion.h)
+			   ${CMAKE_CURRENT_BINARY_DIR}/kdbversion.h ${CMAKE_CURRENT_BINARY_DIR}/kdberrors.h)


Not sure, what this custom target is, but I'm pretty sure kdberrors.h shouldn't be in here. Even if it was included, the ${CMAKE_CURRENT_BINARY_DIR}/ part is wrong, since that is not where the header is located anymore.

markus2330 · 2019-09-18T12:10:30Z

There is some formatting problem in "function (add_lib name)". which makes the PR hard to read.

The error message is very clear, it cannot find elektra/errors.h. This most likely has nothing to do with CMake as no deps are needed for non-generated header files.

kodebach · 2019-09-18T12:14:27Z

src/include/kdberrors.h

+#include <string.h>
+
+#ifdef __cplusplus
+using namespace ckdb;


@markus2330 I just saw this line... We really should remove that, using directives in headers are very much not good practice. I suspect however, some files already rely on this line being here, so this could be a rather big change.

Yes, this definitely need to be removed. Thanks for the hint. I did not look so far into the PR.

I think also some other things will come up as kdberrors.h will be much easier to read in the non-generated way.

kodebach · 2019-09-18T12:21:16Z

The error message is very clear, it cannot find elektra/errors.h. This most likely has nothing to do with CMake as no deps are needed for non-generated header files.

No part of the header is actually used anywhere, so the #includes as well as the file itself, can be removed. All elektraError* functions for creating errors are manually implemented in elektra_error.c and declared either in elektra/error.h or kdbprivate.h. It has been this way, since we switched to the new error codes.

ghost · 2019-09-19T13:57:38Z

There is some formatting problem in "function (add_lib name)". which makes the PR hard to read.

I upgraded my cmake-format. It should be easier to read now.

@kodebach was so kind and sent me his version of refactoring from the generation which he had done a time ago. Big thanks!
Unfortunately I now run into the following problem which is very confusing as I have not touched anything there:

.../libelektra/src/plugins/ccode/ccode.cpp: In function ‘KeySet* {anonymous}::contract()’:
.../libelektra/src/plugins/ccode/ccode.cpp:30:116: error: ‘keyNew’ was not declared in this scope
  return ksNew (30, keyNew ("system/elektra/modules/ccode", KEY_VALUE, "ccode plugin waits for your orders", KEY_END),
                                                                                                                    ^
.../libelektra/src/plugins/ccode/ccode.cpp:30:116: note: suggested alternative:
In file included from .../libelektra/src/include/kdbplugin.h:12:0,
                 from .../libelektra/src/plugins/ccode/ccode.hpp:12,
                 from .../libelektra/src/plugins/ccode/ccode.cpp:9:
.../libelektra/build/src/include/kdb.h:140:6: note:   ‘ckdb::keyNew’
 Key *keyNew(const char *keyname, ...);

Do you have a clue there?

kodebach · 2019-09-19T20:51:56Z

Do you have a clue there?

Ah, that is probably what happens, when we remove using namespace ckdb from kdberrors.h...
Basically, there will be a few files that (maybe unknowingly) relied on the fact that kdberrors.h imports the ckdb namespace.

gcc actually tells you the solution already:

[...] note: suggested alternative:
[...]
[...] note:   ‘ckdb::keyNew’

But for .cpp files it should be possible to add using namespace ckdb; to the top of the file. That is probably easier than adding ckdb:: in all the places.

For headers, we shouldn't do that (same reason why kdberror.h shouldn't contain the line). Most likely you just need to replace Key with ckdb::Key (or KeySet with ckdb::KeySet) in a declarations. If there are any headers that cause more complicated problems, I'll need more specific infos.

ghost · 2019-09-20T07:13:42Z

It looks like I could fix most of those problems on my own with your help (thank you!) but now ran into a problem concerning the highlevel files:

make[2]: *** No rule to make target 'src/error/elektra_errors.c', needed by 'src/libs/highlevel/CMakeFiles/elektra-highlevel-objects.dir/__/__/error/elektra_errors.c.o'.  Stop.
CMakeFiles/Makefile2:16577: recipe for target 'src/libs/highlevel/CMakeFiles/elektra-highlevel-objects.dir/all' failed
make[1]: *** [src/libs/highlevel/CMakeFiles/elektra-highlevel-objects.dir/all] Error 2
Makefile:162: recipe for target 'all' failed
make: *** [all] Error 2

kodebach · 2019-09-26T15:17:51Z

These lines should be remove:

libelektra/src/libs/highlevel/CMakeLists.txt

Lines 3 to 6 in 5519cb8

    
           set_source_files_properties (${PROJECT_BINARY_DIR}/src/error/elektra_errors.c 
        
           			     PROPERTIES 
        
           			     GENERATED 
        
           			     ON)

libelektra/src/libs/highlevel/CMakeLists.txt

Line 10 in 5519cb8

${PROJECT_BINARY_DIR}/src/error/elektra_errors.c

ghost · 2019-09-28T08:47:00Z

@kodebach I receive some warnings in the jenkins build which I think causes it to fail:

error: argument to ‘sizeof’ in ‘strncpy’ call is the same expression as the source; did you mean to use the size of the destination? [-Werror=sizeof-pointer-memaccess]

  strncpy (bufferEnd, "/module", sizeof ("/module"));

do I need to change this to
strncpy (bufferEnd, "/module", buffer);?
At least this is what I have seen in similar PRs from other projects

ghost · 2019-10-01T06:37:08Z

@markus2330 did I understand strncpy correctly in the upper comment? I am rarely that unsure about fixing something but since @kodebach is that proficient in C I assume a fix from me on his code is more likely another bug than a fix.

kodebach · 2019-10-01T07:57:15Z

Sorry, i missed this.

do I need to change this to
strncpy (bufferEnd, "/module", buffer);

That won't work, buffer is a pointer not a size.

I always found this GCC warning somewhat annoying. It warns you about perfectly correct code...
The easiest solution is to just use strcpy instead of strncpy. It should be safe in this case. I think you can also replace the sizeof(...)s with integers, i.e. replace sizeof("/module") with 8 etc.

ghost · 2019-10-02T06:22:20Z

Thank you for your help!

i.e. replace sizeof("/module") with 8 etc.

why 8 and not 7? is it for /0?

kodebach · 2019-10-02T10:05:24Z

is it for \0?

Yes

kodebach · 2019-10-21T11:09:38Z

diff --git a/src/plugins/crypto/botan_operations.h b/src/plugins/crypto/botan_operations.h
index e2b66e22a..108f941ea 100644
--- a/src/plugins/crypto/botan_operations.h
+++ b/src/plugins/crypto/botan_operations.h
@@ -13,6 +13,11 @@
 #include <kdb.h>
 #include <kdbtypes.h>
 
+#ifdef __cplusplus
+extern "C" {
+using namespace ckdb;
+#endif
+
 #define ELEKTRA_CRYPTO_BOTAN_KEYSIZE (32)
 #define ELEKTRA_CRYPTO_BOTAN_BLOCKSIZE (16)
 #define ELEKTRA_CRYPTO_BOTAN_ALGORITHM "AES-256/CBC"
@@ -22,4 +27,8 @@ int elektraCryptoBotanInit (Key * errorKey);
 int elektraCryptoBotanEncrypt (KeySet * pluginConfig, Key * k, Key * errorKey, Key * masterKey);
 int elektraCryptoBotanDecrypt (KeySet * pluginConfig, Key * k, Key * errorKey, Key * masterKey);
 
+#ifdef __cplusplus
+}
+#endif
+
 #endif

Applying this diff should fix the namespace problems for Botan.

ghost · 2019-11-06T07:46:20Z

jenkins build libelektra please

ghost · 2019-11-07T09:03:29Z

jenkins build libelektra please

ghost · 2019-11-07T16:31:58Z

@markus2330 please take a look through the PR, all I am left to do is restart the jenkins until it finally gives green light to merge

ghost · 2019-11-07T17:58:53Z

jenkins build libelektra please

mpranj · 2019-11-07T18:38:44Z

The current build has been running for 9h 34m 56s. Can we kill/restart that?

ghost · 2019-11-07T18:40:05Z

The current build has been running for 9h 34m 56s. Can we kill/restart that?

yes please, I tried to restart it already with the restart command here

markus2330

Looks good, a lot of code removed!

markus2330 · 2019-11-08T02:51:50Z

src/include/kdberrors.h

+/**
+ * @file
+ *
+ * @brief


Please write a sentence what this file is for.

markus2330 · 2019-11-08T02:52:14Z

src/include/kdberrors.h

+
+#ifdef __cplusplus
+extern "C" {
+using Key = ckdb::Key;


using should not be used in header files. Did you have any luck with completely removing ckdb?

No, I have tried it but failed due to my insufficient lack of C/C++ skills.

Removing the ckdb namespace should be pretty easy, but it should be a separate PR since it affects many files. I can do it after #3115, if no one else has the time/knowledge.

Maybe @Piankero can try again, 90% of the effort should be to simply remove all occurrences of the text string "ckdb".

yeah sure, I can write that regex remove those occurrences and open a PR for @kodebach to continue?

Yes, it would be nice if you could start the PR. Just using a regex won't work since you cannot correctly remove the closing brackets corresponding to namespace ckdb { declarations. But that shouldn't be too much effort to do manually.

no problem, I will create it in your repository tomorrow morning.
Just to be sure:
I should remove all ckdb:: as well as all namespace ckdb { + } occurrences?

I should remove all ckdb:: as well as all namespace ckdb { + } occurrences?

The safest thing would be too replace ckdb:: with just ::, e.g. replacing ckdb::Key to ::Key. That way we explicitly refer to the global namespace and shouldn't run into any problems, if someone used for example using namespace kdb; and then ckdb::Key.

markus2330 · 2019-11-08T02:53:14Z

src/include/kdberrors.h

+#endif
+
+#ifndef ELEKTRA_MODULE_NAME
+#define ELEKTRA_MODULE_NAME kdb


I would prefer if we set the modules at the appropriate places and not having a catch-all here. Which places do not set the ELEKTRA_MODULE_NAME?

When I implemented the new error codes I was not able to implement this for the core of elektra. Every plugin sets it correctly (guaranteed via the cmake LibAddPlugin). I remember also a longer discussion with @kodebach why this was difficult/ not possible but cannot remember the actual reasons.

If you cannot remember, please remove and find out.

The module name should be set in CMake (as a compiler argument), whenever we want something other than "kdb". AFAIK this is currently done in add_plugin and maybe also add_lib.

Yes, but the question was if we need this default "kdb". Which module does not have any name set? We should fix that to not get the possibly wrong module name "kdb".

I think elektra-core and elektra-kdb don't set a module name. In any case we should either have a default set or replace the #define with an error, otherwise the module name will be set to ELEKTRA_MODULE_NAME since the macro is simply not replaced.

Yes, good objection. Of course it should be an error and not simply removed.

markus2330 · 2019-11-08T02:56:41Z

src/libs/elektra/symbols.map

@@ -132,6 +132,29 @@ libelektra_0.8 {
 	elektraKeySetName;
 };

+libelektra_1.0 {
+	# kdberrors.h
+	ELEKTRA_ERROR_RESOURCE;


These are macros and not symbols? Why are they added here?

After various frustrating attempts I asked @kodebach for help despite I wanted to implement it myself. He added those lines

I actually didn't add those lines I just moved them from the private part below since they should be accessible by applications. However, you are right they are macros we should be able to remove those lines.

markus2330 · 2019-11-08T02:59:50Z

src/plugins/crypto/botan_operations.h

@@ -13,6 +13,12 @@
 #include <kdb.h>
 #include <kdbtypes.h>

+#ifdef __cplusplus
+extern "C" {
+using Key = ckdb::Key;


Also here using should be avoided.

… code

This reverts commit 636b218

ghost · 2019-11-08T08:15:22Z

As the PR already passed the build server, maybe we should merge it and make a follow-up PR?

It had a conflict so I needed to rebase. After the build servers passes please merge it

ghost · 2019-11-08T08:58:18Z

jenkins build libelektra please

markus2330 · 2019-11-08T18:58:24Z

Thank you, I am looking forward to the follow-up.

ghost added the work in progress label Sep 18, 2019

kodebach reviewed Sep 18, 2019

View reviewed changes

dominicjaeger mentioned this pull request Oct 17, 2019

test_service_convertengine fails on website build #3086

Closed

ghost mentioned this pull request Oct 19, 2019

src/libs/highlevel/elektra_error.c #3092

Closed

kodebach mentioned this pull request Oct 21, 2019

Consider removing ckdb namespace #3104

Closed

ghost mentioned this pull request Nov 7, 2019

Error message pitfall remedies. Discussion PR #3176

Closed

markus2330 reviewed Nov 8, 2019

View reviewed changes

Michael Zronek and others added 25 commits November 8, 2019 09:13

error-generation: namespace problem fix for botan

6a518cb

error-gen: moving error generation to own file

dfde3c1

error-gen: removed generation (incomplete as of now)

e72c209

error-gen: removed generation highlevel (incomplete as of now)

4d3195d

error: restructured kdberrors.h file, removed errors.h, fixed cmake

1f7516f

error: cmake reformat

b6a4e11

error: fixed namespaces after refactor

9241408

error: removed old error files

7ced598

error: fixed broken links

069371c

error: reformat markdown

0c1158d

error: refactored errors.c for memaccess error, removed old highlevel…

222ef4d

… code

error-generation: added namespace to dump.cpp to avoid shadowing

4639607

error-generation: Added namespace to botan_operations.cpp

bebc085

error-generation: namespace problem fix for botan

645f2e1

error-generation: namespace problem fix try 123412512

b3aae72

code-generation: moved namespace to top of botan file

bc5e761

error-generation: fixed warning meta keys

de64c2c

removed warning array code as irrelevant in this PR

a9924bf

removed unused method

e3a7e01

Revert "error-generation: fixed warning meta keys"

148dca4

This reverts commit 636b218

Formatting

a3f56c0

errors: fix symbol versioning for error functions

96e7c0e

errors: fix using namespace ckdb

f5d8348

crypto: fix botan

fc52a83

error-generation: Added explanation on top of file

5f5069b

markus2330 merged commit 4b5c0d5 into ElektraInitiative:master Nov 8, 2019

ghost deleted the remove-error-code-generation branch December 4, 2019 09:12

Remove error code generation #2976

Remove error code generation #2976

Conversation

ghost commented Sep 18, 2019

Basics

Checklist

Review

Labels

ghost commented Sep 18, 2019

kodebach commented Sep 18, 2019

kodebach commented Sep 18, 2019

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 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kodebach commented Sep 18, 2019

ghost commented Sep 19, 2019

kodebach commented Sep 19, 2019

ghost commented Sep 20, 2019

kodebach commented Sep 26, 2019

ghost commented Sep 28, 2019 • edited by ghost Loading

ghost commented Oct 1, 2019

kodebach commented Oct 1, 2019

ghost commented Oct 2, 2019

kodebach commented Oct 2, 2019

kodebach commented Oct 21, 2019

ghost commented Nov 6, 2019

ghost commented Nov 7, 2019

ghost commented Nov 7, 2019

ghost commented Nov 7, 2019

mpranj commented Nov 7, 2019

ghost commented Nov 7, 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

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

ghost Nov 8, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Nov 8, 2019

ghost commented Nov 8, 2019

markus2330 commented Nov 8, 2019

ghost commented Sep 28, 2019 •

edited by ghost

Loading

ghost Nov 8, 2019 •

edited by ghost

Loading