nodejs: fix cross compilation for armv7l by Cynerd · Pull Request #220204 · NixOS/nixpkgs

Cynerd · 2023-03-08T19:46:49Z

Description of changes

The cross build for armv7l requires build to be on i686 (32bit in general). This ensures that and thus fixes build for armv7l.

I am not happy with this solution but I am unable to figure out how to fix cross compilation for amv7l in better way than this.

Things done

alyssais

If we truly need to force a 32-bit build system, I think a cleaner way to do it would be to do it in the callPackage invocation. Then we could just do something like

nodejs = (if stdenv.buildPlatform.isx86_64 && stdenv.is32bit then pkgsi686Linux else pkgs).callPackage …

which is nice because then there's one less package set involved in the actual expression, and it prevents adding 64-bit dependencies as well by mistake.

Either way, this needs a comment explaining what's going on, with a link to documentation or reference to the source code so people can understand what's going on, and where to check to see if it can be removed.

pkgs/development/web/nodejs/nodejs.nix

Cynerd · 2023-03-28T15:05:21Z

If we truly need to force a 32-bit build system, I think a cleaner way to do it would be to do it in the callPackage invocation. Then we could just do something like

I tweaked it a bit. It is probably reasonable requirement.

Either way, this needs a comment explaining what's going on, with a link to documentation or reference to the source code so people can understand what's going on, and where to check to see if it can be removed.

I added comment but I am unable to find that this is actually required in the documentation. At the same time build really fails with missing 32bit stubs and thus it for sure tries to use 32bit build somewhere inside. The upstream builds it on Ubuntu and thus they might expect that 32bit libraries are just available while that is not the case with nixpkgs.

alyssais

LGTM but I'd really like there to be an upstream issue about this. They shouldn't be passing -m32 to the native compiler just because they're cross compiling for 32 bit.

lilyinstarlight · 2023-06-29T17:13:49Z

I only just now saw this, but I do wonder if it may also be fixed by #238377?

Cynerd · 2023-06-30T10:06:13Z

@lilyinstarlight I just tested it, and it doesn't help my issue. The issue here is with cross-compilation from amd64 to 32-bit systems. Node requires bitness to match it seems, and for that reason, we need to build it not in an amd64 environment but instead in an x86 one.

I suspect that, especially on new Macs that do not have 32-bit arm support (if I remember correctly) it won't even be possible to cross-compile Node to armv7l.

Cynerd · 2023-06-30T14:48:52Z

LGTM but I'd really like there to be an upstream issue about this. They shouldn't be passing -m32 to the native compiler just because they're cross compiling for 32 bit.

I create an issue upstream but I am not expecting much nodejs/node#48612. This is a common issue I saw with other bytecode compilers (for example luajit).

tie · 2023-07-08T01:05:14Z

FWIW I think this is the relevant code that sets -m32 when cross-compiling in your case.
https://github.com/nodejs/node/blob/3205b1936a5c3c21b0c8c568a57b0d2bada366b1/tools/v8_gypfiles/toolchain.gypi#L583-L601

host_cxx_is_biarch is defined at https://github.com/nodejs/node/blob/3205b1936a5c3c21b0c8c568a57b0d2bada366b1/tools/v8_gypfiles/toolchain.gypi#L107-L118

That is, it assumes that x86(-64) (and some other systems) have bi-arch capable toolchains (probably for a reason, see below, but maybe not, this is technical decision is really weird).

With the following patch, we successfully get a meaningful error (instead of vague compilation failure due to the missing headers) that leads us to https://github.com/nodejs/node/blob/3205b1936a5c3c21b0c8c568a57b0d2bada366b1/deps/v8/include/v8config.h#L863-L892 (or deps/v8/src/base/build_config.h for v18)

Details

diff --git a/tools/v8_gypfiles/toolchain.gypi b/tools/v8_gypfiles/toolchain.gypi
index 26c6866fa9..658ebd6e86 100644
--- a/tools/v8_gypfiles/toolchain.gypi
+++ b/tools/v8_gypfiles/toolchain.gypi
@@ -109,7 +109,7 @@
       host_arch=="s390x" or \
       clang==1', {
       'variables': {
-        'host_cxx_is_biarch%': 1,
+        'host_cxx_is_biarch%': 0,
        },
      }, {
       'variables': {

Target architecture arm is only supported on arm and ia32 host

So this seems like an issue with v8 rather than Node.js. At this point I don’t really want to dive deeper into this rabbit hole 🥲

pkgs/top-level/all-packages.nix

lilyinstarlight · 2023-07-08T10:09:26Z

@Cynerd Could you rebase and resolve merge conflicts on this?

If this is what we have to do, I'm mostly alright with this, but it is rather unfortunate

tie · 2024-06-30T05:43:14Z

Yes, should be targeting staging given the number of rebuilds.

See https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#rebasing-between-branches-ie-from-master-to-staging

Cynerd · 2024-07-02T08:42:11Z

Rebase and retargeting is done (I am not sure if GitHub sends an email about it).

tie · 2024-07-02T14:37:12Z

pkgs/development/web/nodejs/add-qemu-wrapper-support.patch

Are there any changes from the buildroot patch? Can we use fetchpatch2 to fetch the patch?

No changes in the end. I will switch it to fetchpatch.

tie · 2024-07-02T14:38:27Z

pkgs/development/web/nodejs/nodejs.nix

I’m not really a fan of conditional patches. Can we apply the patch unconditionally (and replace MAYBE_WRAPPER with an empty string if we are not using an emulator)?

Ok.. in my eyes, it makes it a bit more complex, but ok. There is also an option to just always use a wrapper because, for the native build, it will work as well.

tie · 2024-07-02T14:43:52Z

pkgs/development/web/nodejs/nodejs.nix

Hm, I think a symlink should be enough? IIRC emulator contains additional arguments iff canExecute. Is it the case here?

nixpkgs/lib/systems/default.nix

Lines 285 to 295 in 5d49679

if pkgs.stdenv.hostPlatform.canExecute final

then "${pkgs.runtimeShell} -c '\"$@\"' --"

else if final.isWindows

then "${wine}/bin/wine${optionalString (final.parsed.cpu.bits == 64) "64"}"

else if final.isLinux && pkgs.stdenv.hostPlatform.isLinux && final.qemuArch != null

then "${qemu-user}/bin/qemu-${final.qemuArch}"

else if final.isWasi

then "${pkgs.wasmtime}/bin/wasmtime"

else if final.isMmix

then "${pkgs.mmixware}/bin/mmix"

else null;

Ah, OK.

nix-repl> (lib.systems.elaborate "x86_64-linux").canExecute (lib.systems.elaborate "i686-linux") true

Yeah for i686 it needs to be a script.

FWIW I’ve opened PR #324071 to make an emulator a simple executable path in native case.

With that it should be possible to use only just symlink 👍

Can we proceed with this PR and update it just once #324071 is merged. It should be later only a minor change in postPath. What do you think?

tie · 2024-07-02T14:45:24Z

pkgs/development/web/nodejs/nodejs.nix

Are we using $NIX_CC instead of $NIX_CC_FOR_BUILD in this case? If we don’t need C compiler for build platform, perhaps it makes sense to omit depsBuildBuild entirely if isCrossBitness?

Yes it should be possible to just drop depsBuildBuild in case of isCrossBitness.

Would it even make sense to always use this magic for cross-compilation and not just when bitness differed? The issue with isCrossBitness is that as far as I can see, this is only extraction for the Nix-supported platforms, but as you linked, v8 actually supports only limited build platforms not actually matching bitness.

Would it even make sense to always use this magic for cross-compilation and not just when bitness differed?

Yes, sounds reasonable to me. Always using an emulator instead of cross-compiling should make Node.js build faster, see also #321847.

It is now converted to build always with the patch and with wrapper.

I tested it on amd64 and cross build from amd64 to armv7.

aduh95 · 2024-07-05T14:40:19Z

This needs a rebase.

The v8 library supports only limited number of build platforms based on the host architecture. The rule there is the bitness (the mixing of 32bit and 64bit architectures seems to be in most cases disallowed). Thus this adds usage of the emulator of the host platform and builds tools for that.

tie · 2024-07-15T01:27:21Z

pkgs/development/web/nodejs/nodejs.nix

+    patches = patches ++ [
+      (fetchpatch2 {
+        name = "add-qemu-wrapper-support.patch";
+        url = "https://github.com/buildroot/buildroot/raw/ecf060a2ff93626fb5fbe1c52b8d42b6ee0fbb5b/package/nodejs/nodejs-src/0001-add-qemu-wrapper-support.patch";


GitHub repository is a mirror, the official Git repository is at https://gitlab.com/buildroot.org/buildroot.

Suggested change

url = "https://github.com/buildroot/buildroot/raw/ecf060a2ff93626fb5fbe1c52b8d42b6ee0fbb5b/package/nodejs/nodejs-src/0001-add-qemu-wrapper-support.patch";

url = "https://gitlab.com/buildroot.org/buildroot/-/raw/ecf060a2ff93626fb5fbe1c52b8d42b6ee0fbb5b/package/nodejs/nodejs-src/0001-add-qemu-wrapper-support.patch";

tie · 2024-07-15T02:18:24Z

pkgs/development/web/nodejs/nodejs.nix

      find . -path "./torque_*/**/*.o" -or -path "./v8*/**/*.o" | sort -u >files
      ${if stdenv.buildPlatform.isGnu then ''
-        ar -cqs $libv8/lib/libv8.a @files
+        $AR -cqs $libv8/lib/libv8.a @files


Hm, I’m not sure why this line is conditional. I thought Nixpkgs bintools wrapper handled response files, but maybe that’s not the case here for some reason? I’ll have to double-check that.

tie · 2024-07-15T22:31:07Z

@Cynerd, I’ve made a few minor adjustments based on this PR and re-implemented buildroot patch in 42fa83e (0001-nodejs-fix-cross-compilation-for-armv7l.patch) so that the emulator trick is upstreamable. What do you think?

Cynerd · 2024-07-18T06:55:31Z

@Cynerd, I’ve made a few minor adjustments based on this PR and re-implemented buildroot patch in 42fa83e (0001-nodejs-fix-cross-compilation-for-armv7l.patch) so that the emulator trick is upstreamable. What do you think?

I am ok with that. I tried to compile it and ti seems to work so I would go with your solution.

tie · 2024-07-24T08:40:12Z

Note for reviewers: #327653 contains commits from this PR (with slight modifications, see above) and goes further on top of these changes to use ninja for build.

@Cynerd, would it be OK to close this PR (since you’ve approved ninja PR)?

Cynerd · 2024-07-25T07:00:16Z

@tie Yes. This can be closed.

ofborg bot added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Mar 8, 2023

ofborg bot requested review from cillianderoiste, cko, gilligan and marsam March 8, 2023 20:14

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 Mar 8, 2023

alyssais requested changes Mar 19, 2023

View reviewed changes

alyssais reviewed Mar 19, 2023

View reviewed changes

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

Cynerd force-pushed the nodejs-amrv7l branch from ae5eea3 to 0ef6d8b Compare March 28, 2023 15:03

Cynerd requested a review from alyssais April 25, 2023 12:55

Cynerd force-pushed the nodejs-amrv7l branch from 0ef6d8b to 339be33 Compare April 25, 2023 13:36

alyssais approved these changes May 19, 2023

View reviewed changes

alyssais added the 6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment label May 19, 2023

tie reviewed Jul 8, 2023

View reviewed changes

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved

tie approved these changes Jul 8, 2023

View reviewed changes

lilyinstarlight added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 8, 2023

Cynerd force-pushed the nodejs-amrv7l branch from 339be33 to 6b51601 Compare July 10, 2023 08:06

github-actions bot removed the 6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment label Jul 10, 2023

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 10, 2023

Cynerd changed the base branch from master to staging July 1, 2024 07:59

Cynerd force-pushed the nodejs-amrv7l branch from acfd75d to 2566ac7 Compare July 1, 2024 08:02

tie reviewed Jul 2, 2024

View reviewed changes

tie mentioned this pull request Jul 5, 2024

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

Closed

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 6, 2024

Cynerd force-pushed the nodejs-amrv7l branch from 2566ac7 to 7a053eb Compare July 11, 2024 09:46

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 11, 2024

Cynerd force-pushed the nodejs-amrv7l branch from 7a053eb to fc10c69 Compare July 11, 2024 15:55

Cynerd requested a review from tie July 11, 2024 15:58

Cynerd force-pushed the nodejs-amrv7l branch from fc10c69 to 10af551 Compare July 11, 2024 16:00

tie reviewed Jul 15, 2024

View reviewed changes

tie mentioned this pull request Jul 15, 2024

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

Closed

This was referenced Jul 16, 2024

nodejs: disable JS test suite on Darwin #325844

Merged

nodejs: use ninja for build #327653

Merged

Cynerd closed this Jul 25, 2024

	if pkgs.stdenv.hostPlatform.canExecute final
	then "${pkgs.runtimeShell} -c '\"$@\"' --"
	else if final.isWindows
	then "${wine}/bin/wine${optionalString (final.parsed.cpu.bits == 64) "64"}"
	else if final.isLinux && pkgs.stdenv.hostPlatform.isLinux && final.qemuArch != null
	then "${qemu-user}/bin/qemu-${final.qemuArch}"
	else if final.isWasi
	then "${pkgs.wasmtime}/bin/wasmtime"
	else if final.isMmix
	then "${pkgs.mmixware}/bin/mmix"
	else null;

	url = "https://github.com/buildroot/buildroot/raw/ecf060a2ff93626fb5fbe1c52b8d42b6ee0fbb5b/package/nodejs/nodejs-src/0001-add-qemu-wrapper-support.patch";
	url = "https://gitlab.com/buildroot.org/buildroot/-/raw/ecf060a2ff93626fb5fbe1c52b8d42b6ee0fbb5b/package/nodejs/nodejs-src/0001-add-qemu-wrapper-support.patch";

Uh oh!

Conversation

Cynerd commented Mar 8, 2023

Description of changes

Things done

Uh oh!

alyssais left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Cynerd commented Mar 28, 2023

Uh oh!

alyssais left a comment

Choose a reason for hiding this comment

Uh oh!

lilyinstarlight commented Jun 29, 2023

Uh oh!

Cynerd commented Jun 30, 2023

Uh oh!

Cynerd commented Jun 30, 2023

Uh oh!

tie commented Jul 8, 2023

Uh oh!

Uh oh!

lilyinstarlight commented Jul 8, 2023

Uh oh!

tie commented Jun 30, 2024

Uh oh!

Cynerd commented Jul 2, 2024

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!

tie Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Jul 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tie commented Jul 15, 2024

Uh oh!

Cynerd commented Jul 18, 2024

Uh oh!

tie commented Jul 24, 2024

Uh oh!

Cynerd commented Jul 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

tie Jul 2, 2024 •

edited

Loading