feat(bazel): use new rbe_autoconfig() and new container. by xingao267 · Pull Request #29336 · angular/angular

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

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

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

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

is this part of RBE on-boarding now?

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

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

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 March 15, 2019 17:58

googlebot added the cla: yes label Mar 15, 2019

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

Comment thread tools/BUILD.bazel Resolved

nlopezgi reviewed Mar 28, 2019

View reviewed changes

Comment thread WORKSPACE Resolved

mhevery assigned alexeagle Apr 11, 2019

alexeagle reviewed Apr 12, 2019

View reviewed changes

alexeagle reviewed May 6, 2019

View reviewed changes

alexeagle mentioned this pull request May 7, 2019

user code depends on protoc and therefore clang bazel-contrib/rules_nodejs#739

Closed

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

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

Conversation

xingao267 commented Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

Does this PR introduce a breaking change?

Uh oh!

xingao267 commented Mar 28, 2019

Uh oh!

xingao267 commented Mar 28, 2019

Uh oh!

Uh oh!

Uh oh!

xingao267 commented Apr 1, 2019

Uh oh!

xingao267 commented Apr 8, 2019

Uh oh!

alexeagle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xingao267 commented Apr 25, 2019

Uh oh!

xingao267 commented Apr 25, 2019

Uh oh!

xingao267 commented Apr 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexeagle commented May 9, 2019

Uh oh!

xingao267 commented May 9, 2019

Uh oh!

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

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xingao267 commented Mar 15, 2019 •

edited

Loading