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
Add --config=remote and --config=remote-cache modes for bazel #76815
Conversation
/assign @cblecker |
/retest |
@fejta this is an optional flag, right? It won't enable unless someone specifically adds the remote flag and specifies a remote build instance? |
Correct, this does nothing until someone adds a Also the toolchains and rbe platform are unevaluated until required (which right now only happens when you add those flags). So this is a big noop, but will make it easier to people (and our CI system) to start using remote execution. |
Works for me! I don't mind approving this for /approve |
build:remote --host_javabase=@rbe_default//java:jdk | ||
build:remote --javabase=@rbe_default//java:jdk | ||
build:remote --host_java_toolchain=@bazel_tools//tools/jdk:toolchain_hostjdk8 | ||
build:remote --java_toolchain=@bazel_tools//tools/jdk:toolchain_hostjdk8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's so special about these java args?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I don't understand this very deeply right now. There's some info at https://github.com/bazelbuild/bazel-toolchains/blob/master/rules/rbe_repo.bzl
I'm just copying the config the RBE team setup for test-infra.
I believe this configures the toolchain to use the tools that are present in remote containers.
build:remote --spawn_strategy=remote | ||
build:remote --strategy=Javac=remote | ||
build:remote --strategy=Closure=remote | ||
build:remote --strategy=Genrule=remote |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need these --strategy's if you already have --spawn_strategy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They look unnecessary given that --spawn_strategy value is the default and --strategy is used for action specific overrides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove them later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we can play around with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sent a mail asking for more details. Other changes will likely be necessary as a variety of tests fail when RBE is enabled. Will address this along with making those tests pass
build:remote --strategy=Javac=remote | ||
build:remote --strategy=Closure=remote | ||
build:remote --strategy=Genrule=remote | ||
build:remote --define=EXECUTOR=remote |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this special magic that something else understands or is this for our usage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure
build:remote-cache --remote_cache=remotebuildexecution.googleapis.com | ||
build:remote-cache --tls_enabled=true | ||
build:remote-cache --remote_timeout=3600 | ||
build:remote-cache --auth_enabled=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we only want --auth_enabled and --tls_enabled on remote-cache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a composition: build:remote --config=remote-cache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but the question is "why aren't --auth_enabled and --tls_enabled on --config=remote rather than the composite?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are. See L52. The remote config includes everything in remote-cache (not the reverse).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cblecker, fejta, mikedanese The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/kind feature |
/hold cancel Thanks so much! |
/retest Review the full test history for this PR. Silence the bot with an |
/assign @mikedanese
Enables remote execution on bazel (requires providing your own instance)
bazel test //pkg/... --config=remote \ --remote_instance_name=projects/AN_RBE_PROJ/instances/default_instance
--config=remote-cache
should allow us to simplify/improve our caching solution as well, potentially obviating greenhouse.Builds on similar work happening in test-infra: