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(windows): fix directory cache regression "expected to end with a trailing slash" #9144

Merged
merged 10 commits into from
Feb 28, 2024

Conversation

paperdave
Copy link
Member

@paperdave paperdave commented Feb 28, 2024

What does this PR do?

Fixes passing the wrong path type to bustDirCache for real this time. In doing so, fixes a lot of other dubious uses of the dir_cache.

I still stand by the aggressive assertion in bustDirCache, despite it's recent issues. It, when properly called, is essential for making sure module resolution is reliable when filesystem races happen. This PR calls that assertion in more places. What I found out from this is we are very inconsistent in weather there is a trailing slash in our cache keys.

A lot of existing code was uncertain on weather it took a trailing slash or not, and it doesnt seem to be documented. For example: this paradox.

image

To fix this, I am removing all cases where we keep the trailing slash on the key for dir_cache's hash table. This was easier than requiring trailing slashes because as fixed old cases, new ones came up that were increasingly harder to get a trailing slash on without copying the entire path into a new buffer.

In favor of making it easy to get a cache key, the normalization will remove it. See Resolver.assertValidCacheKey for details on the assertion. I left a very long comment here, as this fix took much longer than it should.

How did you verify your code works?

  • Added another case to the original test (this issue was a windows only re-regression of Runtime Directory Caching Issue (breaks vite config files) #3216) that now passes.
  • I built a ReleaseSafe copy of bun before the regression, then used that release copy to build bun again successfully.
  • I verified vite no longer regresses
  • I double checked that every caller of bustDirCache is indeed correct.
  • Testing the Mac OS build locally to ensure no regressions of the assertion.
  • Running tests in CI for all platforms to ensure zero regressions across the board.

Copy link
Contributor

github-actions bot commented Feb 28, 2024

@Jarred-Sumner 2 files with test failures on linux-x64-baseline:

View test output

#c2d826fd9cccd1ccc9d1647258f472d365ce247b

Copy link
Contributor

github-actions bot commented Feb 28, 2024

@Jarred-Sumner 1 files with test failures on linux-x64:

View test output

#c2d826fd9cccd1ccc9d1647258f472d365ce247b

Copy link
Contributor

github-actions bot commented Feb 28, 2024

❌🪟 @Jarred-Sumner, there are 21 test regressions on Windows x86_64

  • test\cli\run\env.test.ts
  • test\cli\run\transpiler-cache.test.ts
  • test\cli\run\require-cache.test.ts
  • test\js\bun\dns\resolve-dns.test.ts
  • test\js\bun\http\fetch-file-upload.test.ts
  • test\js\bun\http\bun-server.test.ts
  • test\js\bun\resolve\import-meta.test.js
  • test\js\bun\resolve\resolve.test.ts
  • test\js\bun\shell\shelloutput.test.ts
  • test\js\bun\shell\throw.test.ts
  • test\js\node\dns\node-dns.test.js
  • test\js\node\env-windows.test.ts
  • test\js\node\process\process-args.test.js
  • test\js\node\process\process.test.js
  • test\js\third_party\es-module-lexer\es-module-lexer.test.ts
  • test\js\web\fetch\body-stream.test.ts
  • test\js\web\timers\setTimeout.test.js
  • test\js\web\websocket\websocket.test.js
  • test\js\web\workers\worker.test.ts
  • test\regression\issue\08757.test.ts
  • test\transpiler\runtime-transpiler.test.ts

Full Test Output

Copy link
Contributor

github-actions bot commented Feb 28, 2024

Copy link
Contributor

github-actions bot commented Feb 28, 2024

✅ test failures on bun-darwin-x64 have been resolved.

#c2d826fd9cccd1ccc9d1647258f472d365ce247b

@paperdave
Copy link
Member Author

paperdave commented Feb 28, 2024

New test does not pass and it is confusing why Resolved

@Jarred-Sumner Jarred-Sumner marked this pull request as ready for review February 28, 2024 22:51
@Jarred-Sumner Jarred-Sumner merged commit 360bbb4 into main Feb 28, 2024
18 checks passed
@Jarred-Sumner Jarred-Sumner deleted the dave/win/fix-dir-cache-finale branch February 28, 2024 22:51
paperdave and others added 9 commits March 2, 2024 07:56
this was introduced a couple of commits ago in my random fixes,
where i put the wrong fix to another directory caching bug.

i still stand by the assertion in place despite it causing many people
issues. it's precense will prevent subtle module resolutions failures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants