CMake: provide plugin name as compiler definition #2360

petermax2 · 2019-02-01T15:25:35Z

Provides ELEKTRA_PLUGIN_NAME and ELEKTRA_PLUGIN_NAME_C as compiler definitions via CMake.

EDIT: This PR does NOT close the issue #1042 .

Basics

Do not describe the purpose here 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
I ran all tests locally and everything went fine
affected documentation is fixed
I added code comments, logging, and assertions (see doc/CODING.md)
meta data is updated (e.g. README.md of plugins)

Review

Remove the line below and add the "work in progress" label if you do
not want the PR to be reviewed:

@markus2330 please review my pull request

markus2330 · 2019-02-01T15:32:08Z

Wow, great to see this. I am looking forward to review it soon.

petermax2 · 2019-02-01T19:55:47Z

Do you want to get rid of CRYPTO_PLUGIN_FUNCTION and simplify ELEKTRA_PLUGIN_FUNCTION from

ELEKTRA_PLUGIN_FUNCTION(module, function) ELEKTRA_PLUGIN_FUNCTION2 (module, ELEKTRA_VARIANT, function)

to

ELEKTRA_PLUGIN_FUNCTION(function) ELEKTRA_PLUGIN_FUNCTION2 (ELEKTRA_PLUGIN_NAME_C, ELEKTRA_VARIANT, function)

?

It should be possible with the changes in this PR.

Do you prefer a small PR or should I just continue?

markus2330 · 2019-02-02T07:40:00Z

Yes ELEKTRA_PLUGIN_FUNCTION(function) would be great!

Do you prefer a small PR or should I just continue?

Depends on your time budget. It would be good if the PR does not get stuck in the middle.

petermax2 · 2019-02-02T08:45:14Z

OK let's mrege it then, to be sure. I'll use #1042 as follow-up task.

sanssecours

Looks good as far as I can tell. Thank you for the update.

sanssecours · 2019-02-02T10:47:35Z

cmake/Modules/LibAddPlugin.cmake

+# Provides additional compiler definitions:
+#
+# - ELEKTRA_PLUGIN_NAME containing the short plugin name as string
+# - ELEKTRA_PLUGIN_NAME_C contai the short plugin name which can be used in function names


Spelling: contai → containing

sanssecours · 2019-02-02T10:55:42Z

cmake/Modules/LibAddPlugin.cmake

+
+	# provide the plugin name as macro that can be used for building function names, etc.
+	if (NOT "${ARG_COMPILE_DEFINITIONS}" MATCHES "ELEKTRA_PLUGIN_NAME_C")
+		if (ADDITIONAL_COMPILE_DEFINITIONS_PART1)


I think you can replace this if-statement with the following simpler code:

list (APPEND ADDITIONAL_COMPILE_DEFINITIONS_PART2 "ELEKTRA_PLUGIN_NAME_C=${shortname}")

.

sanssecours · 2019-02-02T10:58:29Z

doc/news/_preparation_next_release.md

@@ -256,7 +256,9 @@ you up to date with the multi-language support provided by Elektra.
  anymore, if it is unable to locate a Lua executable. *(René Schwaiger)*
 - We added code that makes sure you can compile [IO GLIB](https://www.libelektra.org/bindings/io_glib) on macOS, even if `pkg-config`
  erroneously reports that GLIB requires linking to the library `intl` (part of [GNU gettext](https://www.gnu.org/software/gettext)).
-  *(René Schwaiger)*
+  (René Schwaiger)


I think this line is already correct, no need to change it back again 😊.

sometimes rebase messes with my brain 😆

sanssecours · 2019-02-02T11:00:29Z

doc/news/_preparation_next_release.md

@@ -256,7 +256,9 @@ you up to date with the multi-language support provided by Elektra.
  anymore, if it is unable to locate a Lua executable. *(René Schwaiger)*
 - We added code that makes sure you can compile [IO GLIB](https://www.libelektra.org/bindings/io_glib) on macOS, even if `pkg-config`
  erroneously reports that GLIB requires linking to the library `intl` (part of [GNU gettext](https://www.gnu.org/software/gettext)).
-  *(René Schwaiger)*
+  (René Schwaiger)
+- The plugin name is provided as compiler definition via CMake. See #1042 .*(Peter Nirschl)*


It would be nice, if you could also provide a link to issue #1042 here.

And a link to the docu or ideally also say which compiler definitions were added.

markus2330

Thank you, great job!

markus2330 · 2019-02-02T11:14:17Z

cmake/Modules/LibAddPlugin.cmake

@@ -1,6 +1,41 @@
 include (LibAddMacros)
 include (LibAddTest)

+# ~~~
+# Provides additional compiler definitions:


It would be good to have this docu also in the API docu of plugins or even in the plugin writing tutorial. But this can also be done in a later PR.

markus2330 · 2019-02-02T11:16:03Z

doc/news/_preparation_next_release.md

@@ -256,7 +256,9 @@ you up to date with the multi-language support provided by Elektra.
  anymore, if it is unable to locate a Lua executable. *(René Schwaiger)*
 - We added code that makes sure you can compile [IO GLIB](https://www.libelektra.org/bindings/io_glib) on macOS, even if `pkg-config`
  erroneously reports that GLIB requires linking to the library `intl` (part of [GNU gettext](https://www.gnu.org/software/gettext)).
-  *(René Schwaiger)*
+  (René Schwaiger)
+- The plugin name is provided as compiler definition via CMake. See #1042 .*(Peter Nirschl)*


And a link to the docu or ideally also say which compiler definitions were added.

markus2330 · 2019-02-02T11:18:37Z

src/plugins/base64/CMakeLists.txt

@@ -5,8 +5,6 @@ add_plugin (base64
 		    base64_functions.c
 		    base64.h
 		    base64.c
-	    COMPILE_DEFINITIONS ELEKTRA_PLUGIN_NAME=\"base64\"


Great to see this simplification!

Similar changes can be done for gpgme, resolver, mmap* (@mpranj)

markus2330 · 2019-02-02T11:20:48Z

Maybe you can also rebase, the Jenkins build failure in master should hopefully be resolved by now #2351

petermax2 · 2019-02-02T18:37:57Z

Thank you both for the great feedback!

Provides ELEKTRA_PLUGIN_NAME and ELEKTRA_PLUGIN_NAME_C as compiler definitions via CMake. See ElektraInitiative#1042 for details.

Simplify code by using ADD_TEST

@sanssecours

Thanks to the feedback by @sanssecours ! See ElektraInitiative#1042 .

petermax2 · 2019-02-03T15:16:29Z

Travis seems to be stuck.

sanssecours · 2019-02-03T16:07:39Z

Travis seems to be stuck.

It is not stuck, but the Mac build jobs are just super slow 🐌. I currently work on transferring most of the Mac build jobs to Cirrus CI, which uses VMs that are much faster (about 5 times faster, as far as I can tell).

markus2330 · 2019-02-04T18:16:15Z

Thank you for this great work!

petermax2 added the work in progress label Feb 1, 2019

petermax2 force-pushed the 1042_elektra_plugin_name branch from 6edf868 to 2ca5555 Compare February 1, 2019 15:41

petermax2 force-pushed the 1042_elektra_plugin_name branch from 559abd4 to ca9fc6b Compare February 2, 2019 08:49

petermax2 added ready to merge and removed work in progress labels Feb 2, 2019

petermax2 requested review from markus2330 and sanssecours February 2, 2019 08:49

petermax2 mentioned this pull request Feb 2, 2019

ELEKTRA_PLUGIN_NAME #1042

Closed

sanssecours approved these changes Feb 2, 2019

View reviewed changes

markus2330 approved these changes Feb 2, 2019

View reviewed changes

markus2330 removed the ready to merge label Feb 2, 2019

mpranj mentioned this pull request Feb 2, 2019

Global Cache #2270

Closed

12 tasks

petermax2 added 10 commits February 2, 2019 19:42

CMake: provide plugin name as compiler definition

5367ff9

Provides ELEKTRA_PLUGIN_NAME and ELEKTRA_PLUGIN_NAME_C as compiler definitions via CMake. See ElektraInitiative#1042 for details.

CMake: remove debugging code from LibAddPlugin.cmake

f2e6c83

CMake: add release notes for ElektraInitiative#1042

d89340a

CMake: reformat cmake code

6bbff6c

CMake: fix release notes for ElektraInitiative#1042

f6283a7

CMake: fix in-code doc of set_additional_compile_definitions

6909cf1

gpgme: improve CMakeLists.txt

c52eafd

Simplify code by using ADD_TEST

CMake: simplify code in set_additional_compile_definitions

eaeb4b7

Thanks to the feedback by @sanssecours ! See ElektraInitiative#1042 .

gpgme: further simplify CMakeLists.txt

231f7ee

CMake: reformat

d6b34e8

petermax2 force-pushed the 1042_elektra_plugin_name branch from 8f803b7 to d6b34e8 Compare February 2, 2019 18:43

petermax2 added the ready to merge label Feb 2, 2019

Merge branch 'master' into 1042_elektra_plugin_name

dba4a07

markus2330 merged commit 182d3a9 into ElektraInitiative:master Feb 4, 2019

petermax2 deleted the 1042_elektra_plugin_name branch February 4, 2019 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake: provide plugin name as compiler definition #2360

CMake: provide plugin name as compiler definition #2360

petermax2 commented Feb 1, 2019 •

edited

markus2330 commented Feb 1, 2019

petermax2 commented Feb 1, 2019 •

edited

markus2330 commented Feb 2, 2019

petermax2 commented Feb 2, 2019

sanssecours left a comment

sanssecours Feb 2, 2019

sanssecours Feb 2, 2019

sanssecours Feb 2, 2019

petermax2 Feb 2, 2019

sanssecours Feb 2, 2019

markus2330 Feb 2, 2019

markus2330 left a comment

markus2330 Feb 2, 2019

markus2330 Feb 2, 2019

markus2330 Feb 2, 2019

markus2330 commented Feb 2, 2019

petermax2 commented Feb 2, 2019

petermax2 commented Feb 3, 2019

sanssecours commented Feb 3, 2019

markus2330 commented Feb 4, 2019

CMake: provide plugin name as compiler definition #2360

CMake: provide plugin name as compiler definition #2360

Conversation

petermax2 commented Feb 1, 2019 • edited

Basics

Checklist

Review

markus2330 commented Feb 1, 2019

petermax2 commented Feb 1, 2019 • edited

markus2330 commented Feb 2, 2019

petermax2 commented Feb 2, 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

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

markus2330 commented Feb 2, 2019

petermax2 commented Feb 2, 2019

petermax2 commented Feb 3, 2019

sanssecours commented Feb 3, 2019

markus2330 commented Feb 4, 2019

petermax2 commented Feb 1, 2019 •

edited

petermax2 commented Feb 1, 2019 •

edited