Move java21 over so we can add java17 for debian13#1855
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @loosebazooka, 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!
This pull request introduces a structural refactoring of the Java 21 module definitions within the repository. By relocating Java 21-specific Bazel files into a dedicated "java_temurin" directory, the changes aim to create a clear separation and prepare the build system for the upcoming integration of Java 17, particularly for Debian 13 environments.
Highlights
- Java Module Path Refactoring: The configuration for Java 21 has been refactored by moving its related Bazel module files into a new "java_temurin" subdirectory.
- Build Script Updates: The "knife" utility and "update_java_archives.sh" script have been updated to reflect the new file paths for Java 21 module definitions.
- Preparation for Java 17: This refactoring prepares the repository to easily integrate Java 17 support for Debian 13, as indicated by the PR title.
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
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.
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 refactors the repository structure to move Java 21 related files into a dedicated directory, in preparation for adding support for Java 17. The changes correctly update all necessary paths in the build and script files.
My review focuses on improving the maintainability of the update scripts. While the file paths have been updated, the scripts themselves are still hardcoded for Java 21. I've provided suggestions to parameterize these scripts, which will make it easier to add support for other Java versions like Java 17 in the future.
knife
Outdated
| source scripts/update_java_archives.sh | ||
| old_version=$(get_java_version) | ||
| generate_java_archives > private/repos/java.MODULE.bazel | ||
| generate_java_archives > private/repos/java_temurin/java_21.MODULE.bazel |
There was a problem hiding this comment.
Now that the output path is specific to Java 21, the function cmd_update_java_archives and the command update-java-archives have become specific to Java 21 as well, because the sourced script scripts/update_java_archives.sh contains hardcoded logic for Java 21.
To align with the goal of supporting multiple Java versions, you could either:
- Parameterize
cmd_update_java_archivesand the functions inscripts/update_java_archives.shto accept a Java version. - Rename
cmd_update_java_archivestocmd_update_java21_archivesto make its specificity clear. If you choose this path, remember to also update the command in thecasestatement (line 213).
The second option is a smaller step that improves clarity and sets up for adding a similar command for Java 17 later.
scripts/update_java_archives.sh
Outdated
|
|
||
| function get_java_version() { | ||
| grep "#VERSION " ./private/repos/java.MODULE.bazel | cut -d" " -f2 | ||
| grep "#VERSION " ./private/repos/java_temurin/java_21.MODULE.bazel | cut -d" " -f2 |
There was a problem hiding this comment.
This path is now hardcoded for Java 21. This makes the function get_java_version specific to Java 21, even though its name is generic. The same applies to generate_java_archives and update_test_versions_java21 in this file.
To prepare for supporting multiple Java versions like Java 17, consider parameterizing these functions. For example, you could change get_java_version to:
function get_java_version() {
local java_version_string=$1 # e.g. "21"
grep "#VERSION " "./private/repos/java_temurin/java_${java_version_string}.MODULE.bazel" | cut -d" " -f2
}This would make the script more reusable and easier to maintain when you add support for Java 17.
|
🌳 🔄 Image Check |
loosebazooka
force-pushed
the
organize_java
branch
from
0ff833d to
1cfec21
Compare
Signed-off-by: Appu Goundan <appu@google.com>
loosebazooka
force-pushed
the
organize_java
branch
from
1cfec21 to
d9a6a45
Compare
| ) | ||
| use_repo(go_deps, "com_github_spdx_tools_golang") | ||
|
|
||
| ### JETTY ### |
There was a problem hiding this comment.
we don't do jetty builds anymore
| new_version=$(get_java_version 21) | ||
| update_test_versions_java21 $old_version $new_version | ||
|
|
||
| # java 17 set up |
There was a problem hiding this comment.
we don't actually have java17 builds yet for debian13, and we need to update this when we do.
| echo "" | ||
|
|
||
| local arch=$(uname -m) | ||
| if [ ${arch} == "x86_64" ]; then |
There was a problem hiding this comment.
uname works funny, so convert to debian arch names
2 participants
No description provided.