bpo-40481: Add include and exclude filters to zipapp cli #19879
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
Lib/zipapp.py
Outdated
| def create_archive(source, target=None, interpreter=None, main=None, | ||
| filter=None, compressed=False): | ||
| filter=None, compressed=False, **filter_kwargs): |
There was a problem hiding this comment.
I'm concerned about file arguments for the filter being "anything that doesn't already have a meaning". What if we want to add another argument to the create_archive function? This functionality seems a bit too niche to "steal" all of the remaining potential argument names.
I'd prefer the filter_kwargs argument to be a normal argument that takes a dictionary. Or maybe even just pass a single argument to the filter - let the filter decide how to interpret it.
| @@ -138,7 +154,7 @@ def create_archive(source, target=None, interpreter=None, main=None, | |||
| with zipfile.ZipFile(fd, 'w', compression=compression) as z: | |||
| for child in source.rglob('*'): | |||
| arcname = child.relative_to(source) | |||
| if filter is None or filter(arcname): | |||
| if filter is None or filter(arcname, **filter_kwargs): | |||
There was a problem hiding this comment.
Note that for backward compatibility, if you change filter_kwargs to be a single argument as I suggest above, you'll need to still allow for filters with no arguments. The current code handles this case, but as mentioned, I don't like the current approach.
| ) | ||
| parser.add_argument("--exclude", "-e", default=[], nargs="*", | ||
| help="A glob-style sequence of paths to exclude.", | ||
| ) |
There was a problem hiding this comment.
What are the use cases for these defaults? They seem reasonable, but personally I've never needed this functionality, so I have no easy way to judge whether this would be more useful than (for example) the ability to exclude a list of named subdirectories.
I'd expect the command line API to handle basic cases, but for more complex uses I'd assume people would write a custom build script that used the create_archive function directly. So I'd like to see some evidence that this particular form of filter is sufficiently common to warrant being a feature of the command line version - otherwise it could become a precedent for more and more complexity in the CLI, which is something I think we should avoid.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
@pfmoore Don't forget to add a news update since you've changed code, add it manually or by using this app: https://blurb-it.herokuapp.com/add_blurb |
|
@jwygoda, please address the code reviews. Thank you! |
|
@csabella Thanks for the ping, I missed this review. @pfmoore This feature is usefull for packaging .pyc only distributions and excluding files with some extensions (e.g. images). I'm deploying packages to aws lambda where there's a limit to package size. These defaults were inspired by find_packages from setuptools. Include and exclude filters are quite usefull for fine graining package content. |
|
Ping, @pfmoore? |
|
@jwygoda Apologies, this dropped off my radar. Are you still interested in working on this? If you are, I'm now happy with the I'd be looking for examples of workflows where you'd build a zipapp using these options - it feels to me as if you'd be better excluding files when you are preparing the directory for zipping, rather than using filters, so I'd like to see examples that demonstrate when that's not practical. |
https://bugs.python.org/issue40481