fix(core) Pass mutex file to yarn classic when installing classic git dependencies#5373
fix(core) Pass mutex file to yarn classic when installing classic git dependencies#5373rally25rs wants to merge 3 commits into
Conversation
| yarn1MutexFilename: { | ||
| description: `When repositories that use yarn1 are included as dependencies, this file is used as a mutex to avoid concurrency issues.`, | ||
| type: SettingsType.ABSOLUTE_PATH, | ||
| default: `./.yarn/.yarn-mutex`, | ||
| }, |
There was a problem hiding this comment.
If we're going to set the mutex for Yarn Classic I don't see the point of making it configurable.
There was a problem hiding this comment.
I just wasn't sure this would always be a writable path. If someone has a locked down user in a docker image with least privilege, it seemed possible they might not have write access to .yarn.
I was just trying to make it flexible. I had also noticed that the global and cache paths are configurable, so thought maybe all the .yarn/* paths were... though looking at it again, it seems stuff like .yarn/releases, /sdks, /plugins etc are not configurable, so maybe yarn just doesn't really work if .yarn isn't a writable path.
I'd be fine removing this config option, but just wasn't sure how configurable to keep this 🤷
| // package.json, which I don't want to do to keep the codebase | ||
| // clean (even if it has a slight perf cost when cloning v1 repos) | ||
| const install = await execUtils.pipevp(`yarn`, [`install`], {cwd, env, stdin, stdout, stderr, end: execUtils.EndStrategy.ErrorCode}); | ||
| const install = await execUtils.pipevp(`yarn`, [`install`, `--mutex`, mutexPath], {cwd, env, stdin, stdout, stderr, end: execUtils.EndStrategy.ErrorCode}); |
There was a problem hiding this comment.
Could we instead limit it to one classic install at a time?
There was a problem hiding this comment.
Line 216 has a limit
const MAX_PREPARE_CONCURRENCY = 2;
const prepareLimit = pLimit(MAX_PREPARE_CONCURRENCY);
which could just be set to 1 to avoid this problem, but it isn't separated by package manager type, so setting it to 1 would have a negative perf impact on all installs that use git repos. I wasn't sure the tradeoff was worth it.
There was a problem hiding this comment.
We can add another limit for just Yarn classic installs.
There was a problem hiding this comment.
That would be a nice way to handle this concurrency issue without having to pass the mutex. I'd certainly prefer to handle the limit in yarn modern.
It seemed like it might be awkward to structure that file to have different limits, since the whole thing is wrapped in one pLimit.
I wasn't sure how make the pLimit more conditional like that... code suggestions welcome.
| "@yarnpkg/cli": minor | ||
| "@yarnpkg/core": minor |
There was a problem hiding this comment.
The commit title says fix which makes this a patch but here it's marked as minor, it can't be both.
There was a problem hiding this comment.
I wasn't aware of the convention. I clicked through some other open PRs and the first 3 other fix( titled PRs also just update patch.
I don't remember seeing suggested title prefixes in the contributing guide. Is there a documented recommended list?
What's the problem this PR addresses?
fixes #5349
Due to yarn classic not being concurrency-safe by default, there was potential for a cache conflict if multiple yarn classic github dependencies were installed in parallel.
For example 2 github repos that both use yarn classic
if they happen to install in parallel, it could lead to a yarn classic
ENOENTerror during theFetching packagesstage....
How did you fix it?
yarnrcconfig optionyarn1MutexFilenamethat defaults to./.yarn/.yarn-mutex.installclassic deps,--mutex file:...option is passed, using the path from theyarn1MutexFilenamesetting....
Checklist