-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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: allow cache overlap in parallel builds #8592
Conversation
const buildId = | ||
outDir.length > 8 || outDir.includes('/') ? getHash(outDir) : outDir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can use flattenId
here so that we don't have to potentially hash it? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. This makes the path easier to debug 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is an interesting idea. So we could flatten and check that the length isn't too much? But maybe a limit like 16 🤔
My main issue was if there is a chance for the user to set an absolute or relative path out of root. But we could detect that, maybe it isn't even a thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a problem with using flattenId
. We can safely use it with node deps because we know that _
isn't a valid char there. But here if we use it we could collide dist/foo
with dist_foo
. I agree it would be extremely weird to have both, but I don't feel that comfortable with making this a special case. I think we should leave the code as it is now. What do you think @bluwy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, I guess if we still want to, we can pre-replace _
with ____
. My main concern is that getDepsCacheSuffix
is a hot-code path, and potentially calling getHash
many times may not be good for perf. Unless we refactor the optimizer to pre-calculate the cacheDir
.
We could probably still use getHash
if we plan to optimize it later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's merge this now as it is an important fix and leave the optimization for a future PR. Right now the big majority of users won't hit getHash anyways.
Description
We found out why the worker tests were failing after:
We now have deps caches during build that can't be used in parallel. @poyoho manually configured the worker (and assets) playground to have different caches (see c9b185c), so tests are now stable again.
We discussed that we need to implement something here because before building in parallel was possible. This PR implements a possible way forward, adding an id to the build deps cache based on the
outputDir
I think this may be enough. We can't use a general config hash, because it isn't easy to know when a dir could be removed. I don't think users would call vite build in parallel over the same outputDir, but we need to keep an eye to check if this is really enough.
The PR avoids the use of a hash if the dir name is small enough so it is easier to debug for the default
dist
or other dirs likeoutput
. The customcacheDir
s in the playground are no longer needed so they are removed (and this serves as a test for the PR)What is the purpose of this pull request?