Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: set timestamp to a higher value to handle timezones #2005

Closed
wants to merge 6 commits into from

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented Oct 19, 2020

What's the problem this PR addresses?

The timestamp we use for packing is the minimum value zip can store, but due to timezone shenanigans the value is either valid or invalid depending on which timezone you reside in.

See babel/babel#12125 (comment) for more details

Fixes #1882
ref #1885

cc @nicolo-ribaudo @JLHwung

How did you fix it?

Move the timestamp away from the minimum value and regenerate the cache.
The timestamp is now 1984-06-22T21:50:00.000Z (456789000 Unix Timestamp)

Checklist

  • I have read the Contributing Guide.
  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@andreialecu
Copy link
Contributor

Question: seems like this would invalidate the local cache, and essentially replace the cached packages with an identical copy with a different file name?

Does it explode the git repo storage size, or is git smart enough to see these as renames for push/pull purposes?

I'm concerned of effects to my own repositories which are already pretty huge because of caching the deps, especially in relation with #180

@Larry1123
Copy link
Contributor

@andreialecu it should not invalidate anything already in the cache. Git store objects by their content hash, if the file is changed it has a new hash, if the path is changed it is the same hash, as the content is the same. This change will create new hashes as the timestamp value in the zip has changed. Yarn tries it best to reuse what it in the cache and only update if it would be replacing the file as it is.

@andreialecu
Copy link
Contributor

andreialecu commented Oct 21, 2020

Are the zips the same though or is the timestamp applied to files within the zip, hence changing the binary data itself?

Edit: nevermind, seems like that's what you said. Seems slightly concerning.

@arcanis
Copy link
Member

arcanis commented Oct 21, 2020

As for any bugfix related to the tarball generation, this will cause all packages that would be covered by this change to be regenerated (and the cache to be updated). That said, it's already fairly rare, and will be even more so as the time passes and we fix annoying issues like this one, which affect all packages (rather than a very small subset as is usually the case).

In any case, this isn't a blocker (it'll transparently update the cache), so we can merge once the tests pass.

@andreialecu
Copy link
Contributor

Could the timestamp be applied on unzip/by the virtual filesystem as a custom "hack" around this and not change the zip contents?

@nicolo-ribaudo
Copy link
Contributor

Could the timestamp be applied on unzip/by the virtual filesystem as a custom "hack" around this and not change the zip contents?

No, because yarn can't control the unzipping process of every published packages. Even if I publish with yarn, my users could be using pnpm, npm or others.

@andreialecu
Copy link
Contributor

@nicolo-ribaudo publishing is separate from yarn's internal cache.

I think this can easily be fixed inside yarn pack so that publishing to npm results in proper timestamps, without exploding the local cache for other yarn users.

@arcanis arcanis added the major label Oct 22, 2020
@arcanis arcanis mentioned this pull request Mar 3, 2021
3 tasks
@arcanis arcanis closed this in #2559 Mar 3, 2021
@merceyz merceyz deleted the merceyz/timestamp branch March 3, 2021 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Invalid timestamp in generated archive
5 participants