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

nodejs_binary incorrectly chooses the target platform with RBE #1305

Closed
josephperrott opened this issue Oct 25, 2019 · 1 comment · Fixed by #1320
Closed

nodejs_binary incorrectly chooses the target platform with RBE #1305

josephperrott opened this issue Oct 25, 2019 · 1 comment · Fixed by #1320
Assignees
Labels

Comments

@josephperrott
Copy link
Collaborator

It appears that nodejs_binary incorrectly chooses the platforms for execution of its outputs.

I believe this is currently controlled by the block found here

I created a demo/proof in this commit: josephperrott/angular@8dbec8c

Running yarn bazel run //tools/rbe-test:bin locally, everything builds and executes as expected and you get the expected console.log message.

$ yarn -s bazel run //tools/rbe-test:bin
INFO: Invocation ID: ce95d7c0-b0d2-4c30-a280-25d015ed331c
...
INFO: Build completed successfully, 1 total action
run!
✨  Done in 1.55s.

When running yarn bazel run //tools/rbe-test:bin --config=remote to force remote execution, everything builds but then on attempted execution it fails as it can't run the binary

$ yarn -s bazel run //tools/rbe-test:bin --config=remote
WARNING: Option 'tls_enabled' is deprecated: Deprecated. Please specify a valid protocol in the URL (https or grpcs).
WARNING: option '--jobs' was expanded to from both option '--config=remote' (source command line options) and option '--config=remote' (source command line options)
INFO: Invocation ID: 26cf4261-e1fb-4b2b-a68f-8f1dc762d7ac
...
INFO: Build completed successfully, 60 total actions
/private/var/tmp/_bazel_jperrott/104b07d8216361462e08a3d288166690/execroot/angular/bazel-out/darwin-fastbuild/bin/tools/rbe-test/bin.sh: line 151: /private/var/tmp/_bazel_jperrott/104b07d8216361462e08a3d288166690/external/nodejs_linux_amd64/bin/nodejs/bin/node: cannot execute binary file
/private/var/tmp/_bazel_jperrott/104b07d8216361462e08a3d288166690/execroot/angular/bazel-out/darwin-fastbuild/bin/tools/rbe-test/bin.sh: line 151: /private/var/tmp/_bazel_jperrott/104b07d8216361462e08a3d288166690/external/nodejs_linux_amd64/bin/nodejs/bin/node: Undefined error: 0
error Command failed with exit code 1.

As you can see from the error message, it is attempting to run the node binary from nodejs_linuxamd64 rather than nodejsdarwin_amd64. This is controlled by line 118 of the bin.sh script that is created for the build:

readonly node=$(rlocation "nodejs_linux_amd64/bin/nodejs/bin/node")

Interestingly the line below, 119, correctly identifies the platform:

readonly repository_args=$(rlocation "nodejs_darwin_amd64/bin/node_repo_args.sh")
@gregmagolan
Copy link
Collaborator

gregmagolan commented Oct 28, 2019

I took a quick look at this and its definitely a bug that needs fixing. The launcher template is expanded on the linux RBE machine where the node tool path is readonly node=$(rlocation "nodejs_linux_amd64/bin/nodejs/bin/node") but the run command is run on the local OSX machine which fails.

The solution would seem to be to have the launcher template check the platform and call the appropriate node tool which can work for the subset of node toolchains defined already but how would that work with user defined toolchains?

@gregmagolan gregmagolan changed the title rules_nodejs incorrectly chooses the target platform rules_nodejs incorrectly chooses the target platform with RBE Oct 28, 2019
@gregmagolan gregmagolan changed the title rules_nodejs incorrectly chooses the target platform with RBE nodejs_binary incorrectly chooses the target platform with RBE Oct 28, 2019
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Oct 30, 2019
A bit hacky but the issue can be reproduced in run_targets in bazelci. There doesn’t seem to be a run flags to pass `—config=remote` so I used shell_commands to add the appropriate settings to .bazelrc. No `—remote_executor` is specified but Bazel seems to use a linux VM for local testing of RBE as RBE is used and the build happens on Linux and the result is run on OSX which reproduces the issue.
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 1, 2019
A bit hacky but the issue can be reproduced in run_targets in bazelci. There doesn’t seem to be a run flags to pass `—config=remote` so I used shell_commands to add the appropriate settings to .bazelrc. No `—remote_executor` is specified but Bazel seems to use a linux VM for local testing of RBE as RBE is used and the build happens on Linux and the result is run on OSX which reproduces the issue.
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 1, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 1, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 1, 2019
A bit hacky but the issue can be reproduced in run_targets in bazelci. There doesn’t seem to be a run flags to pass `—config=remote` so I used shell_commands to add the appropriate settings to .bazelrc. No `—remote_executor` is specified but Bazel seems to use a linux VM for local testing of RBE as RBE is used and the build happens on Linux and the result is run on OSX which reproduces the issue.
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 1, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 1, 2019
A bit hacky but the issue can be reproduced in run_targets in bazelci. There doesn’t seem to be a run_flags to pass —config=remote so I used shell_commands to set the --platform to linux in a new mac_fake_rbe job on buildkite. This sets up the same scenario where the build with --platform linux selected (as it would be on RBE) but the resulting nodejs_binary runnable target is run on osx.
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 1, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 1, 2019
A bit hacky but the issue can be reproduced in run_targets in bazelci. There doesn’t seem to be a run_flags to pass —config=remote so I used shell_commands to set the --platform to linux in a new mac_fake_rbe job on buildkite. This sets up the same scenario where the build with --platform linux selected (as it would be on RBE) but the resulting nodejs_binary runnable target is run on osx.
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 1, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 1, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 1, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 1, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 1, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 7, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 7, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 7, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 7, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 9, 2019
…eb_test_suite (+3 squashed commits)

Squashed commits:
[d93fb4d] fix: cleanup in node_repositories.bzl
[3efce87] test: fix toolchain test now that nodejs_binary chooses the correct runfiles for the host platform

Runfiles are no longer tested as what should be tested is that ctx.toolchains["@build_bazel_rules_nodejs//toolchains/node:toolchain_type"].nodeinfo.tool_files are for the correct `--platform`.
[7551535] fix: fix nodejs_binary cross-platform RBE issue bazelbuild#1305

* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 9, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 12, 2019
A bit hacky but the issue can be reproduced in run_targets in bazelci. There doesn’t seem to be a run_flags to pass —config=remote so I used shell_commands to set the --platform to linux in a new mac_fake_rbe job on buildkite. This sets up the same scenario where the build with --platform linux selected (as it would be on RBE) but the resulting nodejs_binary runnable target is run on osx.
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 12, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 12, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305

fix: node
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Nov 12, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes bazelbuild#1305

fix: node
alexeagle pushed a commit that referenced this issue Nov 13, 2019
A bit hacky but the issue can be reproduced in run_targets in bazelci. There doesn’t seem to be a run_flags to pass —config=remote so I used shell_commands to set the --platform to linux in a new mac_fake_rbe job on buildkite. This sets up the same scenario where the build with --platform linux selected (as it would be on RBE) but the resulting nodejs_binary runnable target is run on osx.
alexeagle pushed a commit that referenced this issue Nov 13, 2019
* node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time
* node_binary now adds the correct node tool_files to the nodejs_binary runfiles

Fixes #1305

fix: node
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants