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

build: fixes for cross-platform RBE #33708

Closed

Conversation

@gregmagolan
Copy link
Contributor

gregmagolan commented Nov 9, 2019

  • patch for build_bazel_rules_nodejs from bazelbuild/rules_nodejs#1320
  • patch for @bazel/protractor from bazelbuild/rules_nodejs#1320 (using patch-package npm tool for this as its much simpler to setup than the manual postinstall-patches.js)
  • work-around for rules_webtesting cross-platform RBE issues

Cross-platform RBE on a mac with linux RBE workers requires the following config:

--config=remote --google_credentials=/path/to/credentials.json --cpu=k8 --host_cpu=k8

both --cpu & --host_cpu need to be specified when building from OSX or else there is bazel toolchain error:

ERROR: /private/var/tmp/_bazel_greg/5e8f8a9cd1c6fbc1afd11e37ee1fe2e5/external/bazel_toolchains/configs/ubuntu16_04_clang/9.0.0/bazel_1.0.0/cc/BUILD:50:1: in cc_toolchain_suite rule @bazel_toolchains//configs/ubuntu16_04_clang/9.0.0/bazel_1.0.0/cc:toolchain: cc_toolchain_suite '@bazel_toolchains//configs/ubuntu16_04_clang/9.0.0/bazel_1.0.0/cc:toolchain' does not contain a toolchain for cpu 'darwin'

Patches added here can be removed after bazelbuild/rules_nodejs#1320 lands and angular is updated to a release of rules_nodejs that includes the changes.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Cross-platform RBE doesn't work.

Issue Number: bazelbuild/rules_nodejs#1305

What is the new behavior?

Cross-platform RBE works.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@gregmagolan gregmagolan requested review from IgorMinar and angular/dev-infra-framework as code owners Nov 9, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 9, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no label Nov 9, 2019
@gregmagolan gregmagolan requested a review from josephperrott Nov 9, 2019
@gregmagolan gregmagolan force-pushed the gregmagolan:with-rulesnodejs-change branch 8 times, most recently from 0a09145 to e20194b Nov 9, 2019
@ngbot ngbot bot added this to the needsTriage milestone Nov 9, 2019
@gregmagolan

This comment has been minimized.

Copy link
Contributor Author

gregmagolan commented Nov 9, 2019

@googlebot I consent

@gregmagolan

This comment has been minimized.

Copy link
Contributor Author

gregmagolan commented Nov 9, 2019

Turned on Windows CI temporarily to verify it is green with this change: https://app.circleci.com/jobs/github/angular/angular/521453

@gregmagolan gregmagolan force-pushed the gregmagolan:with-rulesnodejs-change branch 2 times, most recently from 2504ef7 to b0dc0f4 Nov 9, 2019
gregmagolan added a commit to gregmagolan/angular that referenced this pull request Nov 13, 2019
This release includes nodejs cross-platform RBE fix in bazelbuild/rules_nodejs#1320 and adds `args` to terser_minified in bazelbuild/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
@gregmagolan

This comment has been minimized.

Copy link
Contributor Author

gregmagolan commented Nov 13, 2019

0.40 release is out which includes the changes in the patch here. After the #33802 update lands this PR can be updated and land without patches.

gregmagolan added a commit to gregmagolan/angular that referenced this pull request Nov 13, 2019
This release includes nodejs cross-platform RBE fix in bazelbuild/rules_nodejs#1320 and adds `args` to terser_minified in bazelbuild/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
@gregmagolan gregmagolan referenced this pull request Nov 13, 2019
3 of 14 tasks complete
gregmagolan added a commit to gregmagolan/angular that referenced this pull request Nov 13, 2019
This release includes nodejs cross-platform RBE fix in bazelbuild/rules_nodejs#1320 and adds `args` to terser_minified in bazelbuild/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
gregmagolan added a commit to gregmagolan/angular that referenced this pull request Nov 13, 2019
This release includes nodejs cross-platform RBE fix in bazelbuild/rules_nodejs#1320 and adds `args` to terser_minified in bazelbuild/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
kara added a commit that referenced this pull request Nov 13, 2019
This release includes nodejs cross-platform RBE fix in bazelbuild/rules_nodejs#1320 and adds `args` to terser_minified in bazelbuild/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
kara added a commit that referenced this pull request Nov 13, 2019
This release includes nodejs cross-platform RBE fix in bazelbuild/rules_nodejs#1320 and adds `args` to terser_minified in bazelbuild/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
@gregmagolan gregmagolan force-pushed the gregmagolan:with-rulesnodejs-change branch 2 times, most recently from 96e1876 to 4030351 Nov 13, 2019
# rules_webtesting has a required_tag "native" for `chromium-local` browser
if not "native" in tags:
tags = tags + ["native"]

_karma_web_test_suite(
runtime_deps = local_runtime_deps,
bootstrap = bootstrap,
deps = local_deps,
# Run unit tests on local Chromium by default.

This comment has been minimized.

Copy link
@josephperrott

josephperrott Nov 13, 2019

Member

Can you remove this comment as firefox tests are no longe available with our move to use our own browser_repositories.bzl

@gregmagolan gregmagolan force-pushed the gregmagolan:with-rulesnodejs-change branch from 4030351 to 6749877 Nov 13, 2019
The earlier update to nodejs rules 0.40.0 fixes the cross-platform RBE issues with nodejs_binary. This commit adds a work-around for rules_webtesting cross-platform RBE issues.
@gregmagolan gregmagolan force-pushed the gregmagolan:with-rulesnodejs-change branch from 6749877 to b3e567a Nov 13, 2019
Copy link
Member

josephperrott left a comment

LGTM, global approval

@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 14, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@ngbot ngbot bot added the PR action: merge label Nov 14, 2019
@alxhub alxhub closed this in e6045ee Nov 15, 2019
alxhub added a commit that referenced this pull request Nov 15, 2019
The earlier update to nodejs rules 0.40.0 fixes the cross-platform RBE issues with nodejs_binary. This commit adds a work-around for rules_webtesting cross-platform RBE issues.

PR Close #33708
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.