-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(gradle): ensure Gradle 9 compatibility in FilesTaskV2 #4472
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
base: master
Are you sure you want to change the base?
fix(gradle): ensure Gradle 9 compatibility in FilesTaskV2 #4472
Conversation
Address the removal of ProjectDependency.getDependencyProject() in Gradle 9 by introducing a version-aware resolution strategy. Fixes GoogleContainerTools#4469
Add a new test case testFilesTask_multiProjectComplexService_gradle9 to FilesTaskV2Test that explicitly runs against Gradle 9.0 using Gradle TestKit. This ensures that the reflection-based project resolution logic introduced for Gradle 9+ compatibility is verified. Verifies GoogleContainerTools#4469
Summary of ChangesHello @hpuac, 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 resolves a critical compatibility issue with Gradle 9.0+ where the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
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.
Code Review
This pull request effectively addresses the compatibility issue with Gradle 9.0 by introducing a version-aware mechanism for project dependency resolution using reflection. The changes are well-structured, with helper methods like getDependentProject() and isBeforeGradle9() improving code readability and maintainability. The inclusion of a new test case specifically for Gradle 9.0 is a great addition that ensures the fix is properly verified. I have one suggestion to make the exception handling more precise.
| String path = | ||
| (String) projectDependency.getClass().getMethod("getPath").invoke(projectDependency); | ||
| return getProject().project(path); | ||
| } catch (Exception ex) { |
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.
Catching the generic Exception is a bit too broad and can hide other potential runtime issues. It's better to catch the more specific ReflectiveOperationException, which is the superclass for exceptions thrown by reflective operations. This makes the error handling more robust and specific to the reflection calls being made.
| } catch (Exception ex) { | |
| } catch (ReflectiveOperationException ex) { |
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.
I applied your suggestion. Please verify that is solved and resolve this conversation.
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, apparently gemini code assist does not react to conversations the same way coderabbit does 😁
|
@hpuac Thanks for the fix. Do you know who we should ping to move the review forward? |
I'm also just an individual user blocked by the Gradle 9 incompatibility in JIB. I was hoping this to appear on some maintainers radar and be picked up 😅 |
|
Hi @hpuac, thanks for your contribution. Units on Java 8 and 11 are failing because gradle 9 requires Java 17+ (see workflow file). Is there a way to test this piece without using Gradle 9 runtime? |
@diegomarquezp I see two potential ways:
What do you think makes sense? Is there maybe a third alternative? |
Description
Address issue #4469 where the
_jibSkaffoldFilesV2task fails on Gradle 9.0+ due to the removal of theProjectDependency.getDependencyProject()API.Changes
getDependentProject()to handle project dependency resolution across versions. It uses the original API for Gradle < 9.0 and falls back to path-based resolution via reflection for Gradle 9.0+.isBeforeGradle9()helper method for better readability.FilesTaskV2Test.javathat explicitly runs against Gradle 9.0 using TestKit to verify the fix.Testing Verified
testFilesTask_multiProjectComplexService_gradle9on Java 17.Fixes #4469
Note
Local verification of all tests requires switching between Java 11 (for legacy Gradle) and Java 17 (for Gradle 9.0). CI should handle this matrix automatically.