Skip to content
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

Node package-lock v2 #246

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

wmertens
Copy link
Contributor

Putting this in a separate PR from #195 to split up discussions.

Copy link
Member

@DavHau DavHau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for separating this out. This makes it easier to integrate your valuable work.

Comment on lines 49 to 58
else if nodes ? "package-lock.json"
then let
# Wish there was a way to get the version without reading a 2MB file
lockJson = getLockJson path;
lockVersion = lockJson.lockfileVersion or 0;
in
if lockVersion == 1
then ["package-lock"]
else if lockVersion == 2
then ["package-lock-v2"]
else ["package-json"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of the translators attribute is to list all possible translators.
If there is a package-lock.json and a yarn.lock, then both translators should be listed.

Also I think the new translator should probably be re-named to package-lock-v3.
The v2 file format just contains definitions for v1 and v3 parsers. There isn't really a v2 parser.
See https://docs.npmjs.com/cli/v8/configuring-npm/package-lock-json#lockfileversion

Therefore if lock file version is 2, both package-lock and package-lock-v3 translators should be listed.
dream2nix will then pick the first on of these by default, so if we want v3 to be the preferred logic, it should be the first item in the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to make the v3 parser call the v1 parser if the version is wrong. Then the discoverer doesn't need to parse the json twice and the package-json impure translator can still use old nodejs versions. I'll give that a shot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better for UX and debugging purposes if we could show the user specifically which translator implementation was used.

Actually parsing the json twice can already be prevented by making use of the tree object which is given to the discoverer. tree is created by lib.prepareSourceTree and presents a directory tree as an attribute set. The attribute set also contains the json parsed content of each file. Because both discoverer and translator use the same tree object, json files will only be parsed once.

Instead of fromJSON the discoverer should just use (tree.getNodeFromPath "/rel/path/to/package.json").jsonContent.

Is it really necessary to have the impure package json translator work with older nodejs versions? Couldn't we just fix the version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good tips, thanks, I'll work with those!

Is it really necessary to have the impure package json translator work with older nodejs versions? Couldn't we just fix the version?

I don't want to force a download of a specific version, but maybe I can check if the version of the nodejs pkg is >= 16 and otherwise use v18.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DavHau So, the pure symlink builder needs to have all peer deps and all circular deps for each dep, and neither package-lock v1 nor yarn v1 have that information.

The sure only way around that is to build the node_modules by first copying all sources into one place, and then running builds for every package. This is inefficient, with easily over 1000 dependencies sometimes. It's also tricky, clashing dependencies have to be detected and moved down into the tree.

The current builder+translator combo doesn't quite do that and fails in some cases (documented elsewhere).

For simplicity, I'd like to remove support for npm v1 and yarn v1 lockfiles. Then the pure builder has all the correct information for building a pure tree of symlinks. The disadvantage is that some projects will first need an impure npm step.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For simplicity

If you isolate your features into a new builder/translator pair, I don't see how removing existing builders/translators simplifies things for you. We only need an interface to allow the user to chose between builders translators (which already exists).

I'd like to remove support for npm v1 and yarn v1 lockfiles

Users are currently depending on these modules. Removing that causes a lot of friction as it is unlikely that the new builders/translators will be bug free from the beginning. It will take some time to stabilize the new approach, and for that period, the old modules will still be useful.

I'd suggest the following upgrade path:

  • add new logic as new translator/builder including an example under /examples on how to use it. (we can merge this before being fully stable, as it is an optional feature)
  • generate an index of the most popular 5000 nodejs packages (similar to dream2nix-npm). I'm happy to do take that task once we merged your new transaltor/builder.
  • build the 5000 packages and fix bugs.
  • once stable, make the new translator/builder the default

@wmertens
Copy link
Contributor Author

wmertens commented Aug 11, 2022

I think the test error may be because I'm adding source info for the main package? The reason for that is to be able to pass metadata like hasInstallScripts, and I can only do that via source, right?

My next optimization will be to split the build into

  • a pure module unpack with bin symlinks which doesn't call any scripts (thoughts on getting around Don't unpack sources into X-extracted packages #214 ?)
  • then a built module if hasInstallScripts is true, temporarily adding pure-source cycle members to the build
  • then cyclic packages that combine the previous by copying

This way, if the node version changes, it only rebuilds packages with install scripts.

@DavHau
Copy link
Member

DavHau commented Aug 12, 2022

I noticed that a failed jsonschema validation did not lead to an error in the example tests. This is fixed in 9331fb1.

You can see the schema that the dream-lock is validated against in src/specifications/dream-lock-schema.json.

Instead of adding information to the sources, you can use the subsystmAttrs field to add extra data. This will end up in _subsystem in the final dream-lock and is passed as subsystemAttrs to the builders.

This way, if the node version changes, it only rebuilds packages with install scripts.

Sounds like a nice optimization. But I'd suggest that we first integrate the simplest possible version of the new build logic. Probably as a separate builder. Then test this against many packages. Once we have confidence in it, we can make it the default. That might be valuable even without further optimizations.

- discovers peer dependencies correctly
- adds metadata like os, dev and hasInstallScripts to sources
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/yarn-plugnplay-and-direnv-packaging/19759/26

@xanderio
Copy link

Support for package-lock v2 and v3 in dream2nix would remove a major hurdle for me in adopting dream2nix. Is there anything that I can help with to get this merged?

in
if lockVersion == 1
then ["package-lock"]
else if lockVersion == 2 || lockVersion == 3
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have spent a bit of time trying to understand why dream2nix main doesn'twork and seems like it is because it doesn't support lockVersion 2. I would have much preferred for dream2nix to just bail out with "unsupported lock file version". I would have earned quite some time and this PR is the opportunity to do so for unknown/unsupported lockVersions

@teto
Copy link

teto commented Jul 18, 2023

I was wondering why dream2nix wouldnt work with my project (lockfile version 3) until I stumbled on this. I am trying this out but nix can't find the git revision. I've been trying to run nix flake update (2.16) but it couldn't find mach-nix, the following fixed this:

-      url = "mach-nix";                                                                       
+      url = "github:DavHau/mach-nix"; 

then I tried to use my fork https://github.com/teto/dream2nix/tree/node-pkg-v2 on my project, which failed with

         at /nix/store/mc6avkywi5473yylvyh64fcl62kdhlgx-source/e2e-candidate/flake.nix:49:6:

           48|   in
           49|     (dream2nix.lib.makeFlakeOutputs {
             |      ^
           50|       pname = "toto";

       error: function 'makeFlakeOutputs' called with unexpected argument 'projects'

@wmertens would you mind rebasing this please ?

@wmertens
Copy link
Contributor Author

@teto I'm kinda burnt out on this, maybe I'll revisit this but right now I have too many other projects.
I did notice that nixpkgs now has buildNpmPackage, not sure if that's helpful?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants