Various buildNpmPackage fixes

[winterqt]

Description of changes

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.11 Release Notes (or backporting 22.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[winterqt]

@delroth @mweinelt Please test this branch to see if it's fixed your respective issues. Thanks!

[delroth]

@delroth @mweinelt Please test this branch to see if it's fixed your respective issues. Thanks!

On my side everything now Just Works as it should for etherpad-lite. Thank you so much for the quick turnaround!

[mweinelt]

Running into that new check, even though npmDeps is set.

evcc-0.106.3-go-modules> unpacking sources
evcc-0.106.3-go-modules> unpacking source archive /nix/store/7kxszkq3jd6x1dwl1va6n8fvqppsack5-source
evcc-0.106.3-go-modules> source root is source
evcc-0.106.3-go-modules> patching sources
evcc-0.106.3-go-modules> Executing npmConfigHook
evcc-0.106.3-go-modules> Configuring npm
evcc-0.106.3-go-modules> 
evcc-0.106.3-go-modules> ERROR: no dependencies were specified
evcc-0.106.3-go-modules> Hint: set `npmDeps` if using these hooks individually. If this is happening with `buildNpmPackage`, please open an issue.

  npmDeps = fetchNpmDeps {
    inherit src;
    hash = "";
  };

  nativeBuildInputs = [
    nodejs
    npmHooks.npmConfigHook
  ];

I think the config hook runs before the fetcher.

[winterqt]

@delroth Thanks for confirming!

@mweinelt Notice what derivation it's in -- it's the Go modules one, where it wouldn't be set. 😕

I'll get back to you with a solution on how to cleanly do that for now.

[winterqt]

@lilyinstarlight What do you think of 4e730bbe4d567bbb65cc946fdbbadab0f4d99546, as opposed to having a separate npmPruneFlags? Or do you think I should adjust the diagnostic to suggest npmFlags = [ "--legacy-peer-deps" ]?

[lilyinstarlight]

What do you think of 4e730bb, as opposed to having a separate npmPruneFlags? Or do you think I should adjust the diagnostic to suggest npmFlags = [ "--legacy-peer-deps" ]?

I assume --legacy-peer-deps would affect any subcommand that regenerates the package tree, so it might make sense as a general npmFlags option rather than only for ci and prune

But I also still think it would probably be good to otherwise keep both npmFlags and npmInstallFlags on the npm prune command (like you have in 4e730bb) to avoid unexpected differences in behavior when using a flag that should only matter for install-related stuff but still affects prune (e.g. adding another --omit to npmInstallFlags)

How do you feel about that? Keeping the change in 4e730bb but also adjusting the diagnostic to suggest npmFlags = [ "--legacy-peer-deps" ]

[tjni]

Transcribing comments over from #200475.

[tjni]

Nit: I personally like naming this UrlLike to emphasize that it's still a URL, just maybe not a valid one.

[winterqt]

Not sure on this one. I'm talking about URLs like github:NixOS/nixpkgs here, which technically are valid URLs, but... not.

Maybe yours is best, hm.

[winterqt]

@lilyinstarlight Done in 1612525f97d2101963833ba5f1aa1ef7b01df64c.

@tjni How do you feel about the commit progression here, with your changes applied throughout? It took... a fair bit amount of time to do the rebasing required for this, so let me know if anything seems weird.

[tjni]

Looks great! Thank you for addressing my comments.

[winterqt]

My main concern is doing too much in 83d240907a69684436ca82df80675a3b0c81e260, specifically referring to the if to filter change here, where it really doesn't relate to the other changes, but it kind of does since the change of Package::resolved to UrlOrString allows this to happen, mostly. I don't know.

Am I overthinking this? 😅

[tjni]

I think you should do whatever makes you feel best about the change. If it feels unrelated, feel free to pull it out or leave it out. None of my comments ever need to be addressed, I like and can iterate on code all day, but I almost always prefer to commit something instead of nothing.

[reckenrode]

Saw this mentioned on Discourse and thought I would try converting the FoundryVTT flake I maintain over to buildNpmPackage from node2nix. It went mostly smoothly, but I ran into a few issues. One is minor. I think the other was also reported in #189539.

• I ran into trouble with the src. FoundryVTT is distributed as a zip file, so it took some time to figure out what I needed to override to let it unpack properly in fetchNpmDeps.
• FoundryVTT uses a private fork of pdfjs. The source of their fork is available on GitHub, but the package itself is not. I work around this by changing the package.json to reference the GitHub fork, but that doesn’t work with buildNpmPackage. I get the following error and have to use that URL instead of the repo on GitHub.

Error: request to https://codeload.github.com/foundryvtt/pdfjs/tar.gz/2196ae9bcbd8d6a9b0b9c493d0e9f3aca13f2fd9 failed: cache mode is 'only-if-cached' but no cached response is available.

The revised flake is available at the buildNpmPackage-PoC branch of my repo for reference.

[winterqt]

@reckenrode I can't look further into this right now, but as for the second point, can you please test if changing the lockfile is required when using this branch?

[reckenrode]

@reckenrode I can't look further into this right now, but as for the second point, can you please test if changing the lockfile is required when using this branch?

I was was referencing the PR in my flake (github:nixos/nixpkgs/pull/200470/head). That should be the same thing as the branch, shouldn’t it?

[winterqt]

Yup, you're right. (I think.)

I'll take a look when I can and see what's up, on both fronts. Thanks for reporting!

[winterqt]

@reckenrode Please test the latest commits, which fix the issue by working around what might be an npm bug.

I've marked this PR as a draft until I can file an issue with npm about this and note it in the code.

[delroth]

I rebased my etherpad WIP on top of the latest changes here and --fixup-lockfile is failing to parse the package-lock.json:

Validating consistency between /build/source/src/package-lock.json and /nix/store/377pfxym8iy6dgv83a8bmmr9r668m39b-etherpad-lite-1.8.18-npm-deps/package-lock.json
Error: expected value at line 1 column 1

Haven't debugged this yet since I'm fighting with other stuff.

[winterqt]

That's strange -- will look into it when I can.

[winterqt]

Switched to packing with gzip -9 like npm does.

[SuperSandro2000]

Suggested change

	patchShebangs node_modules
	patchShebangs node_modules/.bin

Would that already be enough? Searching through everyfile in the blackhole could take a bit.

[winterqt]

It could be enough, but who knows what files packages add in their install scripts that have hardcoded shebangs...

[SuperSandro2000]

If patchShebangs would be free, we would just run it on every source file and patch everything we can. node_modules is notorious for being very big and with lots of files.

How about we just search for executable files and patch those?

[mweinelt]

Maybe?

find node_modules -executable -exec patchShebangs {} \;

[mweinelt]

We'll get this in without addressing this for now.

[winterqt]

One last sanity check.

@ofborg eval

[winterqt]

Time is something we don't have right now, so let's just land this.