Allow to deploy specific platform plugins#89
Allow to deploy specific platform plugins#89Djolivald wants to merge 1 commit intolinuxdeploy:masterfrom
Conversation
|
I think there may be an issue with the runner the i386 build action runs on, it errors out saying |
|
That's most likely the issue, yeah. I'll fix that on master, then we'll rebase your branch on it. |
|
That first issue was gone after a rebuild. I fixed another one, though. Please rebase. |
TheAssassin
left a comment
There was a problem hiding this comment.
Looks quite promising overall, but I have a few remarks and change requests.
| **Qt specific:** | ||
| - `$QMAKE=/path/to/my/qmake`: use another `qmake` binary to detect paths of plugins and other resources (usually doesn't need to be set manually, most Qt environments ship scripts changing `$PATH`) | ||
| - `$EXTRA_QT_PLUGINS=pluginA;pluginB`: Plugins to deploy even if not found automatically by linuxdeploy-plugin-qt | ||
| - `$PLATFORM_PLUGIN=pltformA.so;platformB.so`: Platforms to deploy, defaults to `libqxcb.so`. Platform must be available from `QT_INSTALL_PLUGINS/platforms`. |
There was a problem hiding this comment.
The name should be plural, I suppose that's a typo.
I'd also rather not have people manually add libqxcb but deploy it by default. Therefore, the name should be $EXTRA_PLATFORM_PLUGINS.
| std::move(qtTranslationsPath), | ||
| std::move(qtDataPath)) { | ||
| // check if the platform plugins are set in env | ||
| const auto* const platformPluginsFromEnvData = getenv("PLATFORM_PLUGINS"); |
There was a problem hiding this comment.
As said above, I'd rather enforce the deployment of libqxcb.so. Instead of PLATFORM_PLUGINS, I would like to have some EXTRA_PLATFORM_PLUGINS.
| std::move(installLibsPath), | ||
| std::move(qtTranslationsPath), | ||
| std::move(qtDataPath)) { | ||
| // check if the platform plugins are set in env |
There was a problem hiding this comment.
Not sure whether it's a great idea to implement a custom constructor. Of course, software design wise, that's probably the way to go, given that we need to create that list below. However, it adds a fair amount of boilerplate code that will have to be maintained. What do you think about moving the evaluation of the environment variable into deploy()? I am undecided which version would be better in the long term.
There was a problem hiding this comment.
It doesn't seem like platformToDeploy is used at all outside of the deploy function, and that function is only called once per instance, so I think it could just be a local variable.
|
I think this PR should be closed due #118 |
Resolves #88, Add the feature to allow to chose platform plugins from environment variable.