Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add yarn-berry3 and yarn-berry4 to fetch and use version 2 yarn.lock files #355053
base: master
Are you sure you want to change the base?
Add yarn-berry3 and yarn-berry4 to fetch and use version 2 yarn.lock files #355053
Changes from all commits
1b8744e
6eac70d
aef1535
5e2366d
2ff6888
e9079ea
bad8ab6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 inpnpm.fetchDeps
#290715There 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.
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.
Personally I like how in
pnpm
they put all their hooks under the package itself (e.g.pnpm_9.configHook
).This way you can just:
Just an observation, not necessarily a blocker.
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.
I like this idea. However this would mean a major refactoring of how the
yarn
system works. Right now, you can just usefetchYarnDeps
. With your suggestion, it will mean a refactor of all packages usingyarn
to now useyarn.fetchYarnDeps
.This can of course be done, but IMHO would be too much for this PR. Lets keep it in mind and maybe do this refactor in a separate PR or tracking issue. This is what @doronbehar suggested for the long-term ecosystem for the
src
argument (#355053 (comment))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.
Small correction:
yarn.fetchDeps
:).Furthermore, this suggestion can ease the refactoring: We could make
yarn.fetchDeps
get the new signature withsrc
, and simply makefetchYarnDeps
throw a message likefetchYarnDeps is deprecated in favor of yarn.fetchDeps which accepts an src argument instead of yarnLock argument
. This would allow us to not make the effort of making the same function compatible with both signatures.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.
I mean we could easily just make these as aliases. But agree that this couls be done in another PR.
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, lets target that for another PR
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.
We should add a comment somewhere so that people avoid touching this file/are very careful when doing it. Ideally we'd be able to use JSON with comments...
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 file means that we can never expand our platform support without mass FOD breaks across the tree? That seems very bad. There are already platforms you can boot NixOS on that would be ruled out by this file. At the very least we should try to list every value already used in npm here so that it’s relatively safe to expand in future?
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.
Since JSON and comments don't agree (and we also read the file directly), I added a
Readme.md
to the folderThere 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.
yes
We did add all the relevant platforms, cpu's and libc's (see https://yarnpkg.com/configuration/yarnrc#supportedArchitectures) that are available. The only one missing is
win32
and I don't think we need to add thisThere 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.
If it’s everything Yarn supports then I guess it’s not so bad – assuming we will be able to add additional platforms if Yarn does support more in future without invalidating every FOD?
I do think we should include Windows. You can use Nixpkgs to cross‐build a lot of stuff for Windows today, and people are working on native bring‐up. What’s the harm?
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.
done.
On a sidenode: I did neither have to change the test hashes, nor the hash for the (seriously complex and long) yarn pgadmin offlineCache. (and yes, I obviously invalidated them, re-downloaded the packages and compared the hashes)
So maybe this won't be such a big deal after all
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.
Yes, I think your right. My guess is that adding another platform or compiler will a huge undertaking anyway, so maybe they will publish yet another big version upgrade, which would make it easier for us (e.g.
yarn-berry_5
)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.
Unfortunately, I think I was correct here:
The npm docs imply that the fields are quite freeform; e.g. the
cpu
example usesmips
as an example. That means that this is just a limitation of Yarn’s configuration format rather than a constraint on the actual packages in existence, and there are packages that could change in the future if we add new platforms like RISC‐V.The correct solution here would be to fetch everything independent of platform – theoretically Yarn could add
all
values here to opt in to fetching everything, but writing our own deterministic fetcher would make it easy and also obviate the other reproducibility concerns. If neither of those is an option, perhaps we can find a way to check that nothing in the dependency tree references any platform we haven’t included in this file, and otherwise bail out? If we can’t do even that, then it’d be good if someone could write a script to scan the npm repository and collect every value that is used in practice so that we can include them all from the start and reduce the scope of potential changes.Because we’re adding fundamental packaging infrastructure here that is part of our public interface and will surely be relied upon by out‐of‐tree packages, and changing FOD hashes are very difficult to spot and debug, I don’t think we’ll have the option of just breaking it in the future, unless we can show that the backwards incompatibility would be entirely theoretical in the existing public corpus.
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.
Actually, it is not a limitation of the configuration, as it can be adapted to include everything node supports. https://yarnpkg.com/configuration/yarnrc#supportedArchitectures says:
which states:
So we can include all of those for our
supported-archs
. Adding this, btw, didn't change the cache for pgadmin, either.I looked into how
yarn-berry
does its comparisons (since these values are for the local.yarnrc.toml
config file) and it compares it to upstreamspackage.json
file. This file has the same fields (https://yarnpkg.com/configuration/manifest#cpu) and, more importantly, only thelibc
andos
fields we already have.Yes, writing our own fetcher could be a solution. This will be quite a task to parse the
yarn.lock
file and generate the cache. I really feel uncomftable reinventing the wheel, though. We do have a package manager, which already parses this file and also controlls the specs about our architectures. I don't want to write yet another solution ofyarn2nix
There seems to be an option (
yarn info --all --manifest --json
) which prints all the manifests, but not one had acpu
,libc
oros
field. There is another option (yarn npm info _packageName_ --json
) which prints the upstream manifest and from the quick peek I took, didn't include any of those, either.Totally agree. This is why we added versions here. Not only because upstream has two supported versions (3 and 4), but also because their
yarn.lock
files differ.If there is a situation where either the file format changes, or the supported architectures, I would suggest adding another version (e.g.
yarn-berry_5
oryarn-berry_39
). This will keep all the hashes intakt, be reproducible and still be able to adapt to future changes.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.
The documentation says these values are based on Node
process.{arch,cpu}
which has a known documented set of values - namely platforms supported by nodejs. I wouldn't exactly call it freeform.I don't really have a better idea on how to handle adding new platforms other than invalidating the FOD (by changing
name
) and fixing up all the failed builds, or introducing a new builder version like @gador suggested above.I wonder how
pnpm
managed to avoid this issue. I don't see it handlingsupportedArchitectures
at all.