-
Notifications
You must be signed in to change notification settings - Fork 81
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
Pass exclude-libraries as env var to linuxdeploy plugins #283
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
08f1d1c
to
274d684
Compare
@TheAssassin ping? |
@TheAssassin friendly ping 😄 |
Thanks for the contribution and sorry for the delay. |
274d684
to
33fd6d3
Compare
Excluding libraries doesn't properly work as of now because plugins are not notified of such exclusions, by passing the env var and automatically reading it plugins just need to be recompiled.
7295404
to
67115ea
Compare
@@ -656,7 +657,8 @@ namespace linuxdeploy { | |||
AppDir::AppDir(const std::string& path) : AppDir(fs::path(path)) {} | |||
|
|||
void AppDir::setExcludeLibraryPatterns(const std::vector<std::string> &excludeLibraryPatterns) { | |||
d->excludeLibraryPatterns = excludeLibraryPatterns; | |||
d->excludeLibraryPatterns.insert(d->excludeLibraryPatterns.end(), excludeLibraryPatterns.begin(), excludeLibraryPatterns.end()); | |||
util::misc::stringVectorToEnv("LINUXDEPLOY_EXCLUDED_LIBRARIES", ';', d->excludeLibraryPatterns); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line does not make sense. We should not influence the environment from a non-singleton class. As a user, you'd set the environment variable anyway, thus causing linuxdeploy-plugin-qt and subsequent runs of linuxdeploy from the script to pick up the environment variable. This is in line with all the other environment variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the whole reason of this patch.
user call ./ld --exclude-libraries foo --plugin qt
if linux deploy does not set the ENV var the qt plugin won't skipp foo library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I makes no sense for
main ./linux-deploy --exclude-libraries foo works but for it's plugin the user has to manually set the env var.
Also there's no drawback from having duplicate items on the list, else we would use std::set and still doesn't mater for performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comment.
Excluding libraries doesn't properly work as of now because plugins are not notified of such exclusions, by passing the env var and automatically reading it plugins just need to be recompiled.
This fixes linuxdeploy/linuxdeploy-plugin-qt#153