Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rename all plugins's .sos #3486

Closed
markus2330 opened this issue Sep 12, 2020 · 8 comments · Fixed by #4919
Closed

rename all plugins's .sos #3486

markus2330 opened this issue Sep 12, 2020 · 8 comments · Fixed by #4919
Assignees
Projects
Milestone

Comments

@markus2330
Copy link
Contributor

markus2330 commented Sep 12, 2020

As @poldi871reported in #3472 on embedded systems (with the cmake option TARGET_PLUGIN_FOLDER="") kdb plugin-list reports both libraries and plugins. Also by looking at the names of the .sos you cannot know what a plugin is.

An easy fix for both problems would be to rename the .sos to e.g. libelektra-plugin-toml.so instead of libelektra-toml.so. This is a bit redundancy in the case TARGET_PLUGIN_FOLDER!="" but helpful in many other situations (e.g. when looking into the lib folder of the cmake build dir), e.g. on my system (where kdb get system/info/elektra/constants/cmake/BUILTIN_PLUGIN_FOLDER returns /usr/lib/x86_64-linux-gnu/elektra4 the plugins would be called e.g. /usr/lib/x86_64-linux-gnu/elektra4/libelektra-plugin-toml.so.

So I propose to do this rename. Obviously it is an incompatible change: Elektra needs to be fully reinstalled as the new core would only find newly installed plugins (and not the ones with the old name). Furthermore, I propose (maybe even the bigger change in terms of work) that we drop the name "modules". Instead, only the name "plugin" should be used for plugins, even on loader level. E.g. elektraModulesLoad would be renamed to elektraPluginLoad. I also think that these 3 functions for plugin loading should be public, as they are handy in many situations (e.g. for import/export configuration).

What do you think?

@mpranj
Copy link
Member

mpranj commented Sep 13, 2020

I agree with this proposal, and there is no better time to do this than pre-1.0. 👍

@markus2330 markus2330 self-assigned this Sep 18, 2020
@markus2330 markus2330 removed their assignment Sep 22, 2021
@markus2330 markus2330 added this to the 0.9.* milestone Sep 22, 2021
@markus2330
Copy link
Contributor Author

@kodebach any thoughts on this?

@kodebach
Copy link
Member

Yes, adding a -plugin- part to plugin SOs is a very good idea. It would also avoid weird cases like having to call the plugin gopts, because opts would conflict with the library.

Furthermore, I propose (maybe even the bigger change in terms of work) that we drop the name "modules". Instead, only the name "plugin" should be used for plugins, even on loader level.

Since elektraModulesLoad already returns a elektraPluginFactory that is a sensible change.

Additionally, I would also change elektraPluginFactory from

typedef Plugin * (*elektraPluginFactory) (void);

to

typedef KeySet * (*elektraPluginFactory) (void);

where the returned KeySet * is the contract of the Plugin to avoid the hacky overload of the get function.
I think that is already mentioned in another issue. This also requires changing elektraPluginExport and elektraPluginOpen, but should be doable without any other changes.

@markus2330
Copy link
Contributor Author

Thank you for your input!

I have split the issues: this issue is only about renaming: adding the -plugin- part, for the rest there is #4490.

@markus2330 markus2330 added this to To Do in 1.0 Apr 5, 2023
@markus2330
Copy link
Contributor Author

@tmakar last issue for you: spec plugin should be renamed to libelektra-plugin-spec.so (and likewise for all other plugins)

@kodebach
Copy link
Member

kodebach commented May 2, 2023

@tmakar One way this issue can definitely be solved is by adding the -plugin- in this line:

set (PLUGIN_NAME elektra-${PLUGIN_SHORT_NAME})

IMO this would be the best option, but it will probably require more changes to CMake elsewhere, since this changes the name of the CMake targets for plugins.

CMake also supports setting an OUTPUT_NAME for each target independent of the name of a target. That is probably quicker to implement. I haven't looked into details, but I suspect this would only require adding one (or a few) line below this one

add_library (${PLUGIN_NAME} MODULE ${ARG_SOURCES} ${ARG_OBJECT_SOURCES})

to set the OUTPUT_NAME for the ${PLUGIN_NAME} target.

@tmakar
Copy link
Contributor

tmakar commented May 2, 2023

@tmakar One way this issue can definitely be solved is by adding the -plugin- in this line:

set (PLUGIN_NAME elektra-${PLUGIN_SHORT_NAME})

IMO this would be the best option, but it will probably require more changes to CMake elsewhere, since this changes the name of the CMake targets for plugins.

CMake also supports setting an OUTPUT_NAME for each target independent of the name of a target. That is probably quicker to implement. I haven't looked into details, but I suspect this would only require adding one (or a few) line below this one

add_library (${PLUGIN_NAME} MODULE ${ARG_SOURCES} ${ARG_OBJECT_SOURCES})

to set the OUTPUT_NAME for the ${PLUGIN_NAME} target.

Thanks for the details!!

tmakar added a commit to tmakar/libelektra that referenced this issue May 3, 2023
@tmakar tmakar linked a pull request May 3, 2023 that will close this issue
19 tasks
tmakar added a commit to tmakar/libelektra that referenced this issue May 3, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 3, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 3, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 3, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 3, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 3, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 3, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 3, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 3, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 3, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 3, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 3, 2023
@tmakar tmakar moved this from To Do to In Progress in 1.0 May 8, 2023
@tmakar
Copy link
Contributor

tmakar commented May 8, 2023

@markus2330 PR exists in #4919.

tmakar added a commit to tmakar/libelektra that referenced this issue May 9, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 9, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 9, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 9, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 9, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 9, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 9, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 9, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 9, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 9, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 9, 2023
tmakar added a commit to tmakar/libelektra that referenced this issue May 9, 2023
1.0 automation moved this from In Progress to Done May 11, 2023
atmaxinger added a commit that referenced this issue May 11, 2023
flo91 pushed a commit to flo91/libelektra that referenced this issue Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
1.0
  
Done
Development

Successfully merging a pull request may close this issue.

4 participants