| 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) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2012-03-27 14:47 |
| Fixed patch with tests attached. |
|
|
| msg156944 - (view) |
Author: R. David Murray (r.david.murray) * |
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) * |
Date: 2012-03-27 18:43 |
| My fault. |
|
|
| msg156954 - (view) |
Author: Ethan Furman (ethan.furman) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2012-04-13 02:31 |
| I mean "with self.assertRaises(TypeError):". |
|
|
| msg158196 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
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) * |
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) * |
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) * |
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) * |
Date: 2015-05-14 11:53 |
| Just for the record, David's comment at 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) * |
Date: 2015-05-14 12:20 |
| :( :( OK, next time this comes up I won't say "it will probably be OK". |
|
|