-
-
Notifications
You must be signed in to change notification settings - Fork 126
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
NodeJS: combined builder and overrides? #276
Comments
Sounds like a reasonable strategy to me.
You could compare the outputs of I also have an idea for an alternative strategy. The reason why we run into cyclic deps issue in the first place, is because we combine steps, which are actually logically separated. Example. Package A's So, instead of combining packages, we could split single packages into several derivations, for each install step like The problem is, that given the graph of all packages individual steps, it might be hard to find out the exact dependencies between them. But maybe it is possible. Even if the automatic detection of the steps dependencies is not 100% acurate, we could provide a simple interface for the user to manipulate this graph slightly to fix issues + useful error messages on what changes might be required to the graph. |
Good trick and it should avoid a ton of extra derivations we'll see what performance is like.
I think this is actually undefined behavior, I couldn't find any docs around it. I'll just presume that any install-script-time dependencies are source-only, which is what the documented npm algorithm I linked seems to implement. So I'll go ahead and try it that way. My branch already works for all my builds but this new way should make it very robust for the old locks as well. |
As we are proceeding with this, I guess we can close this issue? |
After much consideration I think I found the silver bullet for #195. The older lock files don't provide all the peer dep information for packages, and so those builds should have all packages combined into one derivation. Furthermore, cyclic derivations should always be built together in a single derivation, since they could rely on eachother during the
npm run install
step.So I think that the NodeJS builder should make groups of packages, built according to the steps in npm/npm#5919. (copy all in place then build one by one)
If we have a v2 lock file and we have all the peer dep information, we can build individual packages but cyclic deps still need grouping.
Sometimes we'll have multiple versions of the same dependency, in which
case one is picked as canonical (the one that no other dep or only main is
requesting, or the highest version otherwise) and the others are stored as
/$name-$version
.Packages that want the different version then get a symlink.
So for v1 lock files, this will result in node_modules dirs like this:
This is a completely flat structure and it should work 100% for everything.
The only problem is with the overrides, since they are per-package. I think the only way to handle this is to first make the copied node_modules as combinedSrc, and then create the build derivations, running them one by one, using the output of the previous as the source for the next. So we'll sometimes have 1000 levels deep derivations. I don't think Nix will have a problem with that though.
However, if we know beforehand which packages have overrides, we can run the install steps for all packages in a single build and then only do the stepwise folding build for the overrides.
Is there a way to get this information (which packages have overrides) from inside the builder? If not, would it be possible to add that?
@DavHau really interested in your thoughts on this.
The text was updated successfully, but these errors were encountered: