Skip to content
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

bpo-31072: Add filter to zipapp #3021

Merged
merged 4 commits into from
Aug 9, 2017
Merged

bpo-31072: Add filter to zipapp #3021

merged 4 commits into from
Aug 9, 2017

Conversation

jeffreyrack
Copy link
Contributor

@jeffreyrack jeffreyrack commented Aug 8, 2017

Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

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

In addition to the 2 noted points, the travis check failed with a report of whitespace issues, and the documentation needs to be updated.

Lib/zipapp.py Outdated
@@ -73,7 +73,8 @@ def _copy_archive(archive, new_archive, interpreter=None):
os.chmod(new_archive, os.stat(new_archive).st_mode | stat.S_IEXEC)


def create_archive(source, target=None, interpreter=None, main=None):
def create_archive(source, target=None, interpreter=None, main=None,
include_file=None):
Copy link
Member

Choose a reason for hiding this comment

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

Indentation needs fixing here

Lib/zipapp.py Outdated
@@ -134,8 +135,9 @@ def create_archive(source, target=None, interpreter=None, main=None):
_write_file_prefix(fd, interpreter)
with zipfile.ZipFile(fd, 'w') as z:
for child in source.rglob('*'):
arcname = child.relative_to(source).as_posix()
z.write(child, arcname)
if include_file is None or include_file(pathlib.Path(child)):
Copy link
Member

Choose a reason for hiding this comment

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

The include_file check should take arcfile as argument - specifically it should be passed a path that is relative to the source directory.

@pfmoore
Copy link
Member

pfmoore commented Aug 8, 2017

Thanks for the PR on this - I've made a couple of comments that need to be addressed, but basically the change looks good.

I'm going to be unable to get back to this for a week or so, so if I don't comment any further for a while, don't worry - I will get back to it!

@pfmoore
Copy link
Member

pfmoore commented Aug 9, 2017

I've added a NEWS entry to your PR. Otherwise, this looks good to go.

@pfmoore pfmoore merged commit b811d66 into python:master Aug 9, 2017
@pfmoore
Copy link
Member

pfmoore commented Aug 9, 2017

Thanks for the contribution!

@jeffreyrack jeffreyrack deleted the 31072 branch October 7, 2019 01:31
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.

3 participants