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

Projects
None yet
6 participants
@etr2460
Copy link
Contributor

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!

@etr2460 etr2460 force-pushed the etr2460:fix-cache-file-colons branch from 92b06d9 to 051c575 Apr 21, 2019

@codecov-io

This comment has been minimized.

Copy link

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.

@SimenB

SimenB approved these changes Apr 21, 2019

Copy link
Collaborator

left a comment

Thanks!

@SimenB SimenB requested review from jeysal and scotthovestadt Apr 21, 2019

@jeysal

jeysal approved these changes Apr 21, 2019

Copy link
Collaborator

left a comment

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

@etr2460

This comment has been minimized.

Copy link
Contributor Author

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.

Erik Ritter
@SimenB

This comment has been minimized.

Copy link
Collaborator

commented Apr 21, 2019

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

@etr2460

This comment has been minimized.

Copy link
Contributor Author

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)

This comment has been minimized.

Copy link
@scotthovestadt

scotthovestadt Apr 22, 2019

Contributor

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

This comment has been minimized.

Copy link
@jeysal

jeysal Apr 22, 2019

Collaborator

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_');

This comment has been minimized.

Copy link
@scotthovestadt

scotthovestadt Apr 22, 2019

Contributor

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.

This comment has been minimized.

Copy link
@jeysal

jeysal Apr 22, 2019

Collaborator

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?

This comment has been minimized.

Copy link
@scotthovestadt

scotthovestadt Apr 22, 2019

Contributor

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

This comment has been minimized.

Copy link
@etr2460

etr2460 Apr 22, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@jeysal

jeysal Apr 25, 2019

Collaborator

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

@etr2460 etr2460 force-pushed the etr2460:fix-cache-file-colons branch from 6902c63 to 0737f6b Apr 22, 2019

@jeysal

jeysal approved these changes Apr 22, 2019

@jeysal jeysal requested a review from scotthovestadt Apr 22, 2019

etr2460 added some commits Apr 22, 2019

@etr2460

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

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

@scotthovestadt
Copy link
Contributor

left a comment

LGTM! Thanks for the PR.

@jeysal

This comment has been minimized.

Copy link
Collaborator

commented Apr 30, 2019

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

@jeysal jeysal merged commit d387bcf into facebook:master Apr 30, 2019

11 checks passed

ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-11 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
facebook.jest #20190430.10 succeeded
Details

@etr2460 etr2460 deleted the etr2460:fix-cache-file-colons branch May 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.