Conversation
Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com>
WalkthroughContext-aware Gradle handling is introduced across the Java external provider: Gradle wrapper resolution, version detection, and JAVA_HOME selection are centralized in service_client. Dependency parsing now ignores constraint entries and updates the regex. Gradle 9 is supported via task selection. Call sites set JAVA_HOME per invocation and pass context. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Provider as Provider.resolveSourcesJarsForGradle
participant Client as ServiceClient
participant Gradle as gradlew
Caller->>Provider: resolveSourcesJarsForGradle(ctx)
Provider->>Client: GetGradleWrapper()
Client-->>Provider: wrapperPath or error
Provider->>Client: GetGradleVersion(ctx)
Client->>Gradle: gradlew --version (try JAVA8_HOME, fallback JAVA_HOME)
Gradle-->>Client: version output
Client-->>Provider: parsed version
alt Gradle >= 9.0
Note over Provider: Use task-v9.gradle
else Gradle < 9.0
Note over Provider: Use legacy task file
end
Provider->>Client: GetJavaHomeForGradle(ctx)
Client-->>Provider: javaHome or error
Provider->>Gradle: gradlew <task> with env JAVA_HOME=javaHome
Gradle-->>Provider: result
Provider-->>Caller: sources jars resolution result
sequenceDiagram
autonumber
actor Caller
participant Dep as dependency.getDependenciesForGradle
participant Client as ServiceClient
participant Gradle as gradlew
Caller->>Dep: getDependenciesForGradle(ctx)
Dep->>Client: GetGradleWrapper() / GetJavaHomeForGradle(ctx)
Client-->>Dep: wrapperPath, javaHome
Dep->>Gradle: gradlew :dependencies (env JAVA_HOME=javaHome)
Gradle-->>Dep: output
Note over Dep: Parse, ignoring (n), (*), and (c) constraints
Dep-->>Caller: dependency graph
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
external-providers/java-external-provider/pkg/java_external_provider/dependency.go (2)
464-472: Use the central GetGradleWrapper helper (and Windows .bat support)This block reimplements wrapper resolution. Use the new helper to centralize logic (including Windows support) and consistent error handling.
Apply this diff:
- // get the graph output - exe, err := filepath.Abs(filepath.Join(p.config.Location, "gradlew")) - if err != nil { - return nil, fmt.Errorf("error calculating gradle wrapper path") - } - if _, err = os.Stat(exe); errors.Is(err, os.ErrNotExist) { - return nil, fmt.Errorf("a gradle wrapper must be present in the project") - } + // get the graph output + exe, err := p.GetGradleWrapper() + if err != nil { + return nil, err + }
500-551: Handle GetJavaHome error; use wrapper helper; add timeout and preserve env
- The error from GetJavaHomeForGradle is ignored.
- Reimplementing wrapper resolution duplicates logic (and breaks on Windows).
- No context/timeout; can hang in pathological cases.
- Environment clobbered.
Apply this diff:
func (c *javaServiceClient) getGradleSubprojects(ctx context.Context) ([]string, error) { args := []string{ "projects", } - javaHome, err := c.GetJavaHomeForGradle(ctx) + javaHome, err := c.GetJavaHomeForGradle(ctx) + if err != nil { + return nil, err + } - exe, err := filepath.Abs(filepath.Join(c.config.Location, "gradlew")) - if err != nil { - return nil, fmt.Errorf("error calculating gradle wrapper path") - } - if _, err = os.Stat(exe); errors.Is(err, os.ErrNotExist) { - return nil, fmt.Errorf("a gradle wrapper must be present in the project") - } - cmd := exec.Command(exe, args...) - cmd.Dir = c.config.Location - cmd.Env = append(cmd.Env, fmt.Sprintf("JAVA_HOME=%s", javaHome)) + exe, err := c.GetGradleWrapper() + if err != nil { + return nil, err + } + timeoutCtx, cancel := context.WithTimeout(ctx, 2*time.Minute) + defer cancel() + cmd := exec.CommandContext(timeoutCtx, exe, args...) + cmd.Dir = c.config.Location + cmd.Env = append(os.Environ(), fmt.Sprintf("JAVA_HOME=%s", javaHome)) output, err := cmd.CombinedOutput() if err != nil { return nil, err }
🧹 Nitpick comments (1)
external-providers/java-external-provider/pkg/java_external_provider/dependency.go (1)
600-608: Regex change looks correct; ensure it doesn’t regress on edge casesThe updated pattern captures both “:” and “ -> ” separators and shifts the version group. This should handle “X:+ -> Y.Z” cases better. Keep an eye on exotic lines with additional qualifiers.
If you want, I can add focused parser tests covering:
- org:lib:1.2.3
- org:lib:1.+ -> 1.2.3
- local module: “:lib (n)”, “:lib (c)”
- org:lib:{strictly 1.0.1} -> 1.0.1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
external-providers/java-external-provider/pkg/java_external_provider/dependency.go(4 hunks)external-providers/java-external-provider/pkg/java_external_provider/provider.go(3 hunks)external-providers/java-external-provider/pkg/java_external_provider/service_client.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-30T12:11:45.673Z
Learnt from: pranavgaikwad
PR: konveyor/analyzer-lsp#859
File: external-providers/java-external-provider/pkg/java_external_provider/dependency.go:694-694
Timestamp: 2025-07-30T12:11:45.673Z
Learning: In the Java external provider dependency walker (external-providers/java-external-provider/pkg/java_external_provider/dependency.go), errors from toDependency function calls should be ignored as they are not considered important by the maintainers.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/provider.goexternal-providers/java-external-provider/pkg/java_external_provider/service_client.goexternal-providers/java-external-provider/pkg/java_external_provider/dependency.go
🧬 Code Graph Analysis (1)
external-providers/java-external-provider/pkg/java_external_provider/dependency.go (1)
provider/provider.go (2)
DepDAGItem(474-474)Dep(473-473)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: test (windows-latest)
- GitHub Check: test (macos-latest)
🔇 Additional comments (1)
external-providers/java-external-provider/pkg/java_external_provider/dependency.go (1)
587-599: Good call: ignore Gradle “constraint” entries (c) in dependency parsingThis aligns the parser with Gradle output semantics and avoids polluting the DAG with non-dependencies.
| javaHome, err := p.GetJavaHomeForGradle(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| cmd := exec.CommandContext(timeout, exe, args...) | ||
| cmd.Dir = p.config.Location | ||
| cmd.Env = append(cmd.Env, fmt.Sprintf("JAVA_HOME=%s", javaHome)) | ||
| output, err := cmd.CombinedOutput() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve environment when setting JAVA_HOME for Gradle run
Same environment clobbering issue here: PATH and other variables are dropped.
Apply this diff:
cmd := exec.CommandContext(timeout, exe, args...)
cmd.Dir = p.config.Location
- cmd.Env = append(cmd.Env, fmt.Sprintf("JAVA_HOME=%s", javaHome))
+ cmd.Env = append(os.Environ(), fmt.Sprintf("JAVA_HOME=%s", javaHome))🤖 Prompt for AI Agents
In
external-providers/java-external-provider/pkg/java_external_provider/dependency.go
around lines 476 to 483, the code sets cmd.Env directly which drops the existing
process environment (including PATH); change it to preserve the current
environment and only override/add JAVA_HOME by setting cmd.Env =
append(os.Environ(), fmt.Sprintf("JAVA_HOME=%s", javaHome)) before running
cmd.CombinedOutput(), so PATH and other vars are retained while JAVA_HOME is set
for the Gradle run.
There was a problem hiding this comment.
We don't necessarily want to preserve the environment variables. In this case I don't think this is needed.
There was a problem hiding this comment.
@jmle Thank you for the clarification! You're right that preserving the full environment isn't necessary here, especially if you want to maintain a controlled/isolated environment for the Gradle execution. The current approach with a minimal environment setup makes sense for this use case.
🧠 Learnings used
Learnt from: pranavgaikwad
PR: konveyor/analyzer-lsp#859
File: external-providers/java-external-provider/pkg/java_external_provider/dependency.go:694-694
Timestamp: 2025-07-30T12:11:45.673Z
Learning: In the Java external provider dependency walker (external-providers/java-external-provider/pkg/java_external_provider/dependency.go), errors from toDependency function calls should be ignored as they are not considered important by the maintainers.
| // obtain Gradle version, needed for compatibility checks | ||
| gradleVersion, err := s.GetGradleVersion(ctx) | ||
| if err != nil { | ||
| gradleVersion = version.Version{} | ||
| } | ||
|
|
||
| // append downloader task | ||
| if taskFile == "" { | ||
| // if taskFile is empty, we are in container mode | ||
| taskFile = "/usr/local/etc/task.gradle" | ||
| } | ||
| // if Gradle >= 9.0, use a newer script for downloading sources | ||
| gradle9version, _ := version.NewVersion("9.0") | ||
| if gradleVersion.GreaterThanOrEqual(gradle9version) { | ||
| taskFile = filepath.Join(filepath.Dir(taskFile), "task-v9.gradle") | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against version parsing failures and only select task-v9.gradle when version is known ≥ 9
Currently, if version parsing fails you assign a zero-value version and still compare it, which can be unsafe. Only perform the comparison when you have a valid version; otherwise default to the legacy task file.
Apply this diff:
// obtain Gradle version, needed for compatibility checks
- gradleVersion, err := s.GetGradleVersion(ctx)
- if err != nil {
- gradleVersion = version.Version{}
- }
+ gradleVersion, err := s.GetGradleVersion(ctx)
+ if err != nil {
+ s.log.V(5).Info("could not determine Gradle version; using default task file", "error", err)
+ }
@@
- // if Gradle >= 9.0, use a newer script for downloading sources
- gradle9version, _ := version.NewVersion("9.0")
- if gradleVersion.GreaterThanOrEqual(gradle9version) {
- taskFile = filepath.Join(filepath.Dir(taskFile), "task-v9.gradle")
- }
+ // if Gradle >= 9.0, use a newer script for downloading sources
+ if err == nil {
+ gradle9version, _ := version.NewVersion("9.0")
+ if gradleVersion.GreaterThanOrEqual(gradle9version) {
+ taskFile = filepath.Join(filepath.Dir(taskFile), "task-v9.gradle")
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // obtain Gradle version, needed for compatibility checks | |
| gradleVersion, err := s.GetGradleVersion(ctx) | |
| if err != nil { | |
| gradleVersion = version.Version{} | |
| } | |
| // append downloader task | |
| if taskFile == "" { | |
| // if taskFile is empty, we are in container mode | |
| taskFile = "/usr/local/etc/task.gradle" | |
| } | |
| // if Gradle >= 9.0, use a newer script for downloading sources | |
| gradle9version, _ := version.NewVersion("9.0") | |
| if gradleVersion.GreaterThanOrEqual(gradle9version) { | |
| taskFile = filepath.Join(filepath.Dir(taskFile), "task-v9.gradle") | |
| } | |
| // obtain Gradle version, needed for compatibility checks | |
| gradleVersion, err := s.GetGradleVersion(ctx) | |
| if err != nil { | |
| s.log.V(5).Info("could not determine Gradle version; using default task file", "error", err) | |
| } | |
| // append downloader task | |
| if taskFile == "" { | |
| // if taskFile is empty, we are in container mode | |
| taskFile = "/usr/local/etc/task.gradle" | |
| } | |
| // if Gradle >= 9.0, use a newer script for downloading sources | |
| if err == nil { | |
| gradle9version, _ := version.NewVersion("9.0") | |
| if gradleVersion.GreaterThanOrEqual(gradle9version) { | |
| taskFile = filepath.Join(filepath.Dir(taskFile), "task-v9.gradle") | |
| } | |
| } |
🤖 Prompt for AI Agents
In
external-providers/java-external-provider/pkg/java_external_provider/provider.go
around lines 613 to 629, the code unconditionally uses a zero-value
gradleVersion when parsing fails and then compares it to 9.0; change this so you
only choose task-v9.gradle when version parsing succeeded and the parsed version
is >= 9.0. Specifically, keep taskFile defaulting to container path when empty,
then call GetGradleVersion and if err == nil (or other success indicator)
perform the GreaterThanOrEqual check against version.NewVersion("9.0") and only
then reassign taskFile to task-v9.gradle; if parsing failed, leave the legacy
taskFile unchanged.
| func (s *javaServiceClient) GetGradleWrapper() (string, error) { | ||
| exe, err := filepath.Abs(filepath.Join(s.config.Location, "gradlew")) | ||
| if err != nil { | ||
| return "", fmt.Errorf("error calculating gradle wrapper path") | ||
| } | ||
| if _, err = os.Stat(exe); errors.Is(err, os.ErrNotExist) { | ||
| return "", fmt.Errorf("a gradle wrapper is not present in the project") | ||
| } | ||
| return exe, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Support Windows gradlew and preserve underlying errors
The wrapper resolution hardcodes "gradlew" and hides the underlying error. On Windows the wrapper is "gradlew.bat", and the current implementation will fail. Also, return a wrapped error for easier diagnosis.
Apply this diff:
func (s *javaServiceClient) GetGradleWrapper() (string, error) {
- exe, err := filepath.Abs(filepath.Join(s.config.Location, "gradlew"))
+ wrapper := "gradlew"
+ // On Windows, the wrapper is gradlew.bat
+ if runtime.GOOS == "windows" {
+ wrapper = "gradlew.bat"
+ }
+ exe, err := filepath.Abs(filepath.Join(s.config.Location, wrapper))
if err != nil {
- return "", fmt.Errorf("error calculating gradle wrapper path")
+ return "", fmt.Errorf("error calculating gradle wrapper path: %w", err)
}
if _, err = os.Stat(exe); errors.Is(err, os.ErrNotExist) {
return "", fmt.Errorf("a gradle wrapper is not present in the project")
}
return exe, err
}Add the missing import at the top of this file:
import "runtime"🤖 Prompt for AI Agents
In
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
around lines 457-466, the function hardcodes "gradlew" and discards underlying
errors; change it to pick the correct wrapper name based on runtime.GOOS (use
"gradlew.bat" on Windows, "gradlew" otherwise), add the required runtime import,
and preserve and wrap underlying errors with fmt.Errorf(... %w ...) when
filepath.Abs or os.Stat fail; ensure the function returns the resolved exe and
nil on success and wraps/statements return the original error using %w for
diagnosis.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
demo-output.yaml (2)
1162-1177: Confirm stability of adding build/download artifacts to golden outputThese entries point to Gradle-generated files under build/download/*.jar. If this file is used as a golden snapshot in tests, the presence of build outputs can be nondeterministic across Gradle versions, repositories, and network availability. Recommend either:
- Documenting their purpose in the message field, or
- Gating generation to ensure determinism, or
- Moving such volatile paths into a separate “non-deterministic artifacts” section that tests can ignore.
Minimal, low-impact improvement by annotating the message so future readers know why these appear:
- - uri: file:///examples/gradle-multi-project-example/build/download/animal-sniffer-annotations-1.14-sources.jar - message: "" + - uri: file:///examples/gradle-multi-project-example/build/download/animal-sniffer-annotations-1.14-sources.jar + message: "Gradle: downloaded sources JAR discovered during dependency resolution (may vary by Gradle version/repositories)." - - uri: file:///examples/gradle-multi-project-example/build/download/error_prone_annotations-2.0.18-sources.jar - message: "" + - uri: file:///examples/gradle-multi-project-example/build/download/error_prone_annotations-2.0.18-sources.jar + message: "Gradle: downloaded sources JAR discovered during dependency resolution (may vary by Gradle version/repositories)." - - uri: file:///examples/gradle-multi-project-example/build/download/guava-23.0-sources.jar - message: "" + - uri: file:///examples/gradle-multi-project-example/build/download/guava-23.0-sources.jar + message: "Gradle: downloaded sources JAR discovered during dependency resolution (may vary by Gradle version/repositories)." - - uri: file:///examples/gradle-multi-project-example/build/download/j2objc-annotations-1.1-sources.jar - message: "" + - uri: file:///examples/gradle-multi-project-example/build/download/j2objc-annotations-1.1-sources.jar + message: "Gradle: downloaded sources JAR discovered during dependency resolution (may vary by Gradle version/repositories)." - - uri: file:///examples/gradle-multi-project-example/build/download/log4j-api-2.9.1-sources.jar - message: "" + - uri: file:///examples/gradle-multi-project-example/build/download/log4j-api-2.9.1-sources.jar + message: "Gradle: downloaded sources JAR discovered during dependency resolution (may vary by Gradle version/repositories)." - - uri: file:///examples/gradle-multi-project-example/build/download/log4j-core-2.9.1-sources.jar - message: "" + - uri: file:///examples/gradle-multi-project-example/build/download/log4j-core-2.9.1-sources.jar + message: "Gradle: downloaded sources JAR discovered during dependency resolution (may vary by Gradle version/repositories)." - - uri: file:///examples/gradle-multi-project-example/build/download/log4j-slf4j-impl-2.9.1-sources.jar - message: "" + - uri: file:///examples/gradle-multi-project-example/build/download/log4j-slf4j-impl-2.9.1-sources.jar + message: "Gradle: downloaded sources JAR discovered during dependency resolution (may vary by Gradle version/repositories)." - - uri: file:///examples/gradle-multi-project-example/build/download/slf4j-api-1.7.25-sources.jar - message: "" + - uri: file:///examples/gradle-multi-project-example/build/download/slf4j-api-1.7.25-sources.jar + message: "Gradle: downloaded sources JAR discovered during dependency resolution (may vary by Gradle version/repositories)."
1162-1177: Keep ordering deterministic to avoid flaky fixturesThe eight new entries look alphabetically ordered by artifact. Please ensure the generator enforces a stable sort (e.g., by URI) so re-runs across environments/Gradle versions don’t reshuffle entries and cause noisy diffs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
demo-output.yaml(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/service_client.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- external-providers/java-external-provider/pkg/java_external_provider/service_client.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test
- GitHub Check: test (macos-latest)
- GitHub Check: test (windows-latest)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: benchmark (windows-latest, windows)
🔇 Additional comments (2)
demo-output.yaml (2)
1162-1177: LGTM: Adds useful Gradle context to demo outputThe added Gradle sources artifacts help demonstrate the enhanced Gradle handling described in the PR. Content and indentation are consistent with the surrounding YAML.
1162-1177: No duplicate URIs detected in demo-output.yamlI ran a search across the entire file for each of the eight build/download URIs and confirmed that each appears exactly once (lines 1162, 1164, 1166, 1168, 1170, 1172, 1174, 1176). No additional occurrences were found, so deduplication isn’t necessary.
Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com>
Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com>
2 participants
https://issues.redhat.com/browse/MTA-5907
Requires:
Fixes:
Bundle PR: 142
Summary by CodeRabbit
New Features
Bug Fixes