bpo-34097: Add support for zipping files older than 1980-01-01 #8270

ghost · 2018-07-13T11:43:43Z

This pull request adds a new option for zipping files older than 1980-01-01.
The user has to explicitly opt in by passing a keyword strict_timestamps=False to the ZipFile.write() method. This option will zip the file at the cost of setting the file timestamp to 1980-01-01 if older than zip limit.

https://bugs.python.org/issue34097

encukou · 2018-07-13T11:46:02Z

Lib/test/test_zipfile.py

+        # Set atime and mtime to 1970-01-01
+        os.utime(TESTFN, (0, 0))
+        with zipfile.ZipFile(TESTFN2, "w") as zipfp:
+            zipfp.write(TESTFN, strict_timestamps=False)


Please also test the resulting timestamp is 1980-01-01 as expected.

encukou · 2018-07-13T11:47:00Z

Doc/library/zipfile.rst

+   .. versionadded:: 3.8
+      The *strict_timestamps* keyword argument allows zip files older
+      than 1/1/1980 at the cost of setting the year to 1980.
+


Please describe the argument in the main text, then just use .. versionadded:: 3.8 The *strict_timestamps* keyword argument in the changelog. Same below.

encukou · 2018-07-13T11:48:06Z

Doc/library/zipfile.rst

+   .. versionadded:: 3.8
+      The *strict_timestamps* keyword argument allows zip files older
+      than 1/1/1980 at the cost of setting the year to 1980.
+


Please use the international and unambiguous "ISO format" to refer to dates: 1980-01-01.

bedevere-bot · 2018-07-13T11:48:42Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

ghost · 2018-07-13T12:11:45Z

I have made the requested changes; please review again

bedevere-bot · 2018-07-13T12:11:47Z

Thanks for making the requested changes!

@encukou: please review the changes made to this pull request.

vstinner · 2018-07-18T14:03:26Z

Doc/library/zipfile.rst

@@ -377,6 +377,8 @@ ZipFile Objects
   the new entry. Similarly, *compresslevel* will override the constructor if
   given.
   The archive must be open with mode ``'w'``, ``'x'`` or ``'a'``.
+   The *strict_timestamps* keyword argument allows to zip files older
+   than 1980-01-01 at the cost of setting the timestamp to 1980-01-01.


Just from the doc, I don't understand if I have to set the parameter to True or False to allow years older than 1980.

vstinner · 2018-07-18T14:08:45Z

Lib/zipfile.py

@@ -484,6 +484,8 @@ def from_file(cls, filename, arcname=None):
        isdir = stat.S_ISDIR(st.st_mode)
        mtime = time.localtime(st.st_mtime)
        date_time = mtime[0:6]
+        if not strict_timestamps and date_time[0] < 1980:
+            date_time = (1980, 1, 1, 0, 0, 0)


It seems like the largest year is 2107. Years after 2017 should also be clamped to 2017-12-31, no?

vstinner · 2018-07-18T14:09:28Z

Lib/test/test_zipfile.py

@@ -549,6 +549,14 @@ def test_add_file_before_1980(self):
        with zipfile.ZipFile(TESTFN2, "w") as zipfp:
            self.assertRaises(ValueError, zipfp.write, TESTFN)

+    def test_add_file_before_1980_no_strict_timestamps(self):
+        # Set atime and mtime to 1970-01-01
+        os.utime(TESTFN, (0, 0))


Instead of using the real filesystem which can cause troubles, maybe you can mock os.stat() intsead of return the date that you want?

It may allow to test date after year 2107.

The previous test is also using this. It shouldn't be a problem then.

bedevere-bot · 2018-07-18T14:09:45Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

vstinner · 2018-07-18T14:10:29Z

Doc/library/zipfile.rst

@@ -368,7 +368,7 @@ ZipFile Objects


 .. method:: ZipFile.write(filename, arcname=None, compress_type=None, \
-                          compresslevel=None)
+                          compresslevel=None, strict_timestamps=True)


I would prefer to use a keyword-only parameter. Same remark for other methods and functions.

Bikeshedding: why not just "strict_timestamp=True" (no S)?

there are more timestamps in the zip archive. each file has it's own.

vstinner · 2018-07-18T14:12:19Z

Misc/NEWS.d/next/Library/2018-07-13-13-42-10.bpo-34097.F5Dk5o.rst

@@ -0,0 +1,2 @@
+ZipFile can zip files older than 1980-01-01 at the cost of mangling the


You should mention the new timestamps parameter. And maybe explain "mangling": dates before 1980-01-01 are replaced by 1980-01-01 at 00:00.

ghost · 2018-07-31T12:01:51Z

I have made the requested changes; please review again.

bedevere-bot · 2018-07-31T12:01:53Z

Thanks for making the requested changes!

@vstinner, @encukou: please review the changes made to this pull request.

hroncok · 2018-07-31T13:25:41Z

Doc/library/zipfile.rst

+   The *strict_timestamps* keyword argument, when set to ``False``, allows to
+   zip files older than 1980-01-01 at the cost of setting the
+   timestamp to 1980-01-01.
+   Same behavior occures with files older than 2107-12-31.


It is not the same behavior, it is not set to 1980-01-01 in that case, but rather to 2107-12-31.

Maybe something like: "Similar behavior occurs with files ~~older~~ newer than 2107-12-31, the timestamp is also set to the limit."

Sounds good.
I have also noticed I have 'older' where 'newer' should be.

hroncok · 2018-07-31T13:27:13Z

Doc/library/zipfile.rst

+   The *strict_timestamps* keyword argument, when set to ``False``, allows to
+   zip files older than 1980-01-01 at the cost of setting the
+   timestamp to 1980-01-01.
+   Same behavior occures with files older than 2107-12-31.


vstinner

Aha, the new code now looks better! A few more comments.

vstinner · 2018-07-31T22:30:13Z

Doc/library/zipfile.rst

@@ -400,6 +404,9 @@ ZipFile Objects
      a closed ZipFile will raise a :exc:`ValueError`.  Previously,
      a :exc:`RuntimeError` was raised.

+   .. versionadded:: 3.8
+      The *strict_timestamps* keyword argument


nitpick: you may write "keyword-only", and also add a final dot.

vstinner · 2018-07-31T22:31:11Z

Doc/library/zipfile.rst

@@ -551,11 +559,19 @@ file:
   If *arcname* is not specified, the name will be the same as *filename*, but
   with any drive letter and leading path separators removed.

+   The *strict_timestamps* keyword argument, when set to ``False``, allows to


I don't think that it's needed to repeat here that it's a keyword argument, just write "argument".

vstinner · 2018-07-31T22:32:24Z

Lib/test/test_zipfile.py

+        with zipfile.ZipFile(TESTFN2, "w") as zipfp:
+            self.assertRaises(struct.error, zipfp.write, TESTFN)
+
+    def test_add_file_after_2107_no_strict_timestamps(self):


IMHO you can move this test inside test_add_file_after_2107(), no need to have two different tests.

vstinner · 2018-07-31T22:33:12Z

Lib/test/test_zipfile.py

+        with zipfile.ZipFile(TESTFN2, "w") as zipfp:
+            zipfp.write(TESTFN, strict_timestamps=False)
+            zinfo = zipfp.getinfo(TESTFN)
+            self.assertEqual(zinfo.date_time, (1980, 1, 1, 0, 0, 0))


I would help to also make sure that you can add error when using strict_timestamps=True here.

Code changed

vstinner

LGTM, but I would prefer to be the tests modified for my proposal.

vstinner · 2018-08-01T16:07:46Z

Lib/zipfile.py

@@ -484,6 +484,8 @@ def from_file(cls, filename, arcname=None):
        isdir = stat.S_ISDIR(st.st_mode)
        mtime = time.localtime(st.st_mtime)
        date_time = mtime[0:6]
+        if not strict_timestamps and date_time[0] < 1980:
+            date_time = (1980, 1, 1, 0, 0, 0)


vstinner · 2018-08-01T16:08:35Z

Lib/test/test_zipfile.py

+        os.utime(TESTFN, (4386268800, 4386268800))
+        with zipfile.ZipFile(TESTFN2, "w") as zipfp:
+            self.assertRaises(struct.error, zipfp.write, TESTFN)
+            zipfp.write(TESTFN, strict_timestamps=False)


I would prefer to reopen the file for the second test, since it's hard to guess what happens when on error occurs. I prefer to limit side effets of unit tests.

vstinner · 2018-08-01T16:08:39Z

Lib/test/test_zipfile.py

@@ -548,6 +548,18 @@ def test_add_file_before_1980(self):
        os.utime(TESTFN, (0, 0))
        with zipfile.ZipFile(TESTFN2, "w") as zipfp:
            self.assertRaises(ValueError, zipfp.write, TESTFN)
+            zipfp.write(TESTFN, strict_timestamps=False)


I would prefer to reopen the file for the second test, since it's hard to guess what happens when on error occurs. I prefer to limit side effets of unit tests.

the-knights-who-say-ni added the CLA signed label Jul 13, 2018

bedevere-bot added the awaiting review label Jul 13, 2018

encukou previously requested changes Jul 13, 2018

View reviewed changes

bedevere-bot removed the awaiting review label Jul 13, 2018

bedevere-bot added the awaiting changes label Jul 13, 2018

ghost changed the title ~~bpo-34097: Add support for zipping files older than 1/1/1980~~ bpo-34097: Add support for zipping files older than 1980-01-01 Jul 13, 2018

Marcel Plch added 5 commits July 13, 2018 14:08

Allow zipping files older than 1980-01-01

019df42

Add documentation for strict_timestamps

6259603

Add test for strict_timestamps

9bfe63f

Add NEWS entry.

44af203

fix requested changes

899acba

bedevere-bot removed the awaiting changes label Jul 13, 2018

bedevere-bot added the awaiting change review label Jul 13, 2018

vstinner requested changes Jul 18, 2018

View reviewed changes

bedevere-bot removed the awaiting change review label Jul 18, 2018

bedevere-bot added the awaiting changes label Jul 18, 2018

vstinner reviewed Jul 18, 2018

View reviewed changes

Marcel Plch added 6 commits July 30, 2018 17:50

Make requested changes

de25569

Mention strict_timestamps in NEWS

c1730a1

Fix buildbot fails.

f27234e

Fix buildbot fails #2.

bc7da17

Correct typo.

83e1e67

Fix buildbot fails #3.

0409cfa

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 31, 2018

hroncok reviewed Jul 31, 2018

View reviewed changes

vstinner reviewed Jul 31, 2018

View reviewed changes

Correct docs and simplify tests

e4cb0e0

vstinner approved these changes Aug 1, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Aug 1, 2018

Reopen file in tests.

cc45298

vstinner merged commit a2fe1e5 into python:master Aug 2, 2018

bedevere-bot removed the awaiting merge label Aug 2, 2018

ghost deleted the zipfile branch August 10, 2018 11:29

tirkarthi mentioned this pull request Oct 19, 2020

ERROR: ZIP does not support timestamps before 1980 magic-wormhole/magic-wormhole#401

Open

stefanpejcic mentioned this pull request Dec 2, 2024

OpenPanel > FileManager - Add support for zipping files with date older than 1980-01-01 stefanpejcic/OpenPanel#287

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-34097: Add support for zipping files older than 1980-01-01 #8270

bpo-34097: Add support for zipping files older than 1980-01-01 #8270

ghost commented Jul 13, 2018 •

edited by ghost

Loading

encukou Jul 13, 2018

encukou Jul 13, 2018

encukou Jul 13, 2018

bedevere-bot commented Jul 13, 2018

ghost commented Jul 13, 2018

bedevere-bot commented Jul 13, 2018

vstinner Jul 18, 2018

vstinner Jul 18, 2018

vstinner Aug 1, 2018

vstinner Jul 18, 2018

ghost Jul 18, 2018

bedevere-bot commented Jul 18, 2018

vstinner Jul 18, 2018

vstinner Jul 18, 2018

hroncok Jul 31, 2018

vstinner Jul 18, 2018

ghost commented Jul 31, 2018

bedevere-bot commented Jul 31, 2018

hroncok Jul 31, 2018

hroncok Jul 31, 2018 •

edited

Loading

ghost Jul 31, 2018

hroncok Jul 31, 2018

vstinner left a comment

vstinner Jul 31, 2018

vstinner Jul 31, 2018

vstinner Jul 31, 2018

vstinner Jul 31, 2018

vstinner left a comment

vstinner Aug 1, 2018

vstinner Aug 1, 2018

vstinner Aug 1, 2018

		@@ -0,0 +1,2 @@
		ZipFile can zip files older than 1980-01-01 at the cost of mangling the

bpo-34097: Add support for zipping files older than 1980-01-01 #8270

bpo-34097: Add support for zipping files older than 1980-01-01 #8270

Conversation

ghost commented Jul 13, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jul 13, 2018

ghost commented Jul 13, 2018

bedevere-bot commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 31, 2018

bedevere-bot commented Jul 31, 2018

Choose a reason for hiding this comment

hroncok Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 13, 2018 •

edited by ghost

Loading

hroncok Jul 31, 2018 •

edited

Loading