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

Extract ZipInfo for archive functionality #123424

Closed
jaraco opened this issue Aug 28, 2024 · 7 comments · Fixed by #123429
Closed

Extract ZipInfo for archive functionality #123424

jaraco opened this issue Aug 28, 2024 · 7 comments · Fixed by #123429
Assignees
Labels
easy stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@jaraco
Copy link
Member

jaraco commented Aug 28, 2024

In #123354, I found the need to copy code from zipfile into the test:

def for_name(cls, name, archive):
"""
Construct the same way that ZipFile.writestr does.
TODO: extract this functionality and re-use
"""
self = cls(filename=name, date_time=time.localtime(time.time())[:6])
self.compress_type = archive.compression
self.compress_level = archive.compresslevel
if self.filename.endswith('/'): # pragma: no cover
self.external_attr = 0o40775 << 16 # drwxrwxr-x
self.external_attr |= 0x10 # MS-DOS directory flag
else:
self.external_attr = 0o600 << 16 # ?rw-------
return self

Let's instead extract that functionality in the zipfile module for re-use in the test (before it starts diverging).

Linked PRs

@jaraco jaraco assigned jaraco and unassigned jaraco Aug 28, 2024
@jaraco jaraco added stdlib Python modules in the Lib dir easy labels Aug 28, 2024
@picnixz
Copy link
Member

picnixz commented Aug 28, 2024

@jaraco are you taking it or not? if not, I can take it

@jaraco
Copy link
Member Author

jaraco commented Aug 28, 2024

Originally, I was planning to do it, but then I realized it might be an interesting one for others. Have at it!

@serhiy-storchaka
Copy link
Member

See also #121402 that implements a similar feature.

I paused it because I am planning to try a different approach.

@obfusk
Copy link
Contributor

obfusk commented Sep 2, 2024

Nothing against extracting the functionality if it's useful elsewhere, but the test doesn't actually depend on the compression or any of the metadata set by for_name(), just the file name. Nothing else should matter to zipfile.Path here. So removing for_name() and just using zf.writestr(DirtyZipInfo("foo\\bar"), b"content") should work fine.

@jaraco
Copy link
Member Author

jaraco commented Sep 2, 2024

When I added the for_name to DirtyZipInfo, I was aiming to replace writestr without altering the way it worked. That is, every other test relies on writestr, so I wanted to construct an equivalent ZipInfo, assuming that whoever wrote writestr had reasons for defaulting the date_time, compression type, compression level, and external attrs. I wanted to avoid having to research the motivations for each of those concerns to see if they may apply to this specific case. I wanted to avoid the possibility that those attributes had a meaningful effect or worse did not have an effect today but would in the future. More importantly, I wanted to be sure that if writestr changed in the future to add additional behavior, this routine would inherit that same behavior. By reducing incidental variance, we improve reliability long-term.

@obfusk
Copy link
Contributor

obfusk commented Sep 2, 2024

Passing a ZipInfo to writestr() will not set any of those attributes like passing only a filename (and thus having writestr() create the ZipInfo) will and is clearly supported as part of the current API. I rely on this regularly for Reproducible Builds where I need to set the attributes to specific values and using the current time would definitely be unacceptable.

And the fact that writestr() by default uses the current time means no two test runs ever use the exact same metadata, which is something I would generally try to avoid.

That said, even if I very much doubt any of the metadata would ever affect this specific test, and would consider that changing to be a bug, I agree that ensuring consistency wrt to writestr() behaviour between different tests is good. Just wanted to clarify the current situation :)

@picnixz
Copy link
Member

picnixz commented Dec 22, 2024

Removing my assignment on this task since I didn't continue working on the associated PR (and have to prioritize my backlog).

@picnixz picnixz removed their assignment Dec 22, 2024
@jaraco jaraco self-assigned this Dec 28, 2024
jaraco added a commit that referenced this issue Dec 29, 2024
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this issue Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy stdlib Python modules in the Lib dir type-feature A feature request or enhancement
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants