nodejs: use ninja for build #327653

tie · 2024-07-16T13:21:45Z

Description of changes

This PR contains changes to build Node.js without cross-compilation (see #220204 (comment)) and use ninja for build.

Note that we can’t use --cross-compiling with Ninja without nodejs/gyp-next#185, but we don’t have to because we use an emulator instead. This is based on Buildroot’s 0001-add-qemu-wrapper-support.patch patch, but re-implemented in a manner that hopefully can be upstreamed (see also nodejs/node#53899).

Supersedes

nodejs: fix cross compilation for armv7l #220204 (thanks @Cynerd for working on this!)
nodejs: move configure flags to a separate variable #317109

Fixes

Things done

Add a 👍 reaction to pull requests you find important.

tie · 2024-07-16T23:17:46Z

pkgs/development/web/nodejs/nodejs.nix

-          done
-        fi
-      ''}
+      $AR -cqs $libv8/lib/libv8.a @files


Note: before this change, we had fallback for ar that does not support response files. As far as I know, that is only the case for Darwin’s cctools, but stdenv already uses LLVM for most of the tools on Darwin.

nixpkgs/pkgs/os-specific/darwin/binutils/default.nix

Line 117 in 034f3ee

isCCTools = true; # The fact ld64 is used instead of lld is why this isn’t `isLLVM`.

That is, I think we can assume that ar supports response files (unless there is some edge case I’m missing).

In the future, ideally, we should update v8 package and avoid using nodejs as v8 library provider.

You could set env.AR to "${lib.getBin llvm}/bin/llvm-ar" to play it really safe, but that’s not necessary.

Darwin bintools has been using llvm-ar since 23.11. Going forward, more tools will be switched to their LLVM versions as compatibility and stability improve. None are expected to switch back to their cctools versions.

aduh95

Just tested that on a aarch64-darwin machine:

$ nixpkgs-review pr   --checkout commit   --package nodejs_18   --package nodejs_20   --package nodejs_22   327653

6 packages built:
nodejs_18 nodejs_18.libv8 nodejs_20 nodejs_20.libv8 nodejs_22 nodejs_22.libv8

emilazy · 2024-07-17T09:19:43Z

pkgs/development/web/nodejs/bypass-darwin-xcrun-node16.patch

+                "--show-sdk-platform-path": "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform",
+                "--show-sdk-path": "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk",


How does this work with the build sandbox?

Node.js has two copies of GYP and this change just extends the existing patch to the second GYP copy. This works fine with sandbox, but I'm not really sure how that's supposed to work without sandbox since technically nothing stops the build from accessing these paths.

Anyway, this allows us to drop xcbuild from Node.js native build inputs with minor copy-paste modification to an existing patch. I'm leaving refactoring or documenting this behavior to my future self (e.g. if we add split dev output, we can use fake xcbuild in propagatedNativeBuildInputs instead of patching GYP).

emilazy

Do you know if this fixes the weird double‐compilation test race condition, or is that still yet to be diagnosed?

pkgs/development/web/nodejs/nodejs.nix

tie · 2024-07-17T14:38:07Z

Do you know if this fixes the weird double‐compilation test race condition, or is that still yet to be diagnosed?

Not yet, but I think Node.js had the same issue that they fixed by limiting Make parallelism: nodejs/node#22006 (comment)

We are not using test-ci target since IIRC that never worked in Nix sandbox and requires network access, but we should be able to apply the Nix equivalent of the upstream fix, that is, enableParallelChecking = false.

tie · 2024-07-17T15:27:59Z

Force-pushed to minimize formatting changes (see #327653 (comment)).

tie · 2024-07-17T15:34:50Z

pkgs/development/web/nodejs/nodejs.nix

+    else if platform.isLoongArch64 then
+      "loong64"
+    else
+      throw "unsupported cpu ${platform.uname.processor}";


This is not an idiomatic Nixpkgs pattern and should probably use some fallback for unknown platforms instead of throwing an error. I’m keeping it for now and will address this in subsequent code style PRs.

It's fine to throw if we don't know the platform.

tie · 2024-07-17T17:31:30Z

pkgs/development/web/nodejs/nodejs.nix

+    env = {
+      # Tell ninja to avoid ANSI sequences, otherwise we don’t see build
+      # progress in Nix logs.
+      NINJA = "TERM=dumb ninja";


Not setting TERM=dumb globally because Node.js v18 needs nodejs/node@f76b28f backport.

We should write that into a comment

Hmm, this is unfortunate. I think Nix build environments have enough of a PTY to run these tests, but we’re using TERM=dumb to get Ninja to give us more detailed output. It’d be nice if we could still run those tests, but it’s probably not that important.

These tests run fine unless TERM=dumb is set 😅

Yeah. We could override TERM for just those tests, but it’d be ugly. I’m guessing Ninja has probably already been approached about an alternate way to toggle non‐fancy build output and maybe rejected it. Oh well.

Filed ninja-build/ninja#2475.

tie · 2024-07-17T18:11:39Z

@emilazy, I’ve added a commit that should fix races in check phase.

emilazy · 2024-07-17T22:03:54Z

Thanks! Do you think there’s any chance of splitting off the tests into a separate derivation like you suggested? Maybe that should wait for a second PR…

I can test this on x86_64-darwin if it’s ready.

tie · 2024-07-18T11:00:34Z

I can test this on x86_64-darwin if it’s ready.

Yes, thanks, the PR is ready. I’ve run built the package a few times on aarch64-darwin and didn’t notice any issues. Note: --rebuild/--check flag does not work for pre-v22 versions (yet?), see nodejs/build#3043 (comment) (that is, nodejs_18 and nodejs_20 builds are not reproducible).

Do you think there’s any chance of splitting off the tests into a separate derivation like you suggested? Maybe that should wait for a second PR…

The change set of this PR is complete and I’d like to avoid adding more unrelated changes.

tie · 2024-07-28T20:44:57Z

@emilazy, it’s ready for review and tests, hopefully ofborg-eval error will go away after force push (it’s not caused by changes in Node.js package). Let’s wait for green checkmark on the commit before testing this again, otherwise I’d have to rebase staging branch…

Last time I’ve tested this PR on aarch64-linux, x86_64-linux and aarch64-darwin (with sandbox enabled). I haven’t tested without sandbox on macOS and also x86_64-darwin, @agj reports that there are some issues in the check phase with the latter.

emilazy · 2024-07-28T21:05:56Z

The Hydra macOS builders have the sandbox turned off, so if we want to stop the Node.js packages being so flaky on them we’ll need to figure out what’s going on there.

tie · 2024-08-07T04:35:22Z

Hm, I can’t reproduce build failure on aarch64-darwin with and without Nix sandbox. Perhaps it’s really something specific to x86_64-darwin.

tie · 2024-08-08T02:11:11Z

I’ve installed macOS on my old Intel MacBook, it’ll take some time to build stdenv (oof dual core CPU), but hopefully I’ll be able to test this PR on x86_64-darwin by the end of this week.

tie · 2024-08-10T07:56:00Z

I can reproduce test-fs-readv and test-fs-readv-sync test failures on x86_64-darwin (see #327653 (comment)) without Nix sandbox. Not sure what the actual issue is, my completely unfounded guess is that it is caused by different SDK versions being used for x86_64-darwin and aarch64-darwin. Unfortunately, currently I don’t have the compute power on MacBook or time to set up Hackintosh to find the root cause.

That said, these tests were failing before changes in this PR (they were disabled before and we revert that in d7f46fb), so I’m adding them to the list of disabled tests for x86_64-darwin.

emilazy · 2024-08-10T11:25:31Z

I’ve installed macOS on my old Intel MacBook, it’ll take some time to build stdenv (oof dual core CPU), but hopefully I’ll be able to test this PR on x86_64-darwin by the end of this week.

Sometimes this isn’t possible due to conflicts, but I would generally recommend basing your staging PRs on master, so that you can test them without rebuilding the world. Of course, that mostly helps when it’s staging because it causes too many rebuilds, rather than because it relates to some actual stdenv change.

Going to try these builds out now (…rebased onto master), thanks for enduring with this PR!

emilazy · 2024-08-10T11:43:05Z

pkgs/development/web/nodejs/nodejs.nix

+    # See https://github.com/nodejs/node/issues/22006
+    enableParallelChecking = false;


It seems like upstream hasn’t seen any issues here for years and closed this issue as a result. I’m wondering if it really is an upstream issue, but if so, it’d be good to ping the issue with example logs to hopefully get it reopened (and maybe even fixed).

emilazy · 2024-08-10T13:28:18Z

Here we go again

Do you want the application “node” to accept incoming network connections? Do you want the application “node” to accept incoming network connections? Do you want the application “node” to accept incoming network connections? Do you want the application “node” to accept

emilazy · 2024-08-10T14:14:04Z

Successfully built nodejs_{18,20,22} on x86_64-darwin with sandbox = false. Doing the slim builds and checking aarch64-darwin and then I’ll merge.

emilazy · 2024-08-10T15:37:00Z

Failed on the aarch64-darwin community builder with --option sandbox relaxed; a lot of errors like this:

../../test/cctest/test_inspector_socket.cc:379: Failure
Expected equality of these values:
  0
  err
    Which is: -16

[  FAILED  ] InspectorSocketTest.HostIPv6NonRoutableDual (0 ms)
[ RUN      ] InspectorSocketTest.HostIPv4InSquareBrackets
../../test/cctest/test_inspector_socket.cc:361: Failure
Expected equality of these values:
  0
  uv_listen(reinterpret_cast<uv_stream_t*>(&server), 1, on_new_connection)
    Which is: -48

[-AI] async    0x101bd7060
[R--] tcp      0x101bd6d88

The full list:

[  FAILED  ] 40 tests, listed below:
[  FAILED  ] InspectorSocketTest.ReadsAndWritesInspectorMessage
[  FAILED  ] InspectorSocketTest.BufferEdgeCases
[  FAILED  ] InspectorSocketTest.AcceptsRequestInSeveralWrites
[  FAILED  ] InspectorSocketTest.ExtraTextBeforeRequest
[  FAILED  ] InspectorSocketTest.RequestWithoutKey
[  FAILED  ] InspectorSocketTest.KillsConnectionOnProtocolViolation
[  FAILED  ] InspectorSocketTest.CanStopReadingFromInspector
[  FAILED  ] InspectorSocketTest.CloseDoesNotNotifyReadCallback
[  FAILED  ] InspectorSocketTest.CloseWorksWithoutReadEnabled
[  FAILED  ] InspectorSocketTest.ReportsHttpGet
[  FAILED  ] InspectorSocketTest.HandshakeCanBeCanceled
[  FAILED  ] InspectorSocketTest.GetThenHandshake
[  FAILED  ] InspectorSocketTest.WriteBeforeHandshake
[  FAILED  ] InspectorSocketTest.CleanupSocketAfterEOF
[  FAILED  ] InspectorSocketTest.EOFBeforeHandshake
[  FAILED  ] InspectorSocketTest.Send1Mb
[  FAILED  ] InspectorSocketTest.ErrorCleansUpTheSocket
[  FAILED  ] InspectorSocketTest.NoCloseResponseFromClient
[  FAILED  ] InspectorSocketTest.HostCheckedForGET
[  FAILED  ] InspectorSocketTest.HostCheckedForUPGRADE
[  FAILED  ] InspectorSocketTest.HostIPChecked
[  FAILED  ] InspectorSocketTest.HostNegativeIPChecked
[  FAILED  ] InspectorSocketTest.HostIpOctetOutOfIntRangeChecked
[  FAILED  ] InspectorSocketTest.HostIpOctetFarOutOfIntRangeChecked
[  FAILED  ] InspectorSocketTest.HostIpEmptyOctetStartChecked
[  FAILED  ] InspectorSocketTest.HostIpEmptyOctetMidChecked
[  FAILED  ] InspectorSocketTest.HostIpEmptyOctetEndChecked
[  FAILED  ] InspectorSocketTest.HostIpTooFewOctetsChecked
[  FAILED  ] InspectorSocketTest.HostIpTooManyOctetsChecked
[  FAILED  ] InspectorSocketTest.HostIpInvalidOctalOctetStartChecked
[  FAILED  ] InspectorSocketTest.HostIpInvalidOctalOctetMidChecked
[  FAILED  ] InspectorSocketTest.HostIpInvalidOctalOctetEndChecked
[  FAILED  ] InspectorSocketTest.HostIpLeadingZeroStartChecked
[  FAILED  ] InspectorSocketTest.HostIpLeadingZeroMidChecked
[  FAILED  ] InspectorSocketTest.HostIpLeadingZeroEndChecked
[  FAILED  ] InspectorSocketTest.HostIPNonRoutable
[  FAILED  ] InspectorSocketTest.HostIPv6NonRoutable
[  FAILED  ] InspectorSocketTest.HostIPv6NonRoutableDual
[  FAILED  ] InspectorSocketTest.HostIPv4InSquareBrackets
[  FAILED  ] InspectorSocketTest.HostIPv6InvalidAbbreviation

Running again with the sandbox off.

emilazy · 2024-08-10T16:14:20Z

Uh, actually I think that was with --option sandbox false; I got my builds mixed up. Trying with the sandbox on instead.

emilazy · 2024-08-10T16:30:46Z

The slim variants build fine on x86_64-darwin with sandbox disabled, as expected.

emilazy · 2024-08-10T17:14:14Z

The slim variants build fine on aarch64-darwin with sandbox = relaxed. Maybe it’s something about tests interfering when run in parallel with the sandbox disabled. Trying with -j1.

tie · 2024-08-10T17:49:08Z

Uh, actually I think that was with --option sandbox false; I got my builds mixed up. Trying with the sandbox on instead.

Does Nix actually use --option sandbox false from command line? I though this setting has no effect unless set from nix.conf (and also needs launchctl kickstart -k for Nix daemon).

emilazy · 2024-08-10T17:54:58Z

It does as long as you are in trusted-users, which you shouldn’t be (it’s a trivial root escalation vulnerability), but this is on the community builders.

(Corollary: You can use sudo to do it otherwise.)

emilazy · 2024-08-10T18:37:09Z

Okay, both Node.js 18 variants went through on aaarch64-darwin with the sandbox off and -j1. If you think this is ready to merge, I’m satisfied to do so as soon as I confirm that the default Node.js version also builds on there.

I do worry that the potential hint of test flakiness when builds are run in parallel (a common theme with loopback networking tests) will mean further Hydra build failures and result in us having to turn tests off on Darwin again, but I’m willing to chance it. (We could at least just turn those specific tests off if it ends up being necessary rather than the whole suite.)

emilazy

It succeeded. This PR has gone on for long enough. Let’s ship it!

wolfgangwalther · 2024-08-25T19:46:27Z

postgresql.pkgs.plv8 is broken on staging after this PR (result of bisect).

Build postgresql.pkgs.plv8.tests.smoke and have this error:

2024-08-25 16:12:14.274 UTC [23] ERROR:  could not load library "/nix/store/6942cgs90vlz4qrvy8rzpm000rrlssba-postgresql-and-plugins-15.7/lib/plv8-3.2.2.so": /nix/store/6942cgs90vlz4qrvy8rzpm000rrlssba-postgresql-and-plugins-15.7/lib/plv8-3.2.2.so: undefined symbol: _ZNK2v85Value14IsFloat64ArrayEv

Any idea why?

tie · 2024-08-25T22:33:37Z

Well, we have an outdated v8 package in Nixpkgs and #147506 introduced a libv8 output for Node.js package with v8 static library.

I haven’t really touched libv8 part in this PR aside from removing special case for $AR that doesn’t support response files (see #327653 (comment)), but looks like I’ve missed that these object files are actually under completely different paths when built with Ninja (line 349 in particular does not match any files).

nixpkgs/pkgs/development/web/nodejs/nodejs.nix

Lines 346 to 351 in 7cef6cd

    
                 # assemble a static v8 library and put it in the 'libv8' output 
        
                 mkdir -p $libv8/lib 
        
                 pushd out/Release/obj 
        
                 find . -path "./torque_*/**/*.o" -or -path "./v8*/**/*.o" | sort -u >files 
        
                 $AR -cqs $libv8/lib/libv8.a @files 
        
                 popd

This is no longer required because upstream merged: NixOS/nixpkgs#327653

github-actions bot added the 6.topic: nodejs label Jul 16, 2024

ofborg bot requested review from cillianderoiste and aduh95 July 16, 2024 14:46

ofborg bot added 10.rebuild-darwin: 501+ 10.rebuild-darwin: 501-1000 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500 labels Jul 16, 2024

tie force-pushed the nodejs-ninja branch from 1899610 to 92ca419 Compare July 16, 2024 22:40

tie commented Jul 16, 2024

View reviewed changes

tie force-pushed the nodejs-ninja branch 3 times, most recently from f299a8e to 711c32a Compare July 17, 2024 05:26

aduh95 approved these changes Jul 17, 2024

View reviewed changes

emilazy reviewed Jul 17, 2024

View reviewed changes

emilazy requested changes Jul 17, 2024

View reviewed changes

pkgs/development/web/nodejs/nodejs.nix Show resolved Hide resolved

tie force-pushed the nodejs-ninja branch from 711c32a to 589c7c9 Compare July 17, 2024 15:24

tie commented Jul 17, 2024

View reviewed changes

tie marked this pull request as ready for review July 17, 2024 15:37

tie commented Jul 17, 2024

View reviewed changes

ofborg bot requested a review from aduh95 July 17, 2024 17:45

tie mentioned this pull request Jul 17, 2024

build: support setting an emulator from configure script nodejs/node#53899

Open

tie requested a review from Cynerd July 18, 2024 07:07

Cynerd approved these changes Jul 18, 2024

View reviewed changes

This was referenced Jul 18, 2024

lib/systems: set gcc.arch = "armv6kz", gcc.fpu = "vfpv2" for raspberryPi #327778

Merged

Build failure: nodejs cross compiling for raspberry pi armv6l #319036

Closed

nodejs: fix tests and disable parallel checking

d7f46fb

tie force-pushed the nodejs-ninja branch from 3dadfa5 to d7f46fb Compare July 28, 2024 20:35

nodejs: disable failing tests on x86_64-darwin

31d2381

emilazy reviewed Aug 10, 2024

View reviewed changes

emilazy approved these changes Aug 10, 2024

View reviewed changes

emilazy merged commit 2677781 into NixOS:staging Aug 10, 2024
26 of 27 checks passed

tie deleted the nodejs-ninja branch August 13, 2024 03:35

tie mentioned this pull request Aug 13, 2024

Node.js always builds as if cross-compiling #321847

Closed

emilazy mentioned this pull request Aug 19, 2024

Nixpkgs committers list #321665

Open

wolfgangwalther mentioned this pull request Aug 25, 2024

treewide: fix pg_config / postgresql headers moved to dev output #336994

Merged

15 tasks

tie mentioned this pull request Aug 25, 2024

nodejs: fix libv8 object files list #337360

Merged

13 tasks

Cynerd added a commit to Cynerd/nixos-personal that referenced this pull request Oct 2, 2024

pkgs: remove nodejs patches

fdb4d84

This is no longer required because upstream merged: NixOS/nixpkgs#327653

kazikcz mentioned this pull request Nov 11, 2024

Build failure: zigbee2mqtt (buildNpmPackage cross compilation) #355165

Open

		+ "--show-sdk-platform-path": "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform",
		+ "--show-sdk-path": "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk",

		# See https://github.com/nodejs/node/issues/22006
		enableParallelChecking = false;

nodejs: use ninja for build #327653

nodejs: use ninja for build #327653

Conversation

tie commented Jul 16, 2024 • edited Loading

Description of changes

Things done

tie Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduh95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilazy left a comment

Choose a reason for hiding this comment

tie commented Jul 17, 2024

tie commented Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tie commented Jul 17, 2024

emilazy commented Jul 17, 2024

tie commented Jul 18, 2024

tie commented Jul 28, 2024 • edited Loading

emilazy commented Jul 28, 2024

tie commented Aug 7, 2024

tie commented Aug 8, 2024

tie commented Aug 10, 2024 • edited Loading

emilazy commented Aug 10, 2024

Choose a reason for hiding this comment

emilazy commented Aug 10, 2024

emilazy commented Aug 10, 2024

emilazy commented Aug 10, 2024

emilazy commented Aug 10, 2024

emilazy commented Aug 10, 2024

emilazy commented Aug 10, 2024

tie commented Aug 10, 2024 • edited Loading

emilazy commented Aug 10, 2024 • edited Loading

emilazy commented Aug 10, 2024

emilazy left a comment

Choose a reason for hiding this comment

wolfgangwalther commented Aug 25, 2024

tie commented Aug 25, 2024

tie commented Jul 16, 2024 •

edited

Loading

tie Jul 16, 2024 •

edited

Loading

tie commented Jul 17, 2024 •

edited

Loading

tie commented Jul 28, 2024 •

edited

Loading

tie commented Aug 10, 2024 •

edited

Loading

tie commented Aug 10, 2024 •

edited

Loading

emilazy commented Aug 10, 2024 •

edited

Loading