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.

Sign up for GitHub

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

Jump to bottom

Add OC_EXTRA_PLUGIN_PATH cmake and define #7076

Merged
merged 1 commit into from
Mar 14, 2019
Merged

Add OC_EXTRA_PLUGIN_PATH cmake and define #7076

merged 1 commit into from
Mar 14, 2019

Conversation

ckamm
Copy link
Contributor

@ckamm ckamm commented Mar 7, 2019

By default, plugins are only searched next to the binary or next to the
other Qt plugins. This optional build variable allows another path to be
configured.

The idea is that on linux the oC packaging probably wants the binary in
something like /opt/owncloud/bin and the plugins in
/opt/owncloud/lib/plugins.

Similarly, distribution packagers probably don't want the plugins next
to the binary or next to the other Qt plugins. This flag allows them to
configure another path that the executable will look in.

For #7027

@ckamm ckamm added this to the 2.6.0 milestone Mar 7, 2019
@ckamm ckamm self-assigned this Mar 7, 2019
@ckamm ckamm requested a review from jnweiger March 7, 2019 10:04
ogoffart
ogoffart approved these changes Mar 7, 2019
View reviewed changes
Copy link
Contributor

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the path should be relative to the binary? or at least allow it.

src/gui/application.cpp Outdated
@@ -254,6 +254,15 @@ Application::Application(int &argc, char **argv)
AbstractNetworkJob::httpTimeout = cfg.timeout();

// Check vfs plugins
#define MACRO_TO_STRING2(x) #x
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe ifdef OC_EXTRA_PLUGIN_PATH, and only add the -D if it is set

dschmidt
dschmidt requested changes Mar 7, 2019
View reviewed changes
CMakeLists.txt Outdated
@@ -102,6 +102,9 @@ if(NO_MSG_HANDLER)
add_definitions(-DNO_MSG_HANDLER=1)
endif()

set(OC_EXTRA_PLUGIN_PATH "" CACHE STRING "Extra path to look for Qt plugins")
add_definitions(-DOC_EXTRA_PLUGIN_PATH=${OC_EXTRA_PLUGIN_PATH})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't do it like this. If you absolutely want to use -D instead of putting it in a config.h file, then only apply it to the relevant target not to all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably also define a sane default that just works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make these changes. What default value would you suggest? /usr/lib/owncloud-client? I imagine it might be different per distro.

@ckamm
Copy link
Contributor Author

ckamm commented Mar 11, 2019

@ogoffart I think for most linux packaging we'd want an absolute path. But I'll also allow relative ones, could be useful for osx bundles.

@ckamm ckamm force-pushed the 7027-extra-plugin-search-path branch from 89359c9 to f837915 Compare March 11, 2019 09:06
@ckamm
Copy link
Contributor Author

ckamm commented Mar 11, 2019

updated

jnweiger
jnweiger reviewed Mar 11, 2019
View reviewed changes
Copy link
Contributor

@jnweiger jnweiger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could that be relative to our LIBDIR, so if LIBDIR is /opt/ownCloud/ownCloud/lib/x86_64-linux-gnu/
it would become /opt/ownCloud/ownCloud/lib/x86_64-linux-gnu/plugins ?

@ckamm
Copy link
Contributor Author

ckamm commented Mar 12, 2019 •
edited
Loading

@jnweiger I have set the default to ${CMAKE_INSTALL_LIBDIR}/plugins, which matches the current plugin install location.

@ckamm ckamm force-pushed the 7027-extra-plugin-search-path branch from f837915 to 97bc564 Compare March 12, 2019 08:16
@ckamm ckamm requested review from dschmidt and jnweiger March 12, 2019 08:25
@dschmidt
Copy link
Member

Isn't applicationDirPath() == ${PREFIX}/bin? So the lookup would be in ${PREFIX}/bin/lib/plugins?

If it's indeed correct I think this is ok. A different possibly simpler approach would be to use https://cmake.org/cmake/help/v3.7/module/GNUInstallDirs.html#command:gnuinstalldirs_get_absolute_install_dir

basically something around these lines:

set(PLUGINDIR "${CMAKE_INSTALL_LIBDIR}/plugins")
gnuinstalldirs_get_absolute_install_dir(FULL_PLUGINDIR PLUGINDIR)

And then add FULL_PLUGINDIR to the config.h.in. This should also handle both relative and absolute paths in and outside of the actual install prefix.

@ckamm
Copy link
Contributor Author

ckamm commented Mar 12, 2019

@dschmidt Yes, the relative path support is relative to the current location of the executable file, meaning the current default isn't useful since CMAKE_INSTALL_LIBDIR is a relative path. I'll fix by using CMAKE_INSTALL_FULL_LIBDIR for the default.

@ckamm
Copy link
Contributor Author

ckamm commented Mar 12, 2019

@dschmidt Should we change this path and the default plugin install path to LIBDIR/owncloud-client/plugins? Then the default config would install and search them in something like /usr/local/lib/x86_64-linux-gnu/owncloud-client/plugins which seems nicer than without the extra owncloud-client/ subfolder.

@ckamm ckamm force-pushed the 7027-extra-plugin-search-path branch 2 times, most recently from 0d4ff5d to 150d2aa Compare March 13, 2019 09:44
@ckamm
Add PLUGINDIR cmake setting and define #7027
29e57cf 
By default, plugins are only searched next to the binary or next to the
other Qt plugins. This optional build variable allows another path to be
configured.

The idea is that on linux the oC packaging probably wants the binary in
something like /opt/owncloud/bin and the plugins in
/opt/owncloud/lib/plugins.

Similarly, distribution packagers probably don't want the plugins next
to the binary or next to the other Qt plugins. This flag allows them to
configure another path that the executable will look in.
@ckamm ckamm force-pushed the 7027-extra-plugin-search-path branch from 150d2aa to 29e57cf Compare March 13, 2019 13:38
@ckamm ckamm merged commit 7d34d3b into master Mar 14, 2019
@delete-merged-branch delete-merged-branch bot deleted the 7027-extra-plugin-search-path branch March 14, 2019 07:11
@ckamm ckamm mentioned this pull request Mar 15, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogoffart ogoffart ogoffart approved these changes

@dschmidt dschmidt dschmidt approved these changes

@jnweiger jnweiger Awaiting requested review from jnweiger

Assignees

@ckamm ckamm

Labels
None yet
Projects
None yet
Milestone
2.6.0
Development

Successfully merging this pull request may close these issues.
4 participants
@ckamm @dschmidt @ogoffart @jnweiger