Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apple M1 ARM64 - Detection issues #3438

Closed
dapullar opened this issue May 2, 2022 · 6 comments
Closed

Apple M1 ARM64 - Detection issues #3438

dapullar opened this issue May 2, 2022 · 6 comments
Labels
Can Close? We will close this in 30 days if there is no further activity

Comments

@dapullar
Copy link

dapullar commented May 2, 2022

🐞 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
@alexeagle
Copy link
Collaborator

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

@dapullar
Copy link
Author

dapullar commented May 4, 2022

@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?

@loudmouth
Copy link

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

@pullard
Copy link

pullard commented Sep 21, 2022

@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"],
    ...
)

@github-actions
Copy link

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 github-actions bot added the Can Close? We will close this in 30 days if there is no further activity label Mar 24, 2023
@github-actions
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Can Close? We will close this in 30 days if there is no further activity
Projects
None yet
Development

No branches or pull requests

4 participants