rules_nodejs icon indicating copy to clipboard operation
rules_nodejs copied to clipboard

Apple M1 ARM64 - Detection issues

Open dapullar opened this issue 3 years ago • 4 comments

🐞 bug report

Affected Rule

Any nodejs target

Is this a regression?

N/A

Description

launcher.sh occasionally incorrectly detects arm64 as x86_64

🔬 Minimal Reproduction

Invoke any nodejs target. Can't pin down how or when it occurs, resolves itself on cache clean

🔥 Exception or Error

When adding some logging to see what arch was being detected I was quite surprised to find out the result of uname -m (x86_64 😮)

Running any node target with some added debugging to output the reported arch:

>>>> FAIL: The node binary 'nodejs_darwin_amd64/bin/nodejs/bin/node' not found in runfiles.
This node toolchain was chosen based on your uname 'Darwin x86_64'.
Please file an issue to https://github.com/bazelbuild/rules_nodejs/issues if
you would like to add your platform to the supported rules_nodejs node platforms. <<<<

### DEBUG START ###
uname -m x86_64
uname -s Darwin
### DEBUG END ###

Running via shell

$ uname -m
arm64

I'm assuming that some portion of the toolchain causes rosetta to be required

Example:

$ arch -x86_64 uname -m
x86_64

🌍 Your Environment

Operating System:

  
Darwin Kernel Version 21.4.0
RELEASE_ARM64_T6000 arm64
  

Output of bazel version:

Build label: 4.2.1
Build target: bazel-out/darwin_arm64-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Mon Aug 30 15:23:11 2021 (1630336991)
Build timestamp: 1630336991
Build timestamp as int: 1630336991

Rules_nodejs version:

4.7.0

Anything else relevant?

Thankfully uname -v still reports the kernel version as ARM64.

$ arch -x86_64 uname -v
Darwin Kernel Version 21.4.0: Mon Feb 21 20:35:58 PST 2022; root:xnu-8020.101.4~2/RELEASE_ARM64_T6000

So, here is the patch I used to resolve my issue, it checks uname -v for the existence of ARM64 & falls back to the original check:

diff --git a/internal/node/launcher.sh b/internal/node/launcher.sh
index fd832d02..622b8389 100644
--- a/internal/node/launcher.sh
+++ b/internal/node/launcher.sh
@@ -127,10 +127,15 @@ else
     # The following paths must match up with _download_node in node_repositories
     windows) readonly node_toolchain="nodejs_windows_amd64/bin/nodejs/node.exe" ;;
     darwin)
-      case "${unameArch}" in
-        x86_64*) readonly node_toolchain="nodejs_darwin_amd64/bin/nodejs/bin/node" ;;
-        *) readonly node_toolchain="nodejs_darwin_arm64/bin/nodejs/bin/node" ;;
-      esac
+      unameVersion="$(uname -v)"
+      if [[ "${unameVersion}" == *"ARM64"* ]]; then
+        readonly node_toolchain="nodejs_darwin_arm64/bin/nodejs/bin/node"
+      else
+        case "${unameArch}" in
+          x86_64*) readonly node_toolchain="nodejs_darwin_amd64/bin/nodejs/bin/node" ;;
+          *) readonly node_toolchain="nodejs_darwin_arm64/bin/nodejs/bin/node" ;;
+        esac
+      fi
       ;;
     *)
       case "${unameArch}" in

dapullar avatar May 02 '22 00:05 dapullar

Could you try with a more recent version? It looks like I removed that code in January for 5.0 https://github.com/bazelbuild/rules_nodejs/pull/3234/files

alexeagle avatar May 02 '22 14:05 alexeagle

@alexeagle Yep, looks like you're correct, unable to reproduce this behaviour post 5.x release. Although, should this be handled under 4.x for LTS purposes?

dapullar avatar May 04 '22 02:05 dapullar

i'm similarly curious if this can be pulled into 4.x. migrating to v5 is a bigger task for our project but I think that fix could unblock folks using M1

loudmouth avatar Sep 06 '22 16:09 loudmouth

@loudmouth Not sure if that will ever happen, but you could always include a patch file with the changes that I outlined in the initial post

http_archive(
    name = "build_bazel_rules_nodejs",
    patch_args = ["-p1"],
    patches = ["//patches:rules_nodejs.patch"],
    ...
)

pullard avatar Sep 21 '22 04:09 pullard

This issue has been automatically marked as stale because it has not had any activity for 6 months. It will be closed if no further activity occurs in 30 days. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

github-actions[bot] avatar Mar 24 '23 02:03 github-actions[bot]

This issue was automatically closed because it went 30 days without any activity since it was labeled "Can Close?"

github-actions[bot] avatar Apr 23 '23 02:04 github-actions[bot]