Convert elm-review from node2nix to buildNpmPackage by zupo · Pull Request #351767 · NixOS/nixpkgs

zupo · 2024-10-27T22:28:39Z

Things done

Add a 👍 reaction to pull requests you find important.

zupo · 2024-10-27T22:42:08Z

@hsjobeki: here's the diff we worked on today. As I was preparing the commit I noticed some elm packages have already been ported to buildNpmPackage, so I have re-written our stuff to look like the existing packages. Funnily enough, now I get a different error:

error: builder for '/nix/store/n04nqsnyr1qmz37475bz4hxlmwqgvd5m-elm-review-2.12.0.drv' failed with exit code 1;
       last 23 log lines:
       > Running phase: unpackPhase
       > unpacking source archive /nix/store/ry807l4bg0s77ymsx01baw7rqvi6d2bc-source
       > source root is source
       > Running phase: patchPhase
       > Executing npmConfigHook
       > Configuring npm
       > Validating consistency between /private/tmp/nix-build-elm-review-2.12.0.drv-0/source/package-lock.json and /nix/store/agijrxfmrmqcasr4aydq46m53cnqg3bs-elm-review-2.12.0-npm-deps/package-lock.json
       > \Installing dependencies
       > npm error code EJSONPARSE
       > npm error path /private/tmp/nix-build-elm-review-2.12.0.drv-0/source/package.json
       > npm error JSON.parse Expected double-quoted property name in JSON at position 879 while parsing near ".../run.sh record)\",\n  },\n  \"repository\": {..."
       > npm error JSON.parse Failed to parse JSON data.
       > npm error JSON.parse Note: package.json must be actual JSON, not just JavaScript.

Which is strange, as I don't see what the problem is with package.json, neither does my JSON validator.

pkgs/development/compilers/elm/packages/elm-review/default.nix

dotlambda · 2024-10-28T02:27:05Z

pkgs/development/compilers/elm/packages/node/node-packages.json

  "create-elm-app",
  "elm-optimize-level-2",
  "elm-pages",
-  "elm-review",


Use ./pkgs/development/node-packages/remove-attr.py elm-review to also remove it from node-packages.nix.
And you should add an entry to pkgs/development/node-packages/aliases.nix.

I ran this, and it doesn't seem to do anything. No output, no error, no changes.

I checked pkgs/development/node-packages/aliases.nix and I don't see other elm packages listed there (besides elm-test)? Why does elm-review need to be listed and others not? Should I add others as well?

We want nodePackages.elm-review to remain an alias after we remove it.

I ran this, and it doesn't seem to do anything.

Maybe the entry still needs to exist in node-packages.json for the script to work? I'm sure the Python script isn't too hard to read.

dotlambda · 2024-10-28T02:27:12Z

pkgs/development/compilers/elm/packages/elm-review/default.nix

+  dontNpmBuild = true;
+
+  meta = {
+    changelog = "https://github.com/jfmengels/node-elm-review/blob/v${src.rev}/CHANGELOG.md";


Suggested change

changelog = "https://github.com/jfmengels/node-elm-review/blob/v${src.rev}/CHANGELOG.md";

changelog = "https://github.com/jfmengels/node-elm-review/blob/${src.rev}/CHANGELOG.md";

This packages seems to use the v prefix: https://github.com/jfmengels/node-elm-review/releases

https://github.com/jfmengels/node-elm-review/blob/2.12.0/CHANGELOG.md returns 404.

that is correct. ignore this recommendation

src.rev already contains the leading v

pkgs/development/compilers/elm/packages/elm-review/default.nix

turboMaCk

Please squash the commits into a single one with usual commit message. After that I think we can merge this change.

Thanks a lot for revisiting it.

zupo · 2024-10-29T09:09:36Z

Suggested fixes applied.

@hsjobeki the error is now the same as during NixCon hackday:

       > Running phase: unpackPhase
       > unpacking source archive /nix/store/ry807l4bg0s77ymsx01baw7rqvi6d2bc-source
       > source root is source
       > Running phase: patchPhase
       > Executing npmConfigHook
       > Configuring npm
       > Validating consistency between /private/tmp/nix-build-elm-review-2.12.0.drv-0/source/package-lock.json and /nix/store/agijrxfmrmqcasr4aydq46m53cnqg3bs-elm-review-2.12.0-npm-deps/package-lock.json
       > Installing dependencies
       > npm warn deprecated @types/chalk@2.2.0: This is a stub types definition for chalk (https://github.com/chalk/chalk). chalk provides its own type definitions, so you don't need @types/chalk installed!
       > npm error code ENOTCACHED
       > npm error request to https://registry.npmjs.org/yocto-queue failed: cache mode is 'only-if-cached' but no cached response is available.
       > npm error Log files were not written due to an error writing to the directory: /nix/store/agijrxfmrmqcasr4aydq46m53cnqg3bs-elm-review-2.12.0-npm-deps/_logs
       > npm error You can rerun the command with `--loglevel=verbose` to see the logs in your terminal
       >
       > ERROR: npm failed to install dependencies

zupo · 2024-10-29T09:11:16Z

Please squash the commits into a single one with usual commit message. After that I think we can merge this change.

Thanks a lot for revisiting it.

I'd love it for this to be the case, but the package doesn't build (yet!). I'll squash when it does.

Also, I'm happy to apply the things I learn in this PR to other elm packages too.

hsjobeki · 2024-10-29T18:26:36Z

Suggested fixes applied.

@hsjobeki the error is now the same as during NixCon hackday:

       > Running phase: unpackPhase
       > unpacking source archive /nix/store/ry807l4bg0s77ymsx01baw7rqvi6d2bc-source
       > source root is source
       > Running phase: patchPhase
       > Executing npmConfigHook
       > Configuring npm
       > Validating consistency between /private/tmp/nix-build-elm-review-2.12.0.drv-0/source/package-lock.json and /nix/store/agijrxfmrmqcasr4aydq46m53cnqg3bs-elm-review-2.12.0-npm-deps/package-lock.json
       > Installing dependencies
       > npm warn deprecated @types/chalk@2.2.0: This is a stub types definition for chalk (https://github.com/chalk/chalk). chalk provides its own type definitions, so you don't need @types/chalk installed!
       > npm error code ENOTCACHED
       > npm error request to https://registry.npmjs.org/yocto-queue failed: cache mode is 'only-if-cached' but no cached response is available.
       > npm error Log files were not written due to an error writing to the directory: /nix/store/agijrxfmrmqcasr4aydq46m53cnqg3bs-elm-review-2.12.0-npm-deps/_logs
       > npm error You can rerun the command with `--loglevel=verbose` to see the logs in your terminal
       >
       > ERROR: npm failed to install dependencies

I'll try to debug this tomorrow.

dotlambda · 2024-10-29T18:49:06Z

npm error request to https://registry.npmjs.org/yocto-queue failed: cache mode is 'only-if-cached' but no cached response is available.

The yocto-queue entry in upstream's package-lock.json is missing values for resolved and integrity. Upstream needs to regenerate their lock file.

zupo · 2024-10-29T21:09:20Z

npm error request to https://registry.npmjs.org/yocto-queue failed: cache mode is 'only-if-cached' but no cached response is available.

The yocto-queue entry in upstream's package-lock.json is missing values for resolved and integrity. Upstream needs to regenerate their lock file.

Is there a workaround if they don't do it?

dotlambda · 2024-10-29T21:32:01Z

Is there a workaround if they don't do it?

Vendor the lock file.

hsjobeki · 2024-10-31T08:21:41Z

@zupo I validated the ca-cache. It seems not all packages are installed. Maybe due to missing integrity hashes as mentioned. (Didn't dive into the rust code there.)

Workaround without vendoring in my mind: Create a fixed output derivation for node_modules?

You could try to install dependencies with npm natively and pass the generated node_modules to your package.
Trusting npm itself to be reproducible and properly lock the dependencies.
You can then pass this as npmDeps. Since you dont need any other logic you can disable build and configure phases and copy the node_modules manually (maybe symlinking would also work?)

But this might be potentially unstable? But could also be fine. There are cases where the registry randomly gives u other reponses, in which case both methods should fail.

Both solutions have pro and cons so it might be up for the node ecosystem maintainers to decide what they prefer.
I guess @dotlambda already pointed that he would prefer a vendored lockfile.

dotlambda · 2024-10-31T14:17:34Z

The first step is opening an upstream issue or PR for regenerating the lock file.

turboMaCk · 2024-10-31T17:12:46Z

I would first check if it's not fixed already in HEAD upstream. Using commit like this or later jfmengels/node-elm-review@de899d2

zupo · 2025-02-15T11:50:56Z

Work continues in #382294.

Convert elm-review from node2nix to buildNpmPackage

084da85

zupo mentioned this pull request Oct 27, 2024

Bump elmPackages #344866

Merged

dotlambda reviewed Oct 28, 2024

View reviewed changes

pkgs/development/compilers/elm/packages/elm-review/default.nix Outdated Show resolved Hide resolved

dotlambda reviewed Oct 28, 2024

View reviewed changes

ofborg bot requested a review from turboMaCk October 28, 2024 06:09

turboMaCk approved these changes Oct 29, 2024

View reviewed changes

apply fixes suggested on the PR

af4690b

zupo force-pushed the elm_buildNpmPackage/elm-review branch from fd650e2 to af4690b Compare October 29, 2024 09:09

ofborg bot requested a review from turboMaCk October 29, 2024 12:01

zupo mentioned this pull request Feb 15, 2025

elmReview: 2.12.0 -> 2.13.1 #382294

Merged

13 tasks

zupo closed this Feb 15, 2025

	changelog = "https://github.com/jfmengels/node-elm-review/blob/v${src.rev}/CHANGELOG.md";
	changelog = "https://github.com/jfmengels/node-elm-review/blob/${src.rev}/CHANGELOG.md";

Uh oh!

Conversation

zupo commented Oct 27, 2024

Things done

Uh oh!

zupo commented Oct 27, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

turboMaCk left a comment

Choose a reason for hiding this comment

Uh oh!

zupo commented Oct 29, 2024

Uh oh!

zupo commented Oct 29, 2024

Uh oh!

hsjobeki commented Oct 29, 2024

Uh oh!

dotlambda commented Oct 29, 2024

Uh oh!

zupo commented Oct 29, 2024

Uh oh!

dotlambda commented Oct 29, 2024

Uh oh!

hsjobeki commented Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotlambda commented Oct 31, 2024

Uh oh!

turboMaCk commented Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zupo commented Feb 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hsjobeki commented Oct 31, 2024 •

edited

Loading

turboMaCk commented Oct 31, 2024 •

edited

Loading