-
Notifications
You must be signed in to change notification settings - Fork 67
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
Migrate remote JDK / Java tools and add Bzlmod support #45
Conversation
After this, we only reference toolchain type targets under @bazel_tools//tools/jdk - @bazel_tools//tools/jdk:toolchain_type - @bazel_tools//tools/jdk:runtime_toolchain_type
FYI @Wyverald , to make Bzlmod support work, we need to export Update: Hmm, looks like the evaluation env of bzl file in WORKSPACE also doesn't have |
@comius This PR is a bit huge, it's better to review each commit. ;) |
@comius Do we still need the federation stuff in the WORKSPACE file? I need to pass DEFAULT_SYSTEM_JAVABASE to rules_java_toolchains, and the federation repo seems to block me from doing that. |
712b3b0
to
d624bcd
Compare
8493437
to
a38ab3a
Compare
I think you should drop it if it's used anywhere. I don't think it serves any purpose. |
b765185
to
ea5e9bb
Compare
java_tools_javac11_repos is not yet loaded because it breaks compatibility with Bazel 4.2.1`
3cb80ea
to
512b226
Compare
@comius I re-organized the commits to make them easier for review, also updated the PR description. PTAL~ |
], | ||
) | ||
|
||
def remote_jdk8_repos(): |
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.
Perhaps keep JDK8 remote repos. This is the only location where they are still defined.
And I was advertising on rules_appengine (which needs JDK8), this is the way to use it without installing JDK8 locally.
But we could also say we don't support JDK8 anymore.
Also, I was advertising that calling for example remote_jdk8_repos()
from the WORKSPACE will have the JDK8 toolchain ready - defined and registered. Perhaps the code needs to be reorganised a bit to keep this true in the no-bzlmod world.
The user base that's using rules_java and calls in repositories.bzl, is probably really small, though. So I'm not sure, how much is worth spending time on this reorganisation + JDK8.
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, I added remote_jdk8_repos back, also added toolchain registration in it, but I'm not invoking it by default in rules_java_dependencies. PTAL~.
@comius When is the next release for rules_java? |
I don't think they were ever released :) And also copybara looks broken - I know it wasn't a year ago. |
OK 😂, but can you tag a 4.1.0 or 5.0.0 version after merging this? So I don't have to cherry-pick those change into the Bazel Central Registry.
Should I send the changes via internal CL? Update: OK, I see, it's imported, will fix the presubmit issue. |
017f618
to
21e0453
Compare
Of course. I'll tag it 5.0.0.
Thanks! |
Ping @comius for tagging a 5.0.0 version ;) |
Sorry, done. |
Thanks! 🎉 |
Important changes in this PR:
local_java_repository
rule so that we don't need Bazel to exportDEFAULT_SYSTEM_JAVABASE
In Bzlmod, users automatically get all remote jdk dependencies and toolchains registered by depending on rules_java.
In old WORKSPACE way, users can do that by
If everyone migrate to this, we no longer need to append jdk.WORKSPACE as default workspace suffix.