-
-
Notifications
You must be signed in to change notification settings - Fork 16.6k
Add yarn-berry3 and yarn-berry4 to fetch and use version 2 yarn.lock files #355053
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
Conversation
975cc57
to
e3d06a8
Compare
Note also the CI failure. |
This is caused by not adding the
|
Hmm seems like a false negative to me. Let's ask the advice of @NixOS/nixpkgs-vet . |
e3d06a8
to
5f0eecf
Compare
Think this recommendation applies here: https://github.com/NixOS/nixpkgs/tree/master/pkgs/by-name#recommendation-for-new-packages-with-multiple-versions |
5f0eecf
to
e2615ea
Compare
Awesome, thanks for the pointer! |
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.
Looks pretty good, though I wouldn't feel comfortable to merge this because I don't know the yarn config set
tings in yarnBerryConfigHook
but they looks pretty reasonable.
76670ba
to
51531b2
Compare
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.
This is a no-go in its current state.
mkdir -p $out | ||
'' | ||
+ lib.optionalString (yarnVersion > 1) '' | ||
yarn install --immutable --mode skip-build |
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.
This will break the FODs almost instantly if Yarn changes anything in how they pull deps, and also is probably not reproducible between platforms when platform-specific dependencies are used.
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.
Valid point. This is partly why there are two yarn versions.
Also this could be said for all external package managers, not just yarn-berry.
I'm uncertain about your point about platform specific dependencies. I'll have to look into 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.
Platform specific dependencies should be made reproducible by explicitly setting supportedArchitectures
. Your point about Yarn breaking how they fetch deps may be valid depending on the upstream stability guarantees. Explicitly picking the version of yarn somewhat mitigates that issue, but it's no guarantee yes. This is the same path taken in pnpm.fetchDeps
#290715
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.
@winterqt what is your reply on the arguments laid out above? Your "request for changes" is blocking the merge of the PR, and this is the only review comment left really open.
86534dd
to
6d5d7f6
Compare
9312a29
to
7a126ea
Compare
7a126ea
to
cf9593e
Compare
$TMP should be univsal for all derivations and should work on darwin as well, if it ever switches to another build-dir (e.g. /nix/) see NixOS#355053 (comment) Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
@emilazy I know this is a controversial PR, but I'd love to here your thoughts on #355053 (comment) |
Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
$TMP should be univsal for all derivations and should work on darwin as well, if it ever switches to another build-dir (e.g. /nix/) see NixOS#355053 (comment) Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
A new argument has been added to fetch-yarn-deps: `yarnVersion` which will default to `1` which uses the original `yarn`. For newer `yarn-berry` `yarn.lock`, one can set the `yarnVersion` to either `3` or `4` which will use yarn-berry3 or yarn-berry4 respectively. The difference is the `yarn.lock` file, which follows a different format, depending on the version. This also adds the corresponding tests. The added support for yarn.lock files > version 1 has been inspired by @szlend from NixOS#254369 (comment) fixes NixOS#254369 Signed-off-by: Florian Brandes <florian.brandes@posteo.de> Co-authored-by: Doron Behar <doron.behar@gmail.com>
Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
cf9593e
to
bad8ab6
Compare
@emilazy I see that there is no activity from you since end of November. I'd still love to get your feedback to my reply above. In the meantime: @SuperSandro2000 could you also have a look at this? I'd like to get this merged and I believe I addressed most of the issues concerning stability and reliability in my comment above |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/how-to-implement-version-2-yarn-lock-package-management/59792/1 |
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.
Sadly, I have to agree with winterqt.
This kind of fetcher is not sustainable and just defers the cost of an actual pure fetcher to the future.
The reason is ultimately that using FODs like this is always a hack, and the only valid use of them is to reduce evaluation cost and avoid Import-From-Derivation for a problem that we could theoretically also solve by generating many small derivations which would match the hashes specified in the lockfile.
If we don't really understand what yarn is doing under the hood to arrive at this hash, it will explode in our faces sooner or later.
I built a prototype to convert the npm registry tar files to zipfiles with hashes matching the ones in the lockfile: https://cyberchaos.dev/yuka/yarn-zip I still need to perform larger scale testing, but it generally seems to produce identical outputs to what is present in my yarn berry cache. Thanks to flokli for partially funding this work. |
#399404 has landed, this can be closed. |
Problem
Currently there is no good way to use newer
yarn.lock
files because they rely onyarn-berry
and cannot be used together with pre-existing tooling likefetch-yarn-deps
.There is an (abandoned? ) project which converts
v1
files tov2
files with some caveats. This does require an internet connection and cannot be used in an isolated builder.So one needs to convert the
yarn.lock
file and commit it tonixpkgs
to be used for an offline cache.There is an issue open (#254369) which discusses some ways around it. The most sophisticated way was by @szlend and this PR builds on top of it to extend to be included in our new
yarnConfigHook
from #318015Implementation of a solution
A new argument has been added to fetch-yarn-deps:
yarnVersion
which will default to1
which uses the originalyarn
.For newer
yarn.lock
files, one can set theyarnVersion
to either3
or4
which will use yarn-berry3 or yarn-berry4 respectively. We need to separate both versions, because they have some small changes in the way the lock file is created.This also adds the corresponding tests.
How to use it
For an implementation see the included
pgadmin4
derivation, which was also my main motivation for this. I had to build a rather ugly and complicated update script before and do a lot of custom work to build the frontend. This is now simplified.In short, add
yarnBerry3ConfigHook
tonativeBuildInputs
andto your derivation.
fixes #254369
Once we agree on the implantation, I'll add the documentation.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.