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

Replace colons in transform cache filenames #8353

Merged
merged 8 commits into from Apr 30, 2019

Conversation

etr2460
Copy link
Contributor

@etr2460 etr2460 commented Apr 21, 2019

Summary

Fixes #6306 by replacing colons in filenames with the symbol _COLON_

Test plan

Run yarn jest script_transformer.test.js. Let me know if any further testing is required!

@codecov-io
Copy link

codecov-io commented Apr 21, 2019

Codecov Report

Merging #8353 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8353      +/-   ##
==========================================
+ Coverage   62.33%   62.34%   +<.01%     
==========================================
  Files         266      266              
  Lines       10735    10736       +1     
  Branches     2610     2610              
==========================================
+ Hits         6692     6693       +1     
  Misses       3460     3460              
  Partials      583      583
Impacted Files Coverage Δ
packages/jest-transform/src/ScriptTransformer.ts 77.89% <100%> (+0.11%) ⬆️

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 391c4e2...af36665. Read the comment docs.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The hash always contains the filename as well so there it won't collide with file names that really contain _COLON_ right?

@etr2460
Copy link
Contributor Author

etr2460 commented Apr 21, 2019

LGTM. The hash always contains the filename as well so there it won't collide with file names that really contain _COLON_ right?

The hash does contain the rest of the filename too, but technically hashes for the files :.js and _COLON_.js would be indistinguishable from each other. However, since using underscores and all caps like _COLON_ isn't standard JS file naming I think it's an ok solution.

@SimenB
Copy link
Member

SimenB commented Apr 21, 2019

We should hash the original filename, shouldn't we?

@etr2460
Copy link
Contributor Author

etr2460 commented Apr 21, 2019

Ah, I see what you mean. Right now we aren't hashing the filename, only the file contents. I'll make the update to hash the filename as well

@@ -94,6 +94,7 @@ export default class ScriptTransformer {
rootDir: this._config.rootDir,
}),
)
.update(filename)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't necessary because getCacheKey includes the filename already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I thought so as well, my comment was only to seek confirmation ^^

@@ -121,11 +123,11 @@ export default class ScriptTransformer {
// Create sub folders based on the cacheKey to avoid creating one
// directory with many files.
const cacheDir = path.join(baseCacheDir, cacheKey[0] + cacheKey[1]);
const cacheFilenamePrefix = path
.basename(filename, path.extname(filename))
.replace(/:/g, '_COLON_');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is colon really the only character that might cause issues here? Especially when you consider it's possible for a cache to be created on one system and then be used on another (this is what we do at Facebook and I'm sure it's not totally unique).

Stepping back a second, the extension being added to the file seems odd to me in the first place. With your .update(filename) added above, I don't see a reason we can't just drop this entire find/replace and then also stop saving the file with the extname prefix. The cache key has the whole filename in it. That'll do.

@SimenB let me know if you have any thoughts but here is my recommended course of action:

  1. Remove .update(filename) from the custom cache key code path since it has filename available already.
  2. Keep .update(filename) in the built-in cache key code path.
  3. Remove this find/replace and stop using the extname at all.

That should resolve the core issue and other similar issues we don't know about yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is obviously no technical necessity to have the prefix because it's already in the hash (and stripping away most of the path would make it useless for collision avoidance anyway), but I would guess it's there to make debugging / working on cache related things easier?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point. I'm still in favor of killing it if it's causing issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the filename prefix is helpful with debugging cache related things, how about we prefix with just the alphanumeric characters from the filename? That should solve any issues across systems but retain the human friendly context

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scotthovestadt you fine with this too? I think it's a good way to handle it

@etr2460
Copy link
Contributor Author

etr2460 commented Apr 30, 2019

@scotthovestadt any other changes that you'd like me to make here?

Copy link
Contributor

@scotthovestadt scotthovestadt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the PR.

@jeysal
Copy link
Contributor

jeysal commented Apr 30, 2019

@et2460 if you rebase on master then we're good to merge :)

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jest cache file use not allowed characters in windows
6 participants