classification
Title: zipfile and creat/update comment
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Anthony.Kong, acassaigne, ethan.furman, jeffknupp, mark.dickinson, python-dev, r.david.murray, serhiy.storchaka
Priority: normal Keywords: easy, patch

Created on 2012-03-24 16:27 by acassaigne, last changed 2015-05-14 12:20 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
bug_zipfile_comment.py acassaigne, 2012-03-24 16:27
fix_zipfile_comment_simple.patch serhiy.storchaka, 2012-03-27 11:45 Simple patch review
fix_zipfile_comment_complex.patch serhiy.storchaka, 2012-03-27 11:46 No so simple patch review
fix_zipfile_comment_2.patch serhiy.storchaka, 2012-03-27 14:47 review
fix_zipfile_comment_3.patch serhiy.storchaka, 2012-03-27 18:43 review
fix_zipfile_comment_4.patch serhiy.storchaka, 2012-04-12 13:44 Fix TypeError raising review
fix_zipfile_comment_4-2.7.patch serhiy.storchaka, 2012-04-12 13:44 For Python 2.7 review
Messages (26)
msg156699 - (view) Author: Cassaigne (acassaigne) Date: 2012-03-24 16:27
I want to update or create a comment to zip file.

For instance, I have test.zip file.
I'm using these statement to create a comment :

from zipfile import ZipFile

z=ZipFile('test.zip','a')
z.comment='Create a new comment'
z.close()

After to ran this script, the zip file test.zip doesn't including the new comment !

I can have the expected behavior when I add a new file inner zip archive.
msg156793 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-03-26 00:46
I'm marking this as easy hoping someone in the mentoring group will pick it up and take a look.  The relevant code is in Python, and I'm guessing there is some logic bug when only the comment is set (and nothing is added to the zipfile), but I haven't looked at the code.  I'm also adding 3.2 and 3.3; it fails on 3.3 as well.
msg156804 - (view) Author: Cassaigne (acassaigne) Date: 2012-03-26 08:55
Tanks à lot. To complete Information about this bug.

it up and take a look.  The relevant code is in Python, and I'm guessing
there is some logic bug when only the comment is set (and nothing is added
to the zipfile), but I haven't looked at the code.  I'm also adding 3.2 and
3.3; it fails on 3.3 as well.
>
> ----------
> keywords: +easy
> nosy: +r.david.murray
> stage:  -> needs patch
> versions: +Python 3.2, Python 3.3
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue14399>
> _______________________________________
>
msg156885 - (view) Author: Jeff Knupp (jeffknupp) * Date: 2012-03-27 00:51
I'm unable to reproduce on either 2.7 or 3.3. Running the following:

from zipfile import ZipFile

z=ZipFile('test.zip','a')
z.comment='Create a new comment'
z.close()

produces the output:

Archive:  test.zip
Create a new comment

with the comment changed to b'...' for 3.3. Note that in 3.3 if you set the comment to a string and try to close, an exception will be raised and the test.zip file will not have a comment. 

Odd that I don't see the bug on either branch. Can someone else confirm?
msg156886 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-03-27 00:57
Hmm.  When I did the same (changed to b'...') and ran it, I got an archive test.zip.  When I then opened that archive using ZipFile, its comment attribute was b''.  Thus I concluded that the bug exists in Python3.  I don't remember seeing any output from running the script, and indeed I would not expect to.  How did you run it?

Also, I used the uniz unzip command on the produced archive, and got a corrupted archive error message and a warning about an incorrect comment length.
msg156912 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-03-27 11:45
The bug only occurs for non-empty archives.

There are two ways of fixing this bug -- very simple and no so simple. Patches are attached.
msg156919 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-03-27 12:45
Thanks for the patch.

The advantage of the more complicated way being that you get an immediate TypeError instead of a delayed one?  Is there any other advantage? (That's probably enough reason, though :)

Now we just need a unit test for this.
msg156925 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-03-27 13:58
This is the second advantage. The first one is that nothing is to be written when zipfile opened in a mode "a", if we don't change anything.
msg156926 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-03-27 14:03
Got you.  We'll definitely go with the more complete fix, then.
msg156931 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-03-27 14:47
Fixed patch with tests attached.
msg156944 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-03-27 17:24
The tests don't seem to be included in the new patch.  Also, you could modernize the property definition (@property, @comment.setter).
msg156950 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-03-27 18:43
My fault.
msg156954 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2012-03-27 19:13
I see a comment around line 240 that says:

    # It is assumed that the "end of central directory" magic
    # number does not appear in the comment.

Any reason not to add that check to comment.setter?
msg158094 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-04-12 01:16
Serhiy: this looks good.  I get some test errors when I apply it on 2.7 though.  Would you be interested in doing a 2.7 version as well?

(Minor comment: the test method would be better as two test methods, and it would be nice to have a third test method that checked for the TypeError for non-binary comments...that won't apply to 2.7, obviously).
msg158128 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-04-12 13:44
Thank you, the TypeError test helped me find the error. Here is the
corrected patch.

For 2.7 it was necessary to turn the ZipFile in the new-style class.
msg158136 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-04-12 14:35
Thanks.

We've had trouble in the past with a conversion to new style class breaking people's code.  People are less likely to be subclassing ZipFile, though, so it is probably OK.
msg158182 - (view) Author: Roundup Robot (python-dev) Date: 2012-04-12 22:45
New changeset ac0ec1f31b0a by R David Murray in branch '2.7':
#14399: zipfile now correctly handles comments added to empty zipfiles.
http://hg.python.org/cpython/rev/ac0ec1f31b0a

New changeset 4186f20d9fa4 by R David Murray in branch '3.2':
#14399: zipfile now correctly handles comments added to empty zipfiles.
http://hg.python.org/cpython/rev/4186f20d9fa4

New changeset e5b30d4b0647 by R David Murray in branch 'default':
Merge #14399: zipfile now correctly handles comments added to empty zipfiles.
http://hg.python.org/cpython/rev/e5b30d4b0647
msg158183 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-04-12 22:47
Thanks, Serhiy.

I made one small change, using 'with self.assertEqual' in the TypeError test.  You might want to check that out, it is a useful technique.

Oh, and I removed the type check from the 2.7 patch.  You can use a unicode string as long as it doesn't contain non-ASCII (the reason we created python3!), so it would be a backward incompatible change to raise a TypeError there.
msg158193 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-04-13 02:31
I mean "with self.assertRaises(TypeError):".
msg158196 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-04-13 05:34
> I mean "with self.assertRaises(TypeError):".

Thank you, I did not know that the context managers may catch exceptions
from the nested block.
msg158197 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-04-13 05:43
> #14399: zipfile now correctly handles comments added to empty zipfiles.

This is a wrong description of the issue.  On the contrary, the error
occurred when adding or changing the comment in a *non-empty* zipfile
without adding or changing files.

Description in the Misc/NEWS also wrong.
msg158208 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-04-13 13:25
Ah.  I based that on the fact that the third test passed without the change.  I thought you were adding that test of changing the comment just as a double check.  I should have asked instead of assuming.
msg158244 - (view) Author: Roundup Robot (python-dev) Date: 2012-04-14 01:27
New changeset b3b7f9dd7ce4 by R David Murray in branch '3.2':
#14399: corrected news item
http://hg.python.org/cpython/rev/b3b7f9dd7ce4

New changeset 225126c9d4b5 by R David Murray in branch '2.7':
#14399: corrected news item
http://hg.python.org/cpython/rev/225126c9d4b5

New changeset 160245735299 by R David Murray in branch 'default':
Merge #14399: corrected news item
http://hg.python.org/cpython/rev/160245735299
msg158245 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-04-14 01:28
I must have been seeing what I expected to see.  The test that failed was the non-empty test.

News item fixed, thanks for the correction.
msg243180 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2015-05-14 11:53
Just for the record, David's comment at msg158136 is apposite:

> We've had trouble in the past with a conversion to new style class
> breaking people's code.  People are less likely to be subclassing
> ZipFile, though, so it is probably OK.

We just spent some time this morning fixing a bug in some internal software; the cause was precisely this unexpected change from old-style class to new-style class in a bugfix release. And indeed, we were subclassing ZipFile. :-(
msg243184 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-05-14 12:20
:( :(

OK, next time this comes up I won't say "it will probably be OK".
History
Date User Action Args
2015-05-14 12:20:14r.david.murraysetmessages: + msg243184
2015-05-14 11:53:50mark.dickinsonsetnosy: + mark.dickinson
messages: + msg243180
2012-04-14 01:28:42r.david.murraysetmessages: + msg158245
2012-04-14 01:27:46python-devsetmessages: + msg158244
2012-04-13 13:25:28r.david.murraysetmessages: + msg158208
2012-04-13 05:43:33serhiy.storchakasetmessages: + msg158197
2012-04-13 05:34:30serhiy.storchakasetmessages: + msg158196
2012-04-13 02:31:02r.david.murraysetmessages: + msg158193
2012-04-12 22:47:15r.david.murraysetstatus: open -> closed
resolution: fixed
messages: + msg158183

stage: patch review -> resolved
2012-04-12 22:45:16python-devsetnosy: + python-dev
messages: + msg158182
2012-04-12 14:35:55r.david.murraysetmessages: + msg158136
2012-04-12 13:44:52serhiy.storchakasetfiles: + fix_zipfile_comment_4.patch, fix_zipfile_comment_4-2.7.patch

messages: + msg158128
2012-04-12 01:17:50r.david.murraysetmessages: - msg158095
2012-04-12 01:17:21r.david.murraysetmessages: + msg158095
2012-04-12 01:16:45r.david.murraysetmessages: + msg158094
stage: test needed -> patch review
2012-03-27 19:13:03ethan.furmansetnosy: + ethan.furman
messages: + msg156954
2012-03-27 18:43:03serhiy.storchakasetfiles: + fix_zipfile_comment_3.patch

messages: + msg156950
2012-03-27 17:24:42r.david.murraysetmessages: + msg156944
2012-03-27 14:47:16serhiy.storchakasetfiles: + fix_zipfile_comment_2.patch

messages: + msg156931
2012-03-27 14:03:29r.david.murraysetmessages: + msg156926
2012-03-27 13:58:30serhiy.storchakasetmessages: + msg156925
2012-03-27 12:45:02r.david.murraysetmessages: + msg156919
stage: needs patch -> test needed
2012-03-27 11:46:25serhiy.storchakasetfiles: + fix_zipfile_comment_complex.patch
2012-03-27 11:45:39serhiy.storchakasetfiles: + fix_zipfile_comment_simple.patch

nosy: + serhiy.storchaka
messages: + msg156912

keywords: + patch
2012-03-27 00:57:04r.david.murraysetmessages: + msg156886
2012-03-27 00:51:05jeffknuppsetnosy: + jeffknupp
messages: + msg156885
2012-03-26 19:43:11Anthony.Kongsetnosy: + Anthony.Kong
2012-03-26 08:55:38acassaignesetmessages: + msg156804
2012-03-26 00:46:27r.david.murraysetversions: + Python 3.2, Python 3.3
nosy: + r.david.murray

messages: + msg156793

keywords: + easy
stage: needs patch
2012-03-24 16:27:10acassaignecreate