Skip to content

Conversation

@MartelliEnrico
Copy link
Contributor

@MartelliEnrico MartelliEnrico commented Oct 4, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Corrects module-path handling during runtime image creation to ensure modules are located reliably.
  • Refactor

    • Simplifies command execution by using standard execution helpers and streamlines argument handling.
  • Chores

    • Unifies output handling (error routed to standard output) and logs results at an appropriate level based on exit status.
    • Adds an option to ignore modified runtime during image creation.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 4, 2025

📝 Walkthrough

Walkthrough

Refactors JlinkTask to use File-based paths and Gradle ExecOperations for command execution; updates argument construction, module-path handling, and consolidates stdout/stderr capture. Adjusts logging to emit a single message on non‑zero exit and prints captured output conditionally. Also adds the --ignore-modified-runtime option to the jlink task in build.gradle. Keeps final exit assertion via assertNormalExitValue().

Changes

Cohort / File(s) Summary
Exec invocation and logging refactor
buildSrc/src/main/java/com/github/stickerifier/stickerify/JlinkTask.java
Switched to File getters for jlink and jmods, replaced manual command list with Gradle ExecOperations using commandLine/args, used jmods.getAbsolutePath() when including module-path, routed stderr into stdout, consolidated output capture into a single stream, logged once with level based on exit code, retained result.assertNormalExitValue().
Task option change
build.gradle
Added --ignore-modified-runtime to the jlink task options; other task configuration unchanged.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant T as JlinkTask
  participant FS as FileSystem
  participant Exec as Gradle ExecOperations
  participant Log as Logger

  T->>FS: Resolve `jlink` binary (File) and `jmods` dir (File)
  T->>T: Build args (module-path using jmods.getAbsolutePath(), add-modules, output, + --ignore-modified-runtime)
  T->>Exec: exec(commandLine: jlink, args: ...)
  activate Exec
  Exec-->>T: Exit code and combined stdout/stderr
  deactivate Exec

  alt exit code == 0
    T->>Log: info(output if present)
  else exit code != 0
    T->>Log: error("jlink exited with code X") 
    T->>Log: error(output if present)
  end
  T->>T: result.assertNormalExitValue()
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I nibbled paths from Path to File with care,
Gathered args in bundles, tidy as a lair.
I funnelled every whisper to a single stream,
Checked the hop of exit, then celebrated the seam.
Hop! A tiny build feast, carrot-sweet and fair. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly describes the primary focus of the changeset by indicating that improvements were made to the Jlink task, which aligns directly with updates in JlinkTask.java and build.gradle for enhanced command execution and error handling.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8332c11 and 146b391.

📒 Files selected for processing (2)
  • build.gradle (1 hunks)
  • buildSrc/src/main/java/com/github/stickerifier/stickerify/JlinkTask.java (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
buildSrc/src/main/java/com/github/stickerifier/stickerify/JlinkTask.java (1)
buildSrc/src/main/java/com/github/stickerifier/stickerify/JlinkJavaLauncher.java (3)
  • JlinkJavaLauncher (10-24)
  • Override (20-23)
  • JlinkJavaLauncher (11-13)
build.gradle (1)
buildSrc/src/main/java/com/github/stickerifier/stickerify/JlinkJavaLauncher.java (2)
  • JlinkJavaLauncher (10-24)
  • JlinkJavaLauncher (11-13)
⏰ 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). (4)
  • GitHub Check: Execute unit tests (ubuntu-latest)
  • GitHub Check: Execute unit tests (windows-latest)
  • GitHub Check: Execute unit tests (macos-13)
  • GitHub Check: Build and upload Docker image
🔇 Additional comments (5)
buildSrc/src/main/java/com/github/stickerifier/stickerify/JlinkTask.java (4)

65-66: LGTM! Good use of Gradle APIs.

The change to use getAsFile() for obtaining File objects is correct and aligns with Gradle best practices. This approach is more explicit and integrates well with the ExecOperations refactor below.


76-84: LGTM! Cleaner command execution pattern.

The refactor to use Gradle's ExecOperations API with args() is a good improvement over manual command construction. The argument order is correct for jlink, and using getAbsolutePath() ensures proper platform-specific path handling.


87-87: Note the behavioural change in output handling.

Redirecting stderr to stdout consolidates all output for simplified handling, which aligns well with the single consolidated log message approach below. However, be aware that this makes it impossible to distinguish between standard output and error output from jlink if debugging is needed.

Given the exit value checking and appropriate log level selection (lines 92-93), this trade-off seems reasonable for this task.


92-93: LGTM! Improved logging with appropriate levels.

The conditional log level selection based on exit value provides better feedback to users. Logging at INFO level for successful execution and ERROR level for failures is appropriate and makes it easier to identify issues in build logs.

build.gradle (1)

44-44: Verify necessity of --ignore-modified-runtime

  • Confirm why this flag is needed and under which scenarios the runtime image is legitimately modified.
  • Evaluate whether it could mask configuration issues that ought to be addressed instead.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@MartelliEnrico MartelliEnrico force-pushed the jlink-task-improvements branch from 8332c11 to c0594c5 Compare October 4, 2025 09:02
@rob93c rob93c merged commit 647f849 into main Oct 4, 2025
6 checks passed
@rob93c rob93c deleted the jlink-task-improvements branch October 4, 2025 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants