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

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

Closed
wants to merge 1 commit into from

Conversation

@xingao267
Copy link
Contributor

commented Mar 15, 2019

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?

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 xingao267 requested a review from angular/fw-dev-infra as a code owner Mar 15, 2019

@googlebot googlebot added the cla: yes label Mar 15, 2019

@xingao267 xingao267 force-pushed the xingao267:toolchain branch 2 times, most recently from 9e41c88 to f1e5093 Mar 15, 2019

@mhevery mhevery added the comp: bazel label Mar 19, 2019

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

@xingao267 xingao267 force-pushed the xingao267:toolchain branch 6 times, most recently from 13d213e to 2be9995 Mar 20, 2019

@xingao267 xingao267 force-pushed the xingao267:toolchain branch 7 times, most recently from 0b02dc9 to 91fa7d1 Mar 27, 2019

@xingao267

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

Read for review now.

@xingao267

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

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

Show resolved Hide resolved tools/BUILD.bazel Outdated
Show resolved Hide resolved WORKSPACE Outdated

@xingao267 xingao267 force-pushed the xingao267:toolchain branch from 91fa7d1 to bb651b2 Mar 28, 2019

@xingao267

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

@alexeagle @mhevery could you help take a look?

@xingao267 xingao267 force-pushed the xingao267:toolchain branch from bb651b2 to dc80f5d Apr 8, 2019

@xingao267

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

@alexeagle @mhevery friendly ping~

@alexeagle
Copy link
Contributor

left a comment

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

# 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

This comment has been minimized.

Copy link
@alexeagle

alexeagle Apr 12, 2019

Contributor

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

This comment has been minimized.

Copy link
@xingao267

xingao267 Apr 25, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@alexeagle

alexeagle May 6, 2019

Contributor

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?

This comment has been minimized.

Copy link
@xingao267

xingao267 May 6, 2019

Author Contributor

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

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

This comment has been minimized.

Copy link
@alexeagle

alexeagle Apr 12, 2019

Contributor

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.

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

rbe_autoconfig(

This comment has been minimized.

Copy link
@alexeagle

alexeagle Apr 12, 2019

Contributor

is this part of RBE on-boarding now?

This comment has been minimized.

Copy link
@xingao267

xingao267 Apr 12, 2019

Author Contributor

Yes.

WORKSPACE Outdated
# 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.

This comment has been minimized.

Copy link
@alexeagle

alexeagle Apr 12, 2019

Contributor

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

This comment has been minimized.

Copy link
@xingao267

xingao267 Apr 12, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@alexeagle

This comment has been minimized.

Copy link
@alexeagle

alexeagle Apr 18, 2019

Contributor

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

@xingao267 xingao267 force-pushed the xingao267:toolchain branch 6 times, most recently from 20f290e to 2dbb2ae Apr 12, 2019

@xingao267 xingao267 force-pushed the xingao267:toolchain branch from 2dbb2ae to 8678541 Apr 25, 2019

@xingao267

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

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

@xingao267 xingao267 force-pushed the xingao267:toolchain branch 2 times, most recently from d31ffa1 to 60fe7c1 Apr 25, 2019

@xingao267

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@alexeagle this is ready for review again.

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

# 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(),

This comment has been minimized.

Copy link
@alexeagle

alexeagle May 6, 2019

Contributor

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 comment has been minimized.

Copy link
@xingao267

xingao267 May 6, 2019

Author Contributor

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.

feat(bazel): use rbe_autoconfig() and new container.
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.

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

@xingao267 xingao267 requested a review from angular/tools-bazel as a code owner May 9, 2019

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

@alexeagle

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

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

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

alxhub added a commit that referenced this pull request May 9, 2019

feat(bazel): use rbe_autoconfig() and new container. (#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 #29336

@alxhub alxhub closed this in 9abf114 May 9, 2019

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

feat(bazel): use rbe_autoconfig() and new container. (angular#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.