Pass exclude-libraries as env var to linuxdeploy plugins #283
Conversation
08f1d1c to
274d684
Compare
|
@TheAssassin ping? |
|
@TheAssassin friendly ping 😄 |
|
Thanks for the contribution and sorry for the delay. |
7295404 to
67115ea
Compare
src/core/appdir.cpp
Outdated
| @@ -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.
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.
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.
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.
We do this for all other environment variables, though. If you want the values to apply to all the plugins, you use the env var, otherwise, you use the parameter. I do not see a reason to change this.
There was a problem hiding this comment.
Hey @dantti, I also ran into this (but with the gtk plugin while working on a PR for Tauri), and from my reading, I believe @TheAssassin (correct me if I'm wrong) just wants you to remove line 661. I think it's just a consistency thing. The CLI params only apply to linuxdeploy. Env vars apply to linuxdeploy + plugins. Propagating the CLI params here is just inconsistent with how linuxdeploy normally behaves. They don't want to propagate cli parameters and want to configure plugins via environment variables, per the spec: https://github.com/linuxdeploy/linuxdeploy/wiki/Plugin-system#controlling-plugin-behavior
plus there is no ENV var that plugins read to exclude-libraries.
This PR to add $LINUXDEPLOY_EXCLUDED_LIBRARIES to linuxdeploy will be sufficient to add it to all plugins. The bash script based plugins will automatically work since they just bundle extra files and recall the original linuxdeploy package. linuxdeploy-plugin-qt will need to have it's linuxdeploy submodule bumped after this lands, but that's it.
There was a problem hiding this comment.
@DistractionRectangle I could change to remove the cli to ENV passing, but that's just bad UX, I call ./ld --exclude=foo and I expect that foo is going to be excluded, but it's not because plugins will still pick it, so instead I need to LD_EXCLUDE=foo ./ld and now I have it the way I wanted before, this to me is just bad UX. And users will still fill bug reports because --exclude didn't exclude...
I'd then say let's just remove all CLI options, otherwise you need to know how the magic works.
There was a problem hiding this comment.
This just boils down to "why would you ever want to exclude a library from linuxdeploy and not from it's plugins?", if the user is giving this CLI flag it's explicit that it should be excluded, if that's bad then it's the user's QA problem.
There was a problem hiding this comment.
OTOH I'd suggest that all CLI options should be passed to plugins, or worst case have a --exclude-from-(all/plugins...) so that's discoverable...
There was a problem hiding this comment.
I agree with you about the UX, but that's probably a separate issue/PR. It seems to be the blocker for getting this merged, and getting this merged certainly improves the UX vs the workarounds we have to do now.
Passing all cli options to plugins requires all plugins support them, or silently ignore invalid options, which hurts the UX and causes more overhead for the maintainer. It's also a 180 on the maintainers current stance, which is IMO a big ask and out of scope.
|
@TheAssassin how else do you want to have this fixed then? I want to start worrying about wayland+Qt now but this feature is sort of basic to ignore stuff I don't want to package |
b4ff9f1 to
b4c874c
Compare
|
@TheAssassin is it mergeable now? |
|
Hi @dantti, I will have a look in a few minutes, thanks. |
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 or read it.
b4c874c to
882153f
Compare
|
Thanks! And sorry for the delay! |
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