buildNpmPackage: incomplete caching when a dep with same name is repeated in the lockfile

[lucasew]

Describe the bug

A clear and concise description of what the bug is.

Steps To Reproduce

Steps to reproduce the behavior:

1. Try to build bruno (I will show my partial derivation right below)
2. See that it fails with ENOTCACHED for js-yaml

Expected behavior

Build not failing at npm install time

Screenshots

If applicable, add screenshots to help explain your problem.

Additional context

Code for bruno (would be stored in pkgs/by-name/br/bruno/package.nix

{ lib

, fetchFromGitHub
, buildNpmPackage
, nix-update-script
, electron_25-bin
}:

buildNpmPackage rec {
  pname = "bruno";
  version = "0.20.0";

  src = fetchFromGitHub {
    owner = "usebruno";
    repo = "bruno";
    rev = "master";
    hash = "sha256-jwFzvOQhUDDK/5BK2rKdTB6xuFkvzQnGBTTXIifajUE=";
    # rev = "v${version}";
    # hash = "sha256-NaA7WO/DfETnFEDKRdojcZgSgNrNTbFFIffSxXxbBG4=";
  };

  buildInputs = [
    electron_25-bin
  ];

  npmDepsHash = "sha256-13gHlz+MjPRHiy/fvauSxoHVHJ7pULLAL9/r77ZLYWk=";
  # npmDepsHash = "sha256-s/BqHfpZJu1kB8FCXcPTyUCeduaFbt0Tuk959YL4mvA=";
  npmWorkspace = "packages/bruno-electron";
  # npmPackFlags = [ "--ignore-scripts" ];

  # installPhase = ''
  #   runHook preInstall
  #   mkdir -p "$out/bin"
  #   cp -R opt $out
  #   cp -R "usr/share" "$out/share"
  #   ln -s "$out/opt/Bruno/bruno" "$out/bin/bruno"
  #   chmod -R g-w "$out"
  #   runHook postInstall
  # '';

  postFixup = ''
    substituteInPlace "$out/share/applications/bruno.desktop" \
      --replace "/opt/Bruno/bruno" "$out/bin/bruno"
  '';

  passthru.updateScript = nix-update-script { };

  meta = with lib; {
    description = "Open-source IDE For exploring and testing APIs.";
    homepage = "https://www.usebruno.com";
    license = licenses.mit;
    maintainers = with maintainers; [ water-sucks lucasew ];
    platforms = [ "x86_64-linux" ];
  };
}

Logs of the derivation:

unpacking sources
unpacking source archive /nix/store/ba8cq54na1ayg5f40lgbdvp21q170r9r-source
source root is source
patching sources
Executing npmConfigHook
Configuring npm
Validating consistency between /build/source/package-lock.json and /nix/store/fzmdc4kw06yrrd2bz124s83idd8hfmx2-bruno-0.20.0-npm-deps/package-lock.json
Installing dependencies
npm ERR! code ENOTCACHED
npm ERR! request to https://registry.npmjs.org/js-yaml failed: cache mode is 'only-if-cached' but no cached response is available.

npm ERR! Log files were not written due to an error writing to the directory: /nix/store/fzmdc4kw06yrrd2bz124s83idd8hfmx2-bruno-0.20.0-npm-deps/_logs
npm ERR! You can rerun the command with `--loglevel=verbose` to see the logs in your terminal

ERROR: npm failed to install dependencies

Here are a few things you can try, depending on the error:
1. Set `makeCacheWritable = true`
  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.
2. Set `npmFlags = [ "--legacy-peer-deps" ]`

Note that js-yaml is referenced 6 times in the lockfile.

I tested with a few versions from 0.18 to master. Same issue.

Notify maintainers

@water-sucks as also a maintainer of bruno
@winterqt from git blame
@sternenseemann from git blame
@lilyinstarlight from git blame

Metadata

Please run nix-shell -p nix-info --run "nix-info -m" and paste the result.

[user@system:~]$ nix-shell -p nix-info --run "nix-info -m"
output here

[lilyinstarlight]

Okay I thought I'd responded to someone about this but it was @johnrichardrinehart

Basically the upstream lockfile is lacking information and needs to be regenerated (and not a bug on our side), since many registry deps are missing integrity and resolved fields (which was a known bug in npm in the past but the npm folks decided older lockfiles shouldn't be fixed up automatically...)

A tool like https://github.com/jeslie0/npm-lockfile-fix can fix it, or just deleting both package-lock.json and all node_modules directories and having npm regenerate it should do it too

[lucasew]

I ran your tool on a parallel branch then pointed that PR to that branch.

The tool ran successfully and added integrity and resolved without issues but the same error still happens on build.

I started the hashing from scratch commenting all out and letting nix show the right one then set and run again until everything was fetched.

I submit these changes to the linked PR with the exact commit I used in my fork. Not much idea about what to try next.

[lilyinstarlight]

Hmm, alright. I'll take a look at your PR a bit later today then. Thanks!

[felschr]

Thanks to @lilyinstarlight I just noticed some cases that weren't handled by npm-lockfile-fix yet and fixed them in jeslie0/npm-lockfile-fix#3
@lucasew It looks like those fixes should work for your PR, too.

[lucasew]

Thanks to @lilyinstarlight I just noticed some cases that weren't handled by npm-lockfile-fix yet and fixed them in jeslie0/npm-lockfile-fix#3 @lucasew It looks like those fixes should work for your PR, too.

Yayy progress!!! That PR solves the issue.

I just pushed the new version.

[CMCDragonkai]

Is a long term solution to just integrate https://github.com/jeslie0/npm-lockfile-fix into buildNpmPackage?

[CMCDragonkai]

I got my project working by deleting the lock file and node_modules. Thanks for the tip @lilyinstarlight.

But long term it should be better to have this be sorted automatically. I actually don't understand why the package-lock.json is so non-deterministic.

[lucasew]

Is a long term solution to just integrate https://github.com/jeslie0/npm-lockfile-fix into buildNpmPackage?

I think that if we can count on it will always generate a reproducible result it's already ready to be integrated here.

And would be hella useful here because packaging npm packages kinda suck.

If the lockfile already has the exact version that would be fetched I guess there is nothing so big to go wrong.

But ideally, these lock changes should be sent upstream, and even more ideally, npm solve this bug.

Maybe to reduce closure the utility should be implemented using Javascript directly so the derivation could use the same node using in the builder to also run the script.

IDK if we can count on nodejs fetch yet to make it only using the standard library tho.

[lucasew]

It seems that nodejs 16 (oldest available in nixpkgs) has the stuff needed:

• crypto: for sha512
• https: to fetch stuff
• fs: to read and write files

And the extra: it's possible to parallize downloads and hashing using promises if desirable.

[lilyinstarlight]

Is a long term solution to just integrate https://github.com/jeslie0/npm-lockfile-fix into buildNpmPackage?

We have a different solution for that in the works. I'm rewriting the lockfile fixup code to allow for it, but basically we'll be able to inject that data in without having to fix the lockfile manually and then vendor a whole fixed lockfile. This will also be related to implementing importCargoLock-like functionality for npm

But long term it should be better to have this be sorted automatically. I actually don't understand why the package-lock.json is so non-deterministic.

Because the npm project does not always consider non-reproducibility a bug 🫠

But ideally, these lock changes should be sent upstream, and even more ideally, npm solve this bug.

We've tried :(

Maybe to reduce closure the utility should be implemented using Javascript directly so the derivation could use the same node using in the builder to also run the script.

We could do this, but given @winterqt and I are the ones developing it, we felt more comfortable with it in Rust

And the extra: it's possible to parallize downloads and hashing using promises if desirable.

It already is parallel using rayon

[felschr]

Just linking the relevant upstream issues: npm/cli#4263, npm/cli#4460 npm/cli#6301

[lucasew]

Our workaround for bruno to fix this is in production for many releases already. Closing.