Repository navigation
More flexibility in zipfile write interface #70227
Description
| BPO | 26039 |
|---|---|
| Nosy | @takluyver, @vadmium, @serhiy-storchaka, @Carreau |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2016-05-15.09:28:40.190>
created_at = <Date 2016-01-07.15:10:51.849>
labels = ['type-feature', 'library']
title = 'More flexibility in zipfile write interface'
updated_at = <Date 2016-05-15.17:27:05.746>
user = 'https://github.com/takluyver'bugs.python.org fields:
activity = <Date 2016-05-15.17:27:05.746>
actor = 'takluyver'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-05-15.09:28:40.190>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2016-01-07.15:10:51.849>
creator = 'takluyver'
dependencies = []
files = ['41525', '41626', '41637', '41722', '41726', '41752', '41753', '41754', '41758', '41759', '42311', '42375', '42588', '42597', '42598', '42819', '42850', '42851', '42855']
hgrepos = []
issue_num = 26039
keywords = ['patch']
message_count = 58.0
messages = ['257694', '257696', '257697', '258313', '258459', '258949', '258958', '258959', '259022', '259184', '259206', '259209', '259255', '259258', '259553', '259805', '259806', '259833', '260305', '260733', '260734', '260746', '260808', '260810', '260852', '261162', '261168', '261328', '261332', '262534', '262537', '262543', '262557', '262926', '264062', '264070', '264169', '264205', '264242', '264248', '264720', '265232', '265330', '265386', '265388', '265459', '265461', '265532', '265540', '265541', '265544', '265546', '265559', '265580', '265592', '265603', '265605', '265635']
nosy_count = 6.0
nosy_names = ['python-dev', 'takluyver', 'martin.panter', 'serhiy.storchaka', 'mbussonn', 'Dhiraj_Mishra']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue26039'
versions = ['Python 3.6']
Activity
takluyver commentedon Jan 7, 2016
I was working recently on some code writing zip files, and needed a bit more flexibility than the interface of zipfile provides. To do what I wanted, I ended up copying the entirety of ZipFile.write() into my own code with modifications. That's ugly, and to make it worse, the code I copied from the Python 3.4 stdlib didn't work properly with Python 3.5.
Specifically, I want to:
I could do both by using ZipFile.writestr(), but then the entire file must be loaded into memory at once, which I would much rather avoid.
The patch attached is one proposal which would make it easier to do what I want, but it's intended as a starting point for discussion. I'm not particularly attached to the API.
Should this be a new method, or new functionality for either the write or writestr method? I favour a new method, because the existing methods are already quite long, and I think it's nicer to break write() up into two parts like this.
If a new method, what should it be called? I opted for writefile()
What should its signature be? It's currently writefile(file_obj, zinfo, force_zip64=False), but I can see an argument for aligning it with writestr(zinfo_or_arcname, data, compress_type=None).
What to do about ZIP64: the code has to decide whether to use ZIP64 format before writing, because it affects the header size, but we may not know the length before we have read it all. I've used a force_zip64 boolean parameter, and documented that people should pass it True if a file of unknown size could exceed 2 GiB.
Are there other points where it could be made more flexible while we're thinking about this? I'd quite like to split out the code for writing a directory entry to a separate method ('writedir'?). I'd also like to allow datetime objects to be passed in for timestamps as well as the 6-tuples currently expected.
Carreau commentedon Jan 7, 2016
from the last blog post of Brett Cannon, I would say that he might be interested in Participating to this discussion (Rewrite zipimport from scratch section)
Though I'm not sure about the etiquette to ping someone on such an issue.
serhiy-storchaka commentedon Jan 7, 2016
bpo-11980 has a patch that adds a method for writing a file into the ZIP archive.
But more flexible way is to add the support of the "w" mode to ZipFile.open(). This will allow to write a data that doesn't exist in memory at once, nor in disk, but is loaded by chunks from the pipe, or from the socket, or from other archive, or just generated on fly. I'm worked on this, but this feature needed to add support of some other features. For example writing to unseekable file (already implemented) or writing and reading a file without known size (not implemented still).
bpo-18595 has a proposition to add special methods for creating symlinks, directories etc.
takluyver commentedon Jan 15, 2016
Attached is a first go at a patch enabling zipfile.open(blah, mode='w')
Files must be written sequentially, so you have to close one writing handle before opening another. If you try to open a second one before closing the first, it will raise RuntimeError. I considered doing something where it would write to temporary files and add them to the zip file when they were closed, but it seemed like a bad idea.
You can almost certainly break this by reading from a zip file while there's an open writing handle. Resolving this is tricky because there's a disconnect in the requirements for reading and writing: writing allows for a non-seekable output stream, but reading assumes that you can seek freely. The simplest fix is to block reading while there is an open file handle. I don't think many people will need to read one file from a zip while writing another, anyway.
I have used the lock, but I haven't thought carefully about thread safety, so that should still be checked carefully.
takluyver commentedon Jan 17, 2016
zipinfo-from-file.patch has an orthogonal but related change: the code in ZipFile.write() to construct a ZipInfo object from a filesystem file is pulled out to a classmethod ZipInfo.from_file().
Together, these changes make it much easier to control how a file is written to a zip file, like this:
If these changes make it in, I might put a backported copy of the module on PyPI so I can start using it without waiting for Python 3.6.
takluyver commentedon Jan 26, 2016
Serhiy, any chance you'd have some time to review my patch(es)? Or is there someone else interested in zipfile I might interest? Thanks :-)
serhiy-storchaka commentedon Jan 26, 2016
Your patches are great Thomas! This is just what I want to implement. It will take some time to make a careful review. Besides possible corrections I think these features will be added in 3.6. But new features need tests and documenting.
takluyver commentedon Jan 26, 2016
Thanks! I will work on docs and tests.
takluyver commentedon Jan 27, 2016
The '2' versions of the two different patches include some docs and tests for these new features.
vadmium commentedon Jan 29, 2016
I left some review comments.
Since the rules for read-only and write-only access are so different, would it make more sense to have a separate method? Maybe your writefile() name, or open_write() or something? I am not too fussed, but unless there is a chance of being able to open a read-write random access file, I find these split-personality open() methods a bit “un-Pythonic”. On the other hand, I guess it is superficially consistent with other open() functions.
Also, perhaps if you guaranteed the write-only option returned a file-like object, you could use shutil.copyfileobj() rather than a custom read-write loop.
takluyver commentedon Jan 29, 2016
Here's a new version of the zf.open() patch following Martin's review (thanks Martin!).
I agree that it feels a bit awkward having two completely different actions for zf.open(), but it is a familiar interface, and since the mode parameter is already there, it requires a minimum of new public API. But I'm happy to add a new method like open_write() or write_handle() if people prefer that.
The comments on the other patch are minimal, I'll put a new version of that together as well.
takluyver commentedon Jan 29, 2016
Thanks Serhiy for review comments.
36 remaining items
takluyver commentedon May 14, 2016
Thanks Serhiy! I am marking this as fixed.
serhiy-storchaka commentedon May 14, 2016
Let me know when you create a backport package Thomas. I'd like to help.
serhiy-storchaka commentedon May 14, 2016
There are yet few issues.
Currently RuntimeError is widely used in zipfile. But in most cases ValueError would be more appropriate since it programming error (for example ValueError is raised when try to read or write to closed file). See bpo-24693, but this is backward incompatible change. For new exceptions we are free to use any exception type without breaking backward compatibility. It looks to me, that ValueError is more appropriate is these cases than RuntimeError. What do you think about this Thomas? Here is a patch that changes exceptions types.
ZipInfo.is_dir() is not documented and lacks a docstring.
It would be better to make force_zip64 a keyword-only parameter. This would allow to add new positional parameters without breaking compatibility.
takluyver commentedon May 14, 2016
If I was writing it anew, I would use ValueError for something like an invalid mode parameter: zf.open('foo', mode='q') . That particular case was already in the code, though, so I think backwards compatibility should take precedence.
2 & 3. Agreed, I'll prepare a patch
takluyver commentedon May 14, 2016
Patch attached for points 2 and 3
vadmium commentedon May 14, 2016
The bonus patch looks okay, although I wonder if the directory slash (/) information should be in the RST rather than doc string. Usually the RST has all the details, and doc strings are just summaries.
Regarding exceptions, I can sympathise with both sides of the argument and don’t have a strong opinion (why does the exception type matter for programmer errors anyway?). But I think it might be better to be locally consistent within the zipfile module, and the module is already heavily documented with RuntimeError for similar programmer errors.
serhiy-storchaka commentedon May 15, 2016
Agreed that the docstring should be shorten version of the documentation and not vice versa. The directory slash information is implementation detail. I'm not sure it is needed in the documentation.
takluyver commentedon May 15, 2016
Updated version of the patch with the detail moved to rst.
python-dev commentedon May 15, 2016
New changeset 29b470b8ab77 by Serhiy Storchaka in branch 'default':
Issue bpo-26039: Document ZipInfo.is_dir() and make force_zip64 keyword-only.
https://hg.python.org/cpython/rev/29b470b8ab77
serhiy-storchaka commentedon May 15, 2016
LGTM. Thank you Thomas.
takluyver commentedon May 15, 2016
The backported package now exists on PyPI as zipfile36. It's passing the tests on 3.4 - I haven't tested further back. I'm trying out Gitlab for hosting it:
https://gitlab.com/takluyver/zipfile36