npmHooks.npmConfigHook: add npmRoot option

[winterqt]

Description of changes

Only, what, 48 hours late?

Told you it was simple! Would appreciate if someone would test, then I'll document and we can land. Thanks!

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/)
• 23.05 Release Notes (or backporting 22.11 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
• Fits CONTRIBUTING.md.

[winterqt]

...I forgot to pushd/popd...

@ajs124 Can I just add a commit that implements #230092 in this PR so we can test the implementation here?

[mweinelt]

Feel free to pick #229953.

[ajs124]

@ajs124 Can I just add a commit that implements #230092 in this PR so we can test the implementation here?

sure, feel free

[mweinelt]

After setting

  npmRoot = "ui";

on the navidrome derivation I am greeted by

navidrome> unpacking sources
navidrome> unpacking source archive /nix/store/f51j4vbggdg3a7bq8xr8ksf3a5xs0wvv-source
navidrome> source root is source
navidrome> patching sources
navidrome> Executing npmConfigHook
navidrome> Configuring npm
navidrome> Validating consistency between /build/source/ui/package-lock.json and /nix/store/y2splh2dkp37gjkn77hqkscbsd9x89cj-npm-deps/package-lock.json
navidrome> Fixing lockfile
navidrome> Installing dependencies
navidrome> npm ERR! code EUSAGE
navidrome> npm ERR! 
navidrome> npm ERR! The `npm ci` command can only install with an existing package-lock.json or
navidrome> npm ERR! npm-shrinkwrap.json with lockfileVersion >= 1. Run an install with npm@5 or
navidrome> npm ERR! later to generate a package-lock.json file, then try again.
navidrome> npm ERR! 
navidrome> npm ERR! Clean install a project
navidrome> npm ERR! 
navidrome> npm ERR! Usage:
navidrome> npm ERR! npm ci
navidrome> npm ERR! 
navidrome> npm ERR! Options:
navidrome> npm ERR! [-S|--save|--no-save|--save-prod|--save-dev|--save-optional|--save-peer|--save-bundle]
navidrome> npm ERR! [-E|--save-exact] [-g|--global]
navidrome> npm ERR! [--install-strategy <hoisted|nested|shallow|linked>] [--legacy-bundling]
navidrome> npm ERR! [--global-style] [--omit <dev|optional|peer> [--omit <dev|optional|peer> ...]]
navidrome> npm ERR! [--strict-peer-deps] [--no-package-lock] [--foreground-scripts]
navidrome> npm ERR! [--ignore-scripts] [--no-audit] [--no-bin-links] [--no-fund] [--dry-run]
navidrome> npm ERR! [-w|--workspace <workspace-name> [-w|--workspace <workspace-name> ...]]
navidrome> npm ERR! [-ws|--workspaces] [--include-workspace-root] [--install-links]
navidrome> npm ERR! 
navidrome> npm ERR! aliases: clean-install, ic, install-clean, isntall-clean
navidrome> npm ERR! 
navidrome> npm ERR! Run "npm help ci" for more info
navidrome> 
navidrome> npm ERR! Log files were not written due to an error writing to the directory: /nix/store/y2splh2dkp37gjkn77hqkscbsd9x89cj-npm-deps/_logs
navidrome> npm ERR! You can rerun the command with `--loglevel=verbose` to see the logs in your terminal
navidrome> 
navidrome> ERROR: npm failed to install dependencies
navidrome> 
navidrome> Here are a few things you can try, depending on the error:
navidrome> 1. Set `makeCacheWritable = true`
navidrome>   Note that this won't help if npm is complaining about not being able to write to the logs directory -- look above that for the actual error.
navidrome> 2. Set `npmFlags = [ "--legacy-peer-deps" ]`
navidrome> 

[winterqt]

Just needs documentation, but implementation should be good.

Some notes:

1. Do we want src = "${src}/foo" or sourceRoot to be the only way to integrate fetchNpmDeps into this (which either only supports directory sources, or has bad UX.), or should we add support for something else?
2. @ajs124 Any clue why (presumably?) the same Webpack and Node version combination suddenly require the legacy OpenSSL flag for botamusique?

[lilyinstarlight]

Do the other hooks need pushd/popd too?

1. Do we want src = "${src}/foo" or sourceRoot to be the only way to integrate fetchNpmDeps into this (which either only supports directory sources, or has bad UX.), or should we add support for something else?

I think we should allow fetchNpmDeps to accept an npmRoot as well so that people can just inherit npmRoot; in their fetcher. I suppose it doesn't need pushd/popd for that and can just specify an absolute path to the package-lock.json

[ajs124]

@ajs124 Any clue why (presumably?) the same Webpack and Node version combination suddenly require the legacy OpenSSL flag for botamusique?

It's not the same version. Pre #230092, there's a nodejs = pkgs.nodejs_14; in there. That was my motivation for #230092, moving from a soon to me marked EOL nodejs release to a supported one.

[winterqt]

Do the other hooks need pushd/popd too?

I don't think so, all this is meant to do is allow the installation of dependencies (which is arguably the most annoying part without the hook), just like the Rust hooks.

It's not the same version. Pre #230092, there's a nodejs = pkgs.nodejs_14; in there. That was my motivation for #230092, moving from a soon to me marked EOL nodejs release to a supported one.

Got it, I was looking for something like that, but guess I missed it. Thanks.

[lilyinstarlight]

I don't think so, all this is meant to do is allow the installation of dependencies (which is arguably the most annoying part without the hook), just like the Rust hooks.

Yeah, but some people may find benefit in using it with buildNpmPackage since sourceRoot has pretty poor UX

I'm not gonna let it block this PR, though, since this PR blocks other updates. We'll investigate adding npmRoot to the other hooks and fetchNpmDeps in a follow-up

[lilyinstarlight]

That last push was to rebase to fix merge conflicts

Once ofborg is good, I'll merge since this looks good now and unblocks other work