Use writestr to zip files with mtime before 1980

[DiegoPomares]

Related: #2639

[et304383]

I don't feel this should be necessary. Uglify-JS needs to release a fix for

mishoo/UglifyJS#2054

Edit: the reason I don't feel this should be needed here is because this is only going to patch ONE issue caused by these broken timestamps. The underying issue should be fixed rather than trying to patch every possible thing out there that doesn't handle epoch 0 timestamps.

[JordonPhillips]

I'm -1 on modifying files without explicit consent. Maybe if there is a command line flag to opt in to this behavior, but even then I'm still hesitant to deliberately upload obviously corrupted files. With cp for instance we will skip files that have invalid timestamps.

[stealthycoin]

Agreed with @eric-tucker and @JordonPhillips I would rather the user be alerted their file is corrupted rather than paint over it.

[dstufft]

I also agree, we shouldn't be modifying someone's files, even one as minor as changing the mtime, without their knowledge or consent.

[jamesls]

(Just catching up on this now). I suppose I have a basic question. Why is the CLI zip command able to handle timestamps prior to 1980? And is there any way we can do the same thing in python? I'm also against modifying mtime's without the user opting in, but I'd like to know if there's a way we can just preserve timestamps to begin with.

$ ls -la
total 0
-rw-r--r--   1 james              0 Dec 31  1969 a
$ zip -r a.zip ./
  adding: a (stored 0%)
$ unzip -l a.zip
Archive:  a.zip
  Length     Date   Time    Name
 --------    ----   ----    ----
        0  12-31-69 16:00   a
 --------                   -------
        0                   1 file
$ rm a
$ ls -la
total 8
-rw-r--r--   1 james            152 Jun 12 10:35 a.zip
$ unzip a.zip
Archive:  a.zip
 extracting: a
$ ls -la
-rw-r--r--   1 james              0 Dec 31  1969 a
-rw-r--r--   1 james            152 Jun 12 10:35 a.zip

[dstufft]

Well, zip files don't natively support it, the spec explicitly says the modification time is indicated by years since 1980. Zip files do support extra metadata being stored, so maybe the zip utility is storing the real mtime as an extra field?

[codecov-io]

Codecov Report

Merging #2646 into develop will decrease coverage by 0.03%.
The diff coverage is 42.85%.

@@             Coverage Diff             @@
##           develop    #2646      +/-   ##
===========================================
- Coverage    95.56%   95.53%   -0.04%     
===========================================
  Files          151      151              
  Lines        11798    11805       +7     
===========================================
+ Hits         11275    11278       +3     
- Misses         523      527       +4

Impacted Files	Coverage Δ
...customizations/cloudformation/artifact_exporter.py	95.08% <42.85%> (-2.08%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13b7fd0...e57de80. Read the comment docs.

[DiegoPomares]

Seems like I failed to realize that modifying the timestamp was a big deal. I rewrote the whole thing, instead of modifying the timestamp in the source file, I switched to writestr [1] which adds the file to the zip with the current time.

The only caveat is that the whole file is read into memory before adding it to the zip, but I don't think that should be a big deal under most circumstances.

[1] https://docs.python.org/3/library/zipfile.html#zipfile.ZipFile.writestr

[DiegoPomares]

Notice this patch is only for 'aws cloudformation package', but the zipfile module is used in several other places, like lambda and codedeploy. If you consider the writestr approach is good enough, perhaps it is best to write a wrapper module for zipfile, to have something consistent. I can code the whole thing as soon as you agree on a solution.

[dstufft]

Does this have the effect that the mtime is set to the current time in the generated zip file if it was previously set to before 1980?

[DiegoPomares]

dstufft@ that's right, it will only override the mtime for the file inside the zip, not the source file.

[tim-finnigan]

There hasn't been any activity on this PR for a few years, and the initial maintainer response was against these changes. The original issue referenced (#2639) seems to suggest the problem was caused by timestamp issues introduced by a third-party library that have since been fixed. I think this PR should be closed but can be revisited if necessary.