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

Respect rules_java as the source of truth for java toolchains #18423

Closed
wants to merge 26 commits into from

Conversation

meteorcloudy
Copy link
Member

@meteorcloudy meteorcloudy commented May 16, 2023

Context: #18373

Currently the definitions of java tools, remote JDKs, and Java toolchains are duplicated in both Bazel sources and rules_java, and they go out of sync quite often. By default, the default toolchains shipped with the Bazel binary uses the one in Bazel sources except when Bzlmod is enabled.

bazelbuild/rules_java#110 syncs java toolchain related files from Bazel sources to rules_java and this PR removes most of the definitions in Bazel source and uses rules_java as the source of truth.

Significant changes in this PR:

  • Removed all java tools definitions from distdir_deps.bzl
  • Removed all remote JDK definitions from distdir_deps.bzl except the ones for to be embedded within Bazel binary.
  • Changed jdk.WORKSPACE.tmpl to load java tools and JDKs from rules_java as WORKSPACE suffix.
  • Removed the jdk_http_archives hack for some shell tests. That dates back to Test java coverage with java_tools released and built at head. #8361, I don't think it's needed since we have the WORKSPACE suffix by default.
  • When building Bazel with Bazel, we were using the java toolchains shipped in @bazel_tools of the Bazel binary, now we use the java toolchains from the rules_java defined in distdir_deps.bzl. This enables us to build Bazel itself with the latest java toolchain, therefore can detect issues earlier.
  • Removed _for_testing suffixes for the shared repo hack for Bazel CI since the java tools and remote JDKs versions we load to build Bazel are the same as the ones shipped in the Bazel binary we built (via jdk.WORKSPACE.tmpl) and used in the integration tests. They all come from the rules_java version in distdir_deps.bzl.
  • Removed the test_WORKSPACE_files hack for workspace_resolved_test.sh.
  • Mocked rules_java in some integration tests to avoid needing to download it or its transitive dependencies.

Breaking changes:

  • Loading .bzl files under @bazel_tools//tools/jdk in WORKSPACE now requires you to define rules_java in advance, the rules_java definition in WORKSPACE suffix cannot guarantee @rules_java exist because it's appended after the content of the user WORKSPACE file.

@meteorcloudy meteorcloudy requested a review from a team as a code owner May 16, 2023 20:13
@meteorcloudy meteorcloudy marked this pull request as draft May 16, 2023 20:13
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Remote-Exec Issues and PRs for the Execution (Remote) team team-Rules-Java Issues for Java rules labels May 16, 2023
@meteorcloudy meteorcloudy removed the team-Remote-Exec Issues and PRs for the Execution (Remote) team label May 17, 2023
@meteorcloudy meteorcloudy removed the request for review from a team May 17, 2023 12:30
@meteorcloudy meteorcloudy changed the title Respect rules java as the source of truth for java toolchains Respect rules_java as the source of truth for java toolchains May 17, 2023
@meteorcloudy meteorcloudy marked this pull request as ready for review May 22, 2023 11:58
@meteorcloudy meteorcloudy requested a review from a team as a code owner May 22, 2023 11:58
@meteorcloudy meteorcloudy requested review from katre and removed request for a team May 22, 2023 11:58
@github-actions github-actions bot added team-Configurability platforms, toolchains, cquery, select(), config transitions team-Remote-Exec Issues and PRs for the Execution (Remote) team labels May 22, 2023
@meteorcloudy meteorcloudy requested review from comius, hvadehra, aiuto and cushon and removed request for katre May 22, 2023 11:58
@meteorcloudy
Copy link
Member Author

/cc @zhengwei143 since you are working on JDK 20 migration. This PR still keeps the embedded JDK definitions in https://github.com/bazelbuild/bazel/blob/master/repositories.bzl so it should not impact the migration too much.

@meteorcloudy
Copy link
Member Author

@hvadehra @cushon @aiuto This PR is ready for review since we now have rules_java 6.0.0!

exports_files([
"BUILD.java_tools",
"java_stub_template.txt",
])

# Used to distinguish toolchains used for Java development, ie the JavaToolchainProvider.
toolchain_type(name = "toolchain_type")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't move the definition of those two targets because I'm not sure if it's going to be a major breaking change. Would an alias work if I move them to rules_java?

@meteorcloudy
Copy link
Member Author

Please review the internal CL instead!

@meteorcloudy
Copy link
Member Author

This PR is breaking many downstream projects: bazelbuild/rules_java#110 (comment)

copybara-service bot pushed a commit to protocolbuffers/protobuf that referenced this pull request Jun 1, 2023
copybara-service bot pushed a commit that referenced this pull request Jun 7, 2023
… try)

Rolling forward 975866a (which was rollbacked at d51c75f) with fixes.

- Introduce a `rules_java_builtin` repo in WORKSPACE prefix to avoid conflict with user defined rules_java.
    - `@bazel_tools//tools/jdk/*.bzl` loads from `rules_java_builtin` through repo-mappings.
    - `@local_jdk` was overridden in `jdk.WORKSPACE` to add repo_mapping for `rules_java`.
    - `jdk.WORKSPACE` explicitly loads from `rules_java_builtin` for JDK definitions and java toolchain definitions.
- Allow using `__SKIP_WORKSPACE_PREFIX__` and `__SKIP_WORKSPACE_SUFFIX__` in WORKSPACE comment.
- Fixed many tests by adjusting the WORKSPACE file content.
- Re-export more symbols from `@bazel_tools` to be backward-compatible.

Fixes #18551

Related:
- #18373
- bazelbuild/rules_java#110
- #18423

Closes #18558.

PiperOrigin-RevId: 538483417
Change-Id: I5223eec2c4b10131fc8c5b342237385ff2f56413
amishra-u pushed a commit to amishra-u/bazel that referenced this pull request Jun 7, 2023
… try)

Rolling forward bazelbuild@975866a (which was rollbacked at bazelbuild@d51c75f) with fixes.

- Introduce a `rules_java_builtin` repo in WORKSPACE prefix to avoid conflict with user defined rules_java.
    - `@bazel_tools//tools/jdk/*.bzl` loads from `rules_java_builtin` through repo-mappings.
    - `@local_jdk` was overridden in `jdk.WORKSPACE` to add repo_mapping for `rules_java`.
    - `jdk.WORKSPACE` explicitly loads from `rules_java_builtin` for JDK definitions and java toolchain definitions.
- Allow using `__SKIP_WORKSPACE_PREFIX__` and `__SKIP_WORKSPACE_SUFFIX__` in WORKSPACE comment.
- Fixed many tests by adjusting the WORKSPACE file content.
- Re-export more symbols from `@bazel_tools` to be backward-compatible.

Fixes bazelbuild#18551

Related:
- bazelbuild#18373
- bazelbuild/rules_java#110
- bazelbuild#18423

Closes bazelbuild#18558.

PiperOrigin-RevId: 538483417
Change-Id: I5223eec2c4b10131fc8c5b342237385ff2f56413
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions team-Remote-Exec Issues and PRs for the Execution (Remote) team team-Rules-Java Issues for Java rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant