feat(bazel): use new rbe_autoconfig() and new container. #29336

xingao267 · 2019-03-15T17:58:56Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
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
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?

The CI is currently using a container not maintained by RBE Toolchain team. I'm updating it to a container maintained by us.

I also enabled usage of rbe_autoconfig() repository rule, a rule that can dynamically select toolchain configs from @bazel_toolchains repo if available or generate the configs on the fly if not available.

Does this PR introduce a breaking change?

Yes
No

xingao267 · 2019-03-28T20:27:53Z

Read for review now.

xingao267 · 2019-03-28T20:27:58Z

cc @nlopezgi

tools/BUILD.bazel

WORKSPACE

xingao267 · 2019-04-01T14:43:39Z

@alexeagle @mhevery could you help take a look?

xingao267 · 2019-04-08T18:57:33Z

@alexeagle @mhevery friendly ping~

alexeagle

sorry I missed this - you can reach out over chat. My github notification volume is too high for me to keep up with it.

alexeagle · 2019-04-12T19:11:29Z

.bazelrc

-# Load default settings for Remote Build Execution
-# When updating, the URLs of bazel_toolchains in packages/bazel/package.bzl
-# may also need to be updated (see https://github.com/angular/angular/pull/27935)
-import %workspace%/third_party/github.com/bazelbuild/bazel-toolchains/bazelrc/bazel-0.24.0.bazelrc


what's your reasoning for removing the files from third_party?

We are deprecating those bazelrc file per bazel version from bazel-toolchains repo. And the generic one will not work for this project.

I think this is still far too much configuration to ask users to copy-paste into their project to use remote build execution and will significantly hamper the story for Angular users who want to have this mostly managed for them. Could we schedule a quick chat with Geoffrey to discuss?

Will schedule a meeting. +@nlopezgi who owns the new rbe_repo() rule.

alexeagle · 2019-04-12T19:12:32Z

.bazelrc

+# machine exactly match the host machine.
+build:remote --spawn_strategy=remote
+build:remote --strategy=Javac=remote
+build:remote --strategy=Closure=remote


we don't have any Closure or Java code. why would we want these lines? this is why I had vendored the file from the bazel-toolchains repo.

Imagine that this file was used by Angular users. I wouldn't want their configuration to have all these bits either. It's totally unergonomic.

alexeagle · 2019-04-12T19:14:38Z

WORKSPACE

+load("@bazel_toolchains//rules:environments.bzl", "clang_env")
+load("@bazel_toolchains//rules:rbe_repo.bzl", "rbe_autoconfig")
+
+rbe_autoconfig(


is this part of RBE on-boarding now?

alexeagle · 2019-04-12T19:14:53Z

WORKSPACE

+    # Note that if you change the `digest`, you might also need to update the
+    # `base_container_digest` to make sure marketplace.gcr.io/google/rbe-ubuntu16-04-webtest:<digest>
+    # and marketplace.gcr.io/google/rbe-ubuntu16-04:<base_container_digest> have the
+    # same Clang and JDK installed.


why are Clang and JDK involved? we don't have any Java or C++ code

For Java, see the reasoning here GoogleCloudPlatform/container-definitions#52. For Clang, there is at least one cc_binary target dependency in this repo: https://github.com/angular/angular/blob/master/packages/bazel/src/ng_rollup_bundle.bzl#L197 which is a cc_binary (https://github.com/google/brotli/blob/master/BUILD#L130). If you can remove that dependency, I can try to remove the clang layer from the container.

Removing the cc_binary dep in https://github.com/angular/angular/pull/29912/files

that PR has landed so this is unblocked, can remove clang now

xingao267 · 2019-04-25T15:09:17Z

@alexeagle I tried the new container without a c compiler installed. And it failed due to there is cc dependencies in com_google_protobuf. See test failures here https://circleci.com/gh/angular/angular/298502?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

xingao267 · 2019-04-25T15:09:54Z

I'm going to modify this PR to revert back to using the container with clang installed.

xingao267 · 2019-04-26T16:26:01Z

@alexeagle this is ready for review again.

alexeagle · 2019-05-06T21:10:34Z

WORKSPACE

+    # Clang is needed because of the dependency on @com_google_protobuf.
+    # Java is needed for the Bazel's test executor Java tool.
+    digest = "sha256:74a8e9dca4781d5f277a7bd8e7ea7ed0f5906c79c9cd996205b6d32f090c62f3",
+    env = clang_env(),


as discussed on chat, the dependency on clang is a bug and should be fixed. I'd rather fix it before landing a workaround since paying tech debt is a lot harder than not introducing it

This tech debt and workaround is not introduced today or by this PR. The container that all current Angular's RBE builds use (gcr.io/asci-toolchain/nosla-ubuntu16_04-webtest) has Clang installed. Comparing to that container, this new container removed Go layer and few other debs that are not used by current Angular project, which is an improvement. And this container is being actively maintained and updated with new features and latest security patches.

Regarding removing the Angular's dependency on protoc or Clang, we should either open a bug to Bazel team or Angular project's maintainer to fix it. After that is done, our team can test and hopefully remove Clang layer from this container. But I don't think that work should block merging this PR at all.

After this PR is merged, maintainers no longer need to update .bazelrc file, toolchain and platform related flags for RBE builds and tests (unless there is a breaking change in Bazel related to those flags). Maintainers just need to update the pin of @bazel-toolchains repo regularly in the packages/bazel/package.bzl file according to https://releases.bazel.build/bazel-toolchains.html to include the latest checked-in toolchain configs. If rbe_autoconfig() cannot find appropriate toolchain configs for the version of Bazel in the version of @bazel_toolchains repo that is currently used by this project, it will pull down the container and generate the configs on the fly as the beginning of the build/test.

alexeagle · 2019-05-09T18:59:07Z

Going to try kiilling the clang dependency by excluding the one skydoc target from the remote execution

xingao267 · 2019-05-09T19:00:10Z

@alexeagle looks like still used https://circleci.com/gh/angular/angular/313736?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

After this PR is merged, maintainers no longer need to update .bazelrc file, toolchain and platform related flags for RBE builds and tests (unless there is a breaking change in Bazel related to those flags). Maintainers just need to update the pin of @bazel-toolchains repo regularly in the packages/bazel/package.bzl file according to https://releases.bazel.build/bazel-toolchains.html to include the latest checked-in toolchain configs. If rbe_autoconfig() cannot find appropriate toolchain configs for the version of Bazel in the version of @bazel_toolchains repo that is currently used by this project, it will pull down the container and generate the configs on the fly as the beginning of the build/test. PR Close #29336

After this PR is merged, maintainers no longer need to update .bazelrc file, toolchain and platform related flags for RBE builds and tests (unless there is a breaking change in Bazel related to those flags). Maintainers just need to update the pin of @bazel-toolchains repo regularly in the packages/bazel/package.bzl file according to https://releases.bazel.build/bazel-toolchains.html to include the latest checked-in toolchain configs. If rbe_autoconfig() cannot find appropriate toolchain configs for the version of Bazel in the version of @bazel_toolchains repo that is currently used by this project, it will pull down the container and generate the configs on the fly as the beginning of the build/test. PR Close angular#29336

angular-automatic-lock-bot · 2019-09-15T01:51:33Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

xingao267 requested a review from a team as a code owner March 15, 2019 17:58

googlebot added the cla: yes label Mar 15, 2019

xingao267 force-pushed the toolchain branch 2 times, most recently from 9e41c88 to f1e5093 Compare March 15, 2019 18:44

mhevery added the area: bazel Issues related to the published `@angular/bazel` build rules label Mar 19, 2019

ngbot bot added this to the needsTriage milestone Mar 19, 2019

xingao267 force-pushed the toolchain branch 6 times, most recently from 13d213e to 2be9995 Compare March 27, 2019 14:33

xingao267 force-pushed the toolchain branch 7 times, most recently from 0b02dc9 to 91fa7d1 Compare March 28, 2019 20:25

xingao267 changed the title ~~DO NOT REVIEW, TESTING feat(bazel): use new rbe_autoconfig() and new container.~~ feat(bazel): use new rbe_autoconfig() and new container. Mar 28, 2019

nlopezgi reviewed Mar 28, 2019

View reviewed changes

tools/BUILD.bazel Outdated Show resolved Hide resolved

nlopezgi reviewed Mar 28, 2019

View reviewed changes

WORKSPACE Outdated Show resolved Hide resolved

xingao267 force-pushed the toolchain branch from 91fa7d1 to bb651b2 Compare March 28, 2019 21:16

xingao267 force-pushed the toolchain branch from bb651b2 to dc80f5d Compare April 8, 2019 18:56

mhevery assigned alexeagle Apr 11, 2019

alexeagle reviewed Apr 12, 2019

View reviewed changes

xingao267 force-pushed the toolchain branch 5 times, most recently from 20f290e to 2dbb2ae Compare April 15, 2019 23:11

xingao267 force-pushed the toolchain branch from 2dbb2ae to 8678541 Compare April 25, 2019 15:01

xingao267 force-pushed the toolchain branch 2 times, most recently from d31ffa1 to 60fe7c1 Compare April 26, 2019 13:59

xingao267 force-pushed the toolchain branch from 60fe7c1 to ad28200 Compare May 6, 2019 14:18

alexeagle reviewed May 6, 2019

View reviewed changes

alexeagle mentioned this pull request May 7, 2019

user code depends on protoc and therefore clang bazelbuild/rules_nodejs#739

Closed

xingao267 force-pushed the toolchain branch from ad28200 to d6c927a Compare May 9, 2019 16:59

xingao267 requested a review from a team as a code owner May 9, 2019 18:34

xingao267 force-pushed the toolchain branch from ce58d01 to a034eec Compare May 9, 2019 18:48

xingao267 force-pushed the toolchain branch from a034eec to d6c927a Compare May 9, 2019 19:49

alexeagle approved these changes May 9, 2019

View reviewed changes

alexeagle added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels May 9, 2019

alxhub closed this in 9abf114 May 9, 2019

iget-master mentioned this pull request Jul 10, 2019

CHANGELOG.md shouldn't include pre-release changes? #31491

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bazel): use new rbe_autoconfig() and new container. #29336

feat(bazel): use new rbe_autoconfig() and new container. #29336

xingao267 commented Mar 15, 2019 •

edited

xingao267 commented Mar 28, 2019

xingao267 commented Mar 28, 2019

xingao267 commented Apr 1, 2019

xingao267 commented Apr 8, 2019

alexeagle left a comment

alexeagle Apr 12, 2019

xingao267 Apr 25, 2019

alexeagle May 6, 2019

xingao267 May 6, 2019

alexeagle Apr 12, 2019

alexeagle Apr 12, 2019

xingao267 Apr 12, 2019

alexeagle Apr 12, 2019

xingao267 Apr 12, 2019

alexeagle Apr 15, 2019

alexeagle Apr 18, 2019

xingao267 commented Apr 25, 2019

xingao267 commented Apr 25, 2019

xingao267 commented Apr 26, 2019

alexeagle May 6, 2019

xingao267 May 6, 2019

alexeagle commented May 9, 2019

xingao267 commented May 9, 2019

angular-automatic-lock-bot bot commented Sep 15, 2019

feat(bazel): use new rbe_autoconfig() and new container. #29336

feat(bazel): use new rbe_autoconfig() and new container. #29336

Conversation

xingao267 commented Mar 15, 2019 • edited

PR Checklist

PR Type

What is the current behavior?

Does this PR introduce a breaking change?

xingao267 commented Mar 28, 2019

xingao267 commented Mar 28, 2019

xingao267 commented Apr 1, 2019

xingao267 commented Apr 8, 2019

alexeagle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xingao267 commented Apr 25, 2019

xingao267 commented Apr 25, 2019

xingao267 commented Apr 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeagle commented May 9, 2019

xingao267 commented May 9, 2019

angular-automatic-lock-bot bot commented Sep 15, 2019

xingao267 commented Mar 15, 2019 •

edited