Allow construction of a full jdk repo from a build target #17
Conversation
…lopers can create a java_runtime from a build target by constructing an appropriately laid-out jdk in a custom rule, and then point to that rule in the java attr.
|
Hm. How can I see the actual outputs of the failed tests please? (Running the tests locally results in errors accessing bcr.bazel.build, no route to host, which I'm trying to fix; I'd like to see if it's the same error remotely). Thank you! |
|
I think this mistakenly got categorized as requiring my response? |
|
Warning Rate limit exceeded@arvi18 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request modifies the java_runtime rule to allow the java attribute to accept a full JDK repository instead of just a single file. This is achieved by removing the allow_single_file = True restriction on the java attribute in java_runtime.bzl and changing ctx.file.java to ctx.executable.java. Additionally, a new integration test test_fully_generated_jdk is added to bazel_java_test.sh to verify this functionality.
Highlights
- Feature Enhancement: The
java_runtimerule is enhanced to support the construction of a full JDK repository from a build target, providing more flexibility in defining Java runtimes. - Code Modification: The
allow_single_filerestriction is removed from thejavaattribute in thejava_runtimerule definition. - Test Addition: A new integration test,
test_fully_generated_jdk, is added to verify the functionality of constructing a JDK from a build target.
Changelog
- src/main/starlark/builtins_bzl/common/java/java_runtime.bzl
- Changed
ctx.file.javatoctx.executable.javaon line 103 to allow accepting executable targets. - Removed
allow_single_file = Truefrom thejavaattribute definition on line 207.
- Changed
- src/test/shell/integration/bazel_java_test.sh
- Modified existing tests to assert for
dummy_javaanddummy_javacinstead offooandbaron lines 339 and 340. - Added a new test function
test_fully_generated_jdkon line 357 to test the full JDK generation functionality.
- Modified existing tests to assert for
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A runtime defined,
No longer confined,
A full JDK's might,
Brings build targets to light,
Flexibility aligned.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a significant change by allowing the java attribute in java_runtime to accept more than just a single file, enabling the construction of a full JDK repository from a build target. The changes include modifications to the java_runtime rule definition and the addition of a new test case to verify the functionality. The code appears to be well-structured and the new test case provides adequate coverage for the introduced changes.
Summary of Findings
- Attribute Type Change: The
javaattribute in thejava_runtimerule has been changed fromallow_single_file = Trueandattr.labeltoattr.executable. This change allows the attribute to accept a broader range of targets, enabling the construction of a full JDK repository from a build target. - Test Logic Modification: The test
test_genruleinbazel_java_test.shhas been modified to assert the presence ofdummy_javaanddummy_javacin the cquery output, reflecting the change in how the Java runtime is specified and used in genrules. - New Test Case: A new test case,
test_fully_generated_jdk, has been added tobazel_java_test.shto verify the functionality of constructing a full JDK from a build target. This test creates dummy Java executables and ajava_runtimerule to simulate a fully generated JDK.
Merge Readiness
The pull request appears to be well-structured and the new test case provides adequate coverage for the introduced changes. However, it's recommended to ensure that the changes don't introduce any regressions in existing functionality and that the new test case covers all relevant scenarios. I am unable to approve this pull request, and recommend that others review and approve this code before merging.
| @@ -100,7 +100,7 @@ def _java_runtime_rule_impl(ctx): | |||
| java_binary_exec_path = paths.get_relative(java_home, _get_bin_java(ctx)) | |||
| java_binary_runfiles_path = _get_runfiles_java_executable(ctx, java_home, ctx.label) | |||
|
|
|||
| java = ctx.file.java | |||
| java = ctx.executable.java | |||
There was a problem hiding this comment.
Consider adding a comment explaining why ctx.executable.java is now used instead of ctx.file.java. This will help future maintainers understand the change and its implications.
# Use ctx.executable.java to allow for a full jdk repo from a build target
java = ctx.executable.java
| expect_log "dummy_java" | ||
| expect_log "dummy_javac" |
There was a problem hiding this comment.
The log expectations have been updated to reflect the new dummy executables. It would be helpful to add a comment explaining why foo and bar are no longer expected and why dummy_java and dummy_javac are now expected. This will help understand the test logic and the changes introduced by the pull request.
| expect_log "dummy_java" | |
| expect_log "dummy_javac" | |
| # Test the genrule that specifically depends on :bar_runtime. | |
| # foo and bar are no longer expected because we are now using dummy_java and dummy_javac | |
| expect_log "dummy_java" | |
| expect_log "dummy_javac" |
Right now there seems to be no way to create a full java runtime from a build target. This adds that capability by removing the restriction that the 'java' attr in java_runtime be a single_file.