Skip to content

Add --exclude-library option.#200

Merged
TheAssassin merged 1 commit into
linuxdeploy:masterfrom
solemnwarning:exclude-library
May 3, 2022
Merged

Add --exclude-library option.#200
TheAssassin merged 1 commit into
linuxdeploy:masterfrom
solemnwarning:exclude-library

Conversation

@solemnwarning
Copy link
Copy Markdown
Contributor

@solemnwarning solemnwarning commented May 1, 2022

Add --exclude-library path to allow excluding libraries in addition to the ones defined by the upstream excludelist file.

Closes #29.

Comment thread include/linuxdeploy/core/appdir.h Outdated
Comment thread src/core/appdir.cpp

static auto isInExcludelist = [](const bf::path& fileName) {
for (const auto& excludePattern : generatedExcludelist) {
static auto isInExcludelist = [](const bf::path& fileName, const std::vector<std::string> &excludeList) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just capture this, then you can access member variables.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Its passing in the exclude list because isInExcludelist is used to check both lists (command line and built-in), or is there a different variable that could be captured I'm not seeing?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see. I thought it might be easier to move the logic entirely into the lambda. But this seems okay, too.

Comment thread src/core/appdir.cpp Outdated
Comment thread src/main.cpp Outdated
args::ValueFlag<std::string> appDirPath(parser, "appdir", "Path to target AppDir", {"appdir"});

args::ValueFlagList<std::string> sharedLibraryPaths(parser, "library", "Shared library to deploy", {'l', "library"});
args::ValueFlagList<std::string> excludeLibraryPaths(parser, "library", "Shared library to exclude from deployment", {"exclude-library"});
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

They're patterns, not paths, not filenames. This should be reflected in the help text.

Suggested change
args::ValueFlagList<std::string> excludeLibraryPaths(parser, "library", "Shared library to exclude from deployment", {"exclude-library"});
args::ValueFlagList<std::string> excludeLibraryPatterns(parser, "pattern", "fnmatch pattern used to exclude shared libraries from deployment", {"custom-exclude-library-pattern"});

Copy link
Copy Markdown
Member

@TheAssassin TheAssassin left a comment

Choose a reason for hiding this comment

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

Thank you very much. Looks promising. I left some comments.

@solemnwarning
Copy link
Copy Markdown
Contributor Author

@TheAssassin not sure if you'd get notified or not, but I updated the PR.

@TheAssassin
Copy link
Copy Markdown
Member

I saw that, but there's still one open change request: https://github.com/linuxdeploy/linuxdeploy/pull/200/files#r862491555

Copy link
Copy Markdown
Member

@TheAssassin TheAssassin left a comment

Choose a reason for hiding this comment

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

Looks good. Haven't got to testing this yet, but I don't anticipate issues.

Comment thread src/core/appdir.cpp

static auto isInExcludelist = [](const bf::path& fileName) {
for (const auto& excludePattern : generatedExcludelist) {
static auto isInExcludelist = [](const bf::path& fileName, const std::vector<std::string> &excludeList) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see. I thought it might be easier to move the logic entirely into the lambda. But this seems okay, too.

@TheAssassin TheAssassin merged commit 097212a into linuxdeploy:master May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parameter to add exclude patterns

2 participants