Skip to content

Bump elmPackages #344866

Merged
turboMaCk merged 1 commit intoNixOS:masterfrom
domenkozar:bump-elm-packages
Oct 1, 2024
Merged

Bump elmPackages #344866
turboMaCk merged 1 commit intoNixOS:masterfrom
domenkozar:bump-elm-packages

Conversation

@domenkozar
Copy link
Copy Markdown
Member

Note that elm-verify-examples is still broken.

Fixes #320127 #267454 #304198

@PedroHLC
Copy link
Copy Markdown
Member

PedroHLC commented Sep 27, 2024

NOTE: I'll try running it again! EDIT: Same result.

Result of nixpkgs-review pr 344866 run on x86_64-linux 1

2 packages failed to build:
  • elmPackages.elm-pages
  • elmPackages.elm-verify-examples
12 packages built:
  • elmPackages.create-elm-app
  • elmPackages.elm-analyse
  • elmPackages.elm-coverage
  • elmPackages.elm-doc-preview
  • elmPackages.elm-git-install
  • elmPackages.elm-graphql
  • elmPackages.elm-land
  • elmPackages.elm-language-server
  • elmPackages.elm-live
  • elmPackages.elm-review
  • elmPackages.elm-spa
  • elmPackages.elm-upgrade

@ofborg ofborg bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Sep 27, 2024
@domenkozar
Copy link
Copy Markdown
Member Author

I pushed a fix for elm-pages.

@PedroHLC
Copy link
Copy Markdown
Member

Result of nixpkgs-review pr 344866 run on x86_64-linux 1

1 package failed to build:
  • elmPackages.elm-verify-examples
13 packages built:
  • elmPackages.create-elm-app
  • elmPackages.elm-analyse
  • elmPackages.elm-coverage
  • elmPackages.elm-doc-preview
  • elmPackages.elm-git-install
  • elmPackages.elm-graphql
  • elmPackages.elm-land
  • elmPackages.elm-language-server
  • elmPackages.elm-live
  • elmPackages.elm-pages
  • elmPackages.elm-review
  • elmPackages.elm-spa
  • elmPackages.elm-upgrade

Copy link
Copy Markdown
Member

@PedroHLC PedroHLC left a comment

Choose a reason for hiding this comment

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

Diff LGTM

@PedroHLC
Copy link
Copy Markdown
Member

Actually, elm-pages doesn't run after build:

╰─λ ./results/elmPackages.elm-pages/bin/elm-pages --help
/nix/store/11kh8s407rr06vqb47adra26wmgcgjlh-elm-pages-3.0.16/lib/node_modules/elm-pages/node_modules/rollup/dist/native.js:59
                throw new Error(
                      ^

Error: Cannot find module @rollup/rollup-linux-x64-gnu. npm has a bug related to optional dependencies (https://github.com/npm/cli/issues/4828). Please try `npm i` again after removing both package-lock.json and node_modules directory.
    at requireWithFriendlyError (/nix/store/11kh8s407rr06vqb47adra26wmgcgjlh-elm-pages-3.0.16/lib/node_modules/elm-pages/node_modules/rollup/dist/native.js:59:9)
    at Object.<anonymous> (/nix/store/11kh8s407rr06vqb47adra26wmgcgjlh-elm-pages-3.0.16/lib/node_modules/elm-pages/node_modules/rollup/dist/native.js:68:76)
    ... 3 lines matching cause stack trace ...
    at Module._load (node:internal/modules/cjs/loader:1104:12)
    at cjsLoader (node:internal/modules/esm/translators:346:17)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:286:7)
    at ModuleJob.run (node:internal/modules/esm/module_job:234:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:473:24) {
  [cause]: Error: Cannot find module '@rollup/rollup-linux-x64-gnu'
  Require stack:
  - /nix/store/11kh8s407rr06vqb47adra26wmgcgjlh-elm-pages-3.0.16/lib/node_modules/elm-pages/node_modules/rollup/dist/native.js
      at Module._resolveFilename (node:internal/modules/cjs/loader:1225:15)
      at Module._load (node:internal/modules/cjs/loader:1051:27)
      at Module.require (node:internal/modules/cjs/loader:1311:19)
      at require (node:internal/modules/helpers:179:18)
      at requireWithFriendlyError (/nix/store/11kh8s407rr06vqb47adra26wmgcgjlh-elm-pages-3.0.16/lib/node_modules/elm-pages/node_modules/rollup/dist/native.js:41:10)
      at Object.<anonymous> (/nix/store/11kh8s407rr06vqb47adra26wmgcgjlh-elm-pages-3.0.16/lib/node_modules/elm-pages/node_modules/rollup/dist/native.js:68:76)
      at Module._compile (node:internal/modules/cjs/loader:1469:14)
      at Module._extensions..js (node:internal/modules/cjs/loader:1548:10)
      at Module.load (node:internal/modules/cjs/loader:1288:32)
      at Module._load (node:internal/modules/cjs/loader:1104:12) {
    code: 'MODULE_NOT_FOUND',
    requireStack: [
      '/nix/store/11kh8s407rr06vqb47adra26wmgcgjlh-elm-pages-3.0.16/lib/node_modules/elm-pages/node_modules/rollup/dist/native.js'
    ]
  }
}

@ofborg ofborg bot requested a review from PedroHLC September 27, 2024 14:28
@domenkozar
Copy link
Copy Markdown
Member Author

Seems to be #294183

nativeBuildInputs = (old.nativeBuildInputs or [ ]) ++ [ makeWrapper old.nodejs.pkgs.node-gyp-build ];

preRebuild = ''
sed -i 's/"esbuild": "0\.19\.12"/"esbuild": "0.21.5"/' package.json
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should use substituteInPlace with --replace-fail

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Done.

-c node-composition.nix \
--no-copy-node-env -e ../../../../node-packages/node-env.nix
# well, elm-pages requires two different version of esbuild so we twist it's wrist to only use one
sed -i 's/sources."esbuild-0.19.12"/sources."esbuild-0.21.5"/' node-packages.nix
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure how to do this, since this is a shell script and it doesn't have substituteInPlace.

inherit (pkgs) stdenv lib python2 runCommand writeTextFile writeShellScript;
inherit pkgs nodejs;
libtool = if pkgs.stdenv.hostPlatform.isDarwin then pkgs.cctools or pkgs.darwin.cctools else null;
libtool = if pkgs.stdenv.isDarwin then pkgs.cctools or pkgs.darwin.cctools else null;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why was this changed back?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe because we used my mac to run the generation script. Could probably be reverted?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reverted.

Copy link
Copy Markdown
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

elm-packages should also eventually stop using node2nix but that's something for another PR

Comment on lines +9 to +22
ESBUILD_BINARY_PATH = lib.getExe (
pkgs.esbuild.override {
buildGoModule = args: pkgs.buildGoModule (args // rec {
version = "0.20.2";
src = pkgs.fetchFromGitHub {
owner = "evanw";
repo = "esbuild";
rev = "v${version}";
hash = "sha256-h/Vqwax4B4nehRP9TaYbdixAZdb1hx373dNxNHvDrtY=";
};
vendorHash = "sha256-+BfxCyg0KkDQpHt/wycy/8CTG6YBA/VJvJFhhzUnSiQ=";
});
}
);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder - did you try to just use esbuild that is already available in nixpkgs? I think this might not be that version sensitive.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seemed very version sensitive when we tried. Had to provide the exact version.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

uh that will be a bit painful to maintain. Maybe it would make sense to port these away from node2nix to buildNpmPackage right away to prevent potential regression with future bulk updates.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree on it being painful. A slight condolence is the fact that Elm packages are usually quite slow to release new updates.

I tried porting to buildNpmPackage but got lost. I'd need some hand-holding to get it done.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sure not a blocker. Thanks for giving it a try.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tried again today during NixCon Hack Day: #351767

ESBUILD_BINARY_PATH = lib.getExe (
pkgs.esbuild.override {
buildGoModule = args: pkgs.buildGoModule (args // rec {
version = "0.21.5";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

so I see we even need 2 different version of esbuild. I guess there was an issue with version of ES build when you tried to share the same one?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

correct

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Sep 29, 2024
Copy link
Copy Markdown
Member

@turboMaCk turboMaCk left a comment

Choose a reason for hiding this comment

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

LGTM

@turboMaCk turboMaCk merged commit 5080634 into NixOS:master Oct 1, 2024
@domenkozar
Copy link
Copy Markdown
Member Author

Note that the binary still fails to run because we need to also patch up rollup:

❯ /nix/store/mbbvs7bg3n90gj0fva1zxx5fp70fc9lq-elm-land-0.20.1/bin/elm-land 
/nix/store/mbbvs7bg3n90gj0fva1zxx5fp70fc9lq-elm-land-0.20.1/lib/node_modules/elm-land/node_modules/rollup/dist/native.js:59
		throw new Error(
		      ^

Error: Cannot find module @rollup/rollup-linux-x64-gnu. npm has a bug related to optional dependencies (https://github.com/npm/cli/issues/4828). Please try `npm i` again after removing both package-lock.json and node_modules directory.
    at requireWithFriendlyError (/nix/store/mbbvs7bg3n90gj0fva1zxx5fp70fc9lq-elm-land-0.20.1/lib/node_modules/elm-land/node_modules/rollup/dist/native.js:59:9)
    at Object.<anonymous> (/nix/store/mbbvs7bg3n90gj0fva1zxx5fp70fc9lq-elm-land-0.20.1/lib/node_modules/elm-land/node_modules/rollup/dist/native.js:68:76)
    ... 3 lines matching cause stack trace ...
    at Module._load (node:internal/modules/cjs/loader:1104:12)
    at cjsLoader (node:internal/modules/esm/translators:346:17)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:286:7)
    at ModuleJob.run (node:internal/modules/esm/module_job:234:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:473:24) {
  [cause]: Error: Cannot find module '@rollup/rollup-linux-x64-gnu'
  Require stack:
  - /nix/store/mbbvs7bg3n90gj0fva1zxx5fp70fc9lq-elm-land-0.20.1/lib/node_modules/elm-land/node_modules/rollup/dist/native.js
      at Module._resolveFilename (node:internal/modules/cjs/loader:1225:15)
      at Module._load (node:internal/modules/cjs/loader:1051:27)
      at Module.require (node:internal/modules/cjs/loader:1311:19)
      at require (node:internal/modules/helpers:179:18)
      at requireWithFriendlyError (/nix/store/mbbvs7bg3n90gj0fva1zxx5fp70fc9lq-elm-land-0.20.1/lib/node_modules/elm-land/node_modules/rollup/dist/native.js:41:10)
      at Object.<anonymous> (/nix/store/mbbvs7bg3n90gj0fva1zxx5fp70fc9lq-elm-land-0.20.1/lib/node_modules/elm-land/node_modules/rollup/dist/native.js:68:76)
      at Module._compile (node:internal/modules/cjs/loader:1469:14)
      at Module._extensions..js (node:internal/modules/cjs/loader:1548:10)
      at Module.load (node:internal/modules/cjs/loader:1288:32)
      at Module._load (node:internal/modules/cjs/loader:1104:12) {
    code: 'MODULE_NOT_FOUND',
    requireStack: [
      '/nix/store/mbbvs7bg3n90gj0fva1zxx5fp70fc9lq-elm-land-0.20.1/lib/node_modules/elm-land/node_modules/rollup/dist/native.js'
    ]
  }
}

Node.js v20.17.0

@turboMaCk
Copy link
Copy Markdown
Member

ah right. I also ran into this issue in my quick spike - that's where I gave up. Forgot to test it in this PR.

Anyway lets try fix these issues one by one now when this is merged. I also think we can start decoupling these problematic packages away from node2nix to buildNpmPackage.

@domenkozar
Copy link
Copy Markdown
Member Author

I tried using buildNpmPackage, but build from source is quite complicated as it needs to build parts using elm. Going to try to patch @rollup/wasm-node to be used in place of rollup.

zupo added a commit to teamniteo/nixpkgs that referenced this pull request Feb 15, 2025
Current elm-land in master is actually broken. It does build, but it
doesn't run. This commit rewrites it from node2nix to buildNpmPackage
and adds a test to confirm it runs.

Previus work:
* NixOS#344866
* NixOS#304198
* NixOS#267725

Refs Thaigersprint/thaigersprint-2025#1
@zupo zupo mentioned this pull request Feb 15, 2025
13 tasks
@zupo
Copy link
Copy Markdown
Contributor

zupo commented Feb 15, 2025

I have gotten a bit further with elm-land, it now builds and runs on x86-linux and aarch64-darwin: #382339

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants