-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
gh-117829: add --include and --exclude flag to zipapp #120537
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
base: main
Are you sure you want to change the base?
Changes from all commits
9dd3619
b887165
23785b2
3286657
c28771f
935a1a5
60a9aef
ee2b185
aaed611
1abe95c
ac29c0e
6d0ca9a
408dd71
fdcd64f
da75715
c4b7cd1
ab215b7
a85989c
e2e6b35
62dc347
bff0a07
a2bc0bf
36ac9b0
0f077eb
d5ffee3
59e2089
2e17bbf
9ec1cf5
28c425d
3265a5a
c67b6b1
617cf18
e08e765
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,8 @@ | |
| import sys | ||
| import zipfile | ||
|
|
||
| from collections.abc import Iterable, Callable | ||
|
|
||
| __all__ = ['ZipAppError', 'create_archive', 'get_interpreter'] | ||
|
|
||
|
|
||
|
|
@@ -177,6 +179,46 @@ def get_interpreter(archive): | |
| if f.read(2) == b'#!': | ||
| return f.readline().strip().decode(shebang_encoding) | ||
|
|
||
| def _make_glob_filter( | ||
| includes: Iterable[str] | None, | ||
| excludes: Iterable[str] | None, | ||
| ) -> Callable[[pathlib.Path], bool]: | ||
| """ | ||
| Build a filter(relative_path: Path) -> bool applying includes first, then excludes. | ||
|
|
||
| Semantics: | ||
| - Patterns are standard glob patterns as implemented by PurePath.match. | ||
| - If 'includes' is empty, all files/dirs are initially eligible. | ||
| - If any exclude pattern matches, the path is rejected. | ||
| """ | ||
|
|
||
| def _normalize_patterns(values: Iterable[str] | None) -> list[str]: | ||
| """ | ||
| Return patterns exactly as provided by the CLI (no comma splitting). | ||
| Each item is stripped of surrounding whitespace; empty items are dropped. | ||
| """ | ||
| if not values: | ||
| return [] | ||
| out: list[str] = [] | ||
| for v in values: | ||
| v = v.strip() | ||
| if v: | ||
| out.append(v) | ||
| return out | ||
|
|
||
| inc = _normalize_patterns(values=includes) | ||
| exc = _normalize_patterns(values=excludes) | ||
|
|
||
| def _filter(rel: pathlib.Path) -> bool: | ||
| # If includes were provided, at least one must match. | ||
| if inc and not any(rel.match(pat) for pat in inc): | ||
| return False | ||
| # Any exclude match removes the path. | ||
| if exc and any(rel.match(pat) for pat in exc): | ||
| return False | ||
| return True | ||
|
|
||
| return _filter | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This all feels way too complicated. We should just use the
The documentation can simply state that patterns are standard glob patterns, as implemented by In particular, matches should not assume Posix path separators or case sensitivity. The pathlib functions handle this automatically, your current code doesn't. And yes, this does mean that to match a directory and all of its contents, you need to use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thank you for the thorough review. I was also hesitant at first about whether I wanted to implement all this complexities but I figured let's see if it's possible and let you/other reviewers see if it's appropriate. I've made the changes |
||
|
|
||
| def main(args=None): | ||
| """Run the zipapp command line interface. | ||
|
|
@@ -204,6 +246,15 @@ def main(args=None): | |
| help="Display the interpreter from the archive.") | ||
| parser.add_argument('source', | ||
| help="Source directory (or existing archive).") | ||
| parser.add_argument('--include', action='extend', nargs='+', default=None, | ||
| help=("Glob pattern(s) of files/dirs to include (relative to SOURCE). " | ||
| "Repeat the flag for multiple patterns. " | ||
| "To include a directory and its contents, use 'foo/**'.")) | ||
| parser.add_argument('--exclude', action='extend', nargs='+', default=None, | ||
| help=("Glob pattern(s) of files/dirs to exclude (relative to SOURCE). " | ||
| "Repeat the flag for multiple patterns. " | ||
| "To exclude a directory and its contents, use 'foo/**'. " | ||
| "Applied after --include.")) | ||
|
|
||
| args = parser.parse_args(args) | ||
|
|
||
|
|
@@ -222,9 +273,19 @@ def main(args=None): | |
| if args.main: | ||
| raise SystemExit("Cannot change the main function when copying") | ||
|
|
||
| # build a filter from include and exclude flags | ||
| filter_fn = None | ||
| src_path = pathlib.Path(args.source) | ||
| if src_path.exists() and src_path.is_dir() and (args.include or args.exclude): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we only supporting
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently, if a file source is given, we do a byte-copy operation other than modifying the shebang ( That being said, I'm happy to implement a "repack with filtering" functionality. Or I could have the CLI error when an archive source has filters applied. if os.path.isfile(args.source) and (args.include or args.exclude):
raise SystemExit("--include/--exclude only apply when SOURCE is a directory")Please let me know what you prefer and I'll update the PR accordingly @pfmoore .
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have a good instinct here. This is a problem that was introduced when we added the I don't want to add extra complexity (that needs to be maintained, tested and supported) when no-one will actually use it, but equally I don't think we should have a UI that suggests an "obvious" usage ( Honestly, at this point I'm wishing I'd stuck to my original principles and rejected the addition of the Let's just raise an error when the source is an existing zipapp and |
||
| filter_fn = _make_glob_filter( | ||
| includes=args.include, | ||
| excludes=args.exclude | ||
| ) | ||
|
|
||
| create_archive(args.source, args.output, | ||
| interpreter=args.python, main=args.main, | ||
| compressed=args.compress) | ||
| compressed=args.compress, | ||
| filter=filter_fn) | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Add flags ``--include`` and ``--exclude`` to the CLI of the :mod:`zipapp` module. These flags accept glob patterns to | ||
| indicate allow-list and/or deny-list of files to be included in the zipapp file. |
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.
Remove the second sentence (explaining priorities) from these two descriptions, because it's not very clear. Instead, have a separate paragraph that explains how the options interact. Something like this:
Also, add a note against
--includesaying "If this option is not specified, all files in the given directory are included by default."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've made that "second paragraph" as a note. Please let me know what other things need to be changed.
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 don't think these need to be notes, they are too visually imposing like that. Please make them ordinary text.
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 have made the change, please let me know if I should change something else