fix: fix nodejs_binary cross-platform RBE issue #1305 #1320

gregmagolan · 2019-10-30T21:52:34Z

First commit reproduces #1305.

Repo here: https://buildkite.com/bazel/rules-nodejs-nodejs/builds/4727#5a880596-2b15-49a3-947c-a7b2894f6cca from #1324.

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.

2nd commit fixes the issue.

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

3rd commit updates the nodejs toolchain test to no longer test for runfiles. What should be tested is that ctx.toolchains["@build_bazel_rules_nodejs//toolchains/node:toolchain_type"].nodeinfo.tool_files are for the correct selected --platform.

Fixes #1305.

PR Checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature (please, look at the "Scope of the project" section in the README.md file)
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
Other... Please describe:

What is the current behavior?

nodejs_binary does not work with cross-platform RBE. For example, developer machine is osx and RBE is a linux box

What is the new behavior?

nodejs_binary works with cross-platform RBE

Does this PR introduce a breaking change?

Yes
No

Other information

internal/node/node.bzl

internal/node/node_launcher.sh

alexeagle · 2019-11-01T15:55:13Z

internal/node/node_launcher.sh

+    Darwin*)    machine=darwin;;
+    CYGWIN*)    machine=windows;;
+    MINGW*)     machine=windows;;
+    MSYS_NT*)   machine=windows;;


sigh, when we introduce a batch/powershell version of the launcher we'll have to pick through this logic again

alexeagle

Need more error handling in the case fallthrough

.bazelci/presubmit.yml

internal/node/node.bzl

gregmagolan · 2019-11-07T01:48:11Z

Need more error handling in the case fallthrough

Done

* patch for build_bazel_rules_nodejs from bazel-contrib/rules_nodejs#1320 * patch for @bazel/protractor from bazel-contrib/rules_nodejs#1320 * work-around for rules_webtesting cross-platform RBE issues

alexeagle · 2019-11-11T15:35:08Z

.bazelci/presubmit.yml

+    build_flags:
+    - "--platforms=@build_bazel_rules_nodejs//toolchains/node:darwin_amd64"
+    build_targets:
+    - "//..."


feels really expensive to re-build everything in each configuration. I wonder if we are using more CI resources than any other rules

we do have quite a few jobs now. it's probably not strictly necessary to build in every permutation but I think at the least we should build everything or some subset of everything with each platform.

maybe just //internal/... is sufficient to get the coverage we need?

👍 that should do it for now as only node_binary/node_test vary the build based on platform. We can expand coverage in the future if some of the packages with build differences based on platform.

internal/node/node.bzl

alexeagle · 2019-11-11T15:42:31Z

internal/node/node_launcher.sh

+  readonly node=$(rlocation "${vendored_node}")
+
+  if [ ! -f "${node}" ]; then
+      printf "\n>>>> FAIL: The vendored node binary '${vendored_node}' not found in runfiles. <<<<\n\n" >&2


will users know what to do with this error?

It shouldn't happen as the vendored_node binary should be added to runfiles by nodejs_binary... but in the case where there is a bug or something unexpected happens better to have a semi-helpful error message & graceful exit than a crash.

internal/node/node_launcher.sh

internal/node/test/BUILD.bazel

packages/protractor/src/protractor.conf.js

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.

* 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 bazel-contrib#1305 fix: node

…unfiles 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`.

This release includes nodejs cross-platform RBE fix in bazel-contrib/rules_nodejs#1320 and adds `args` to terser_minified in bazel-contrib/rules_nodejs#1317. These changes are needed to land a few outstanding PRs. * build: fixes for cross-platform RBE angular#33708 * build: update zone.js to use the new rollup_bundle angular#33329

This release includes nodejs cross-platform RBE fix in bazel-contrib/rules_nodejs#1320 and adds `args` to terser_minified in bazel-contrib/rules_nodejs#1317. These changes are needed to land a few outstanding PRs. * build: fixes for cross-platform RBE angular#33708 * build: update zone.js to use the new rollup_bundle angular#33329 fix: fix

This release includes nodejs cross-platform RBE fix in bazel-contrib/rules_nodejs#1320 and adds `args` to terser_minified in bazel-contrib/rules_nodejs#1317. These changes are needed to land a few outstanding PRs. * build: fixes for cross-platform RBE #33708 * build: update zone.js to use the new rollup_bundle #33329 fix: fix PR Close #33802

* patch for build_bazel_rules_nodejs from bazel-contrib/rules_nodejs#1320 * patch for @bazel/protractor from bazel-contrib/rules_nodejs#1320 * work-around for rules_webtesting cross-platform RBE issues

googlebot added the cla: yes label Oct 30, 2019

gregmagolan force-pushed the fix_mac_rbe branch from 95ab983 to 0743693 Compare October 30, 2019 22:05

gregmagolan changed the title ~~test: repro~~ test: reproduce issue https://github.com/bazelbuild/rules_nodejs/issues/1305 on buildkite Oct 30, 2019

gregmagolan force-pushed the fix_mac_rbe branch 2 times, most recently from cce1e08 to e016ed0 Compare November 1, 2019 06:35

gregmagolan changed the title ~~test: reproduce issue https://github.com/bazelbuild/rules_nodejs/issues/1305 on buildkite~~ fix: fix nodejs_binary cross-platform RBE issue #1305 Nov 1, 2019

gregmagolan marked this pull request as ready for review November 1, 2019 06:46

gregmagolan requested review from alexeagle and soldair as code owners November 1, 2019 06:46

gregmagolan force-pushed the fix_mac_rbe branch 5 times, most recently from 7d9531c to edbd128 Compare November 1, 2019 08:54

alexeagle approved these changes Nov 1, 2019

View reviewed changes

gregmagolan force-pushed the fix_mac_rbe branch 2 times, most recently from 4d0eef1 to 09446d7 Compare November 1, 2019 20:27

alexeagle approved these changes Nov 1, 2019

View reviewed changes

.bazelci/presubmit.yml Show resolved Hide resolved

internal/node/node.bzl Outdated Show resolved Hide resolved

gregmagolan added a commit to gregmagolan/angular that referenced this pull request Nov 2, 2019

fix: update patch from bazel-contrib/rules_nodejs#1320

a6808ef

gregmagolan force-pushed the fix_mac_rbe branch 5 times, most recently from afc1a95 to d8dfa36 Compare November 7, 2019 01:38

gregmagolan force-pushed the fix_mac_rbe branch from d8dfa36 to d93fb4d Compare November 7, 2019 03:33

gregmagolan pushed a commit to gregmagolan/angular that referenced this pull request Nov 9, 2019

fix: patch from bazel-contrib/rules_nodejs#1320

4dff915

gregmagolan force-pushed the fix_mac_rbe branch from fda9c34 to f68d6f4 Compare November 9, 2019 07:38

gregmagolan force-pushed the fix_mac_rbe branch 2 times, most recently from b7e1030 to e49745e Compare November 9, 2019 09:18

gregmagolan force-pushed the fix_mac_rbe branch from e49745e to 92d269d Compare November 9, 2019 10:00

gregmagolan force-pushed the fix_mac_rbe branch from 92d269d to 1172135 Compare November 9, 2019 17:41

alexeagle reviewed Nov 11, 2019

View reviewed changes

gregmagolan force-pushed the fix_mac_rbe branch 3 times, most recently from 6461dad to d341869 Compare November 12, 2019 16:33

gregmagolan force-pushed the fix_mac_rbe branch from d341869 to 4f9a00d Compare November 12, 2019 16:34

gregmagolan force-pushed the fix_mac_rbe branch from 4f9a00d to b0b638d Compare November 12, 2019 16:38

alexeagle merged commit b07f65f into bazel-contrib:master Nov 13, 2019

gregmagolan mentioned this pull request Nov 13, 2019

Update to nodejs rules 0.40.0 angular/angular#33802

Closed

14 tasks

ajbouh mentioned this pull request Aug 17, 2020

Can't use the output of rollup_bundle as an input to ts_library #2124

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix nodejs_binary cross-platform RBE issue #1305 #1320

fix: fix nodejs_binary cross-platform RBE issue #1305 #1320

gregmagolan commented Oct 30, 2019 •

edited

Loading

alexeagle Nov 1, 2019

alexeagle left a comment

gregmagolan commented Nov 7, 2019

alexeagle Nov 11, 2019

gregmagolan Nov 12, 2019

alexeagle Nov 12, 2019

gregmagolan Nov 12, 2019

alexeagle Nov 11, 2019

gregmagolan Nov 11, 2019 •

edited

Loading

fix: fix nodejs_binary cross-platform RBE issue #1305 #1320

fix: fix nodejs_binary cross-platform RBE issue #1305 #1320

Conversation

gregmagolan commented Oct 30, 2019 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

alexeagle Nov 1, 2019

Choose a reason for hiding this comment

alexeagle left a comment

Choose a reason for hiding this comment

gregmagolan commented Nov 7, 2019

alexeagle Nov 11, 2019

Choose a reason for hiding this comment

gregmagolan Nov 12, 2019

Choose a reason for hiding this comment

alexeagle Nov 12, 2019

Choose a reason for hiding this comment

gregmagolan Nov 12, 2019

Choose a reason for hiding this comment

alexeagle Nov 11, 2019

Choose a reason for hiding this comment

gregmagolan Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

gregmagolan commented Oct 30, 2019 •

edited

Loading

gregmagolan Nov 11, 2019 •

edited

Loading