bazelbuild / bazel-toolchains

Repository that hosts Bazel toolchain configs for remote execution and related support tools.
Apache License 2.0
186 stars 96 forks source link

Add support for Java platformization in Bazel@head #926

Closed comius closed 3 years ago

comius commented 3 years ago

Bazel is adding a better support for Java toolchains and as such this becomes less work for bazel-toolchains.

New flags that are needed in downstream project:

build:remote --java_runtime_version=rbe_jdk
build:remote --tool_java_runtime_version=rbe_jdk
build:remote --extra_toolchains=@rbe_default//java:all

This PR makes essentially two changes:

I had to move autodetection code into bazel-toolchains, because it has to be executed from repository rule context and additionally in the docker container. Otherwise in local Bazel instance the autodetection would run java directly from a repository rule.

I tested the change by compiling bazel@head with RBE on ubuntu8 and ubuntu11, and testing gerrit with RBE.

More information in https://docs.google.com/document/d/1MVbBxbKVKRJJY7DnkptHpvz7ROhyAYy4a-TZ-n7Q0r4/edit#

comius commented 3 years ago

cc @katre

comius commented 3 years ago

The failing tests are already failing at main branch: https://buildkite.com/bazel/bazel-toolchains/builds/22625. They were caused by release of Bazel 3.7.2. Configuration files need to be released.

comius commented 3 years ago

I'm wondering if some changes are also needed here to read the java_version from "checked-in" configs. I anticipate this might be needed after we generate new configs after this PR is merged. Also, this raises the question of backwards, compatibility, i.e., correctly defaulting to the right java_version when the java_version attribute doesn't exist in the configs vs reading it when it does.

The idea is that Bazel < 4.1.0 does not need the version. If the version is not configured (and no autodetection can be done) there are two options, default to "8" or use "unknown". In the first case, this seems to be the most used version everywhere, but might create a tech debt later. "unknown" would cause a gradual breakdown - that is using remote JDK to compile Java if possible.

BTW I noticed in one configuration, there is a java_home set, but there is no Java installed at all in the container. This is why when docker is not present I opted to return "unknown" - because I have no means to detect it. Alternative I was considering, was to guess Java version from java_home. But in this case it would cause weirder failures when Java compiler would be needed (and toolchain pointing to a missing path was configured).

I'm not super familiar with this config generation rule and I admit while reviewing this PR, I spent most of my time trying to figure out the various sources from which the config generator rules can get values for its attributes, the order of precedence in which it selects the value, how the values are persisted in checked-in configs and later used. However, this is just a comment on the state of the toolchain config generator rules and not a blocker for this PR.

Is there a playbook or a readme how this configs are updated? I guess I could do some extra tests manually.

smukherj1 commented 3 years ago

@comius

Is there a playbook or a readme how this configs are updated? I guess I could do some extra tests manually.

No playbooks :(. The target here is used to generate new configs and the target here automatically selects a checked in config for the bazel version running the build (doesn't work correctly for dev or RC bazel builds I think).

See https://github.com/bazelbuild/bazel-toolchains/pull/928 which added configs for 3.7.2. You might want to test locally by generating the new "version" parameter for Bazel 3.7.2 to see if the version is populated correctly in the generated configs and later read in.

Also, if you update your base branch, the CI failure at master should be fixed.

comius commented 3 years ago

I tested the release and the Java version is properly exported in the generated BUILD file.

comius commented 3 years ago

I rebased to master.

smukherj1 commented 3 years ago

/gcbrun

smukherj1 commented 3 years ago

/gcbrun

smukherj1 commented 3 years ago

/gcbrun