fix: Use nisse-gradle-plugin#2464
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR migrates JBang's build versioning from the git-versioner plugin to the nisse JGit counting plugin. CI workflows are updated to fetch full git history, plugin infrastructure is configured in settings.gradle and build.gradle, version computation is set up with nisseConfig and afterEvaluate hooks, JAR manifests are updated with new attributes and lazy version injection, distribution tasks use lazy version providers, git tagging is replaced with a new tagVersion task, and version validation tests are added. ChangesVersioning System Migration: git-versioner to nisse JGit plugin
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
❌ Failed to create PR with unit tests: AGENT_CHAT: Failed to open pull request |
maxandersen
force-pushed
the
nisse-gradle-plugin
branch
from
3ca37d2 to
42c6442
Compare
|
Plugin is used, but something is still off, CI built JBang reports |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/test/java/dev/jbang/TestBuildVersion.java (2)
33-36: ⚡ Quick winConsider validating consistency between BuildConfig.VERSION and JBang-Version.
Both the
BuildConfig.VERSIONand the manifestJBang-Versionattribute should be set from the same nisse plugin output. Adding a consistency check would help catch configuration issues where they might diverge.♻️ Proposed additional assertion
String jbangVersion = manifest.getMainAttributes().getValue("JBang-Version"); assertNotNull(jbangVersion, "JBang-Version should be present in META-INF/MANIFEST.MF"); assertFalse(jbangVersion.equals("unspecified"), "JBang-Version in MANIFEST.MF should not be 'unspecified'"); + assertEquals(BuildConfig.VERSION, jbangVersion, + "JBang-Version in MANIFEST.MF should match BuildConfig.VERSION");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/dev/jbang/TestBuildVersion.java` around lines 33 - 36, Add an assertion in TestBuildVersion that the manifest JBang-Version matches the compiled BuildConfig.VERSION to ensure they come from the same nisse plugin output; locate the existing jbangVersion variable (from manifest.getMainAttributes().getValue("JBang-Version")) and add an assertEquals(BuildConfig.VERSION, jbangVersion, "BuildConfig.VERSION must match JBang-Version in MANIFEST.MF"), ensuring you keep the existing null/unspecified checks.
28-30: ⚡ Quick winConsider using JUnit Assumptions for better test semantics.
The early return makes this test silently pass when the manifest file doesn't exist. Using JUnit's
Assumptions.assumeTrue()would mark the test as skipped rather than passed, providing clearer test reporting about what was actually validated.♻️ Proposed refactor using Assumptions
+import static org.junit.jupiter.api.Assumptions.assumeTrue; + `@Test` void manifestContainsVersion() throws IOException { java.nio.file.Path manifestPath = java.nio.file.Paths.get("build/tmp/jar/MANIFEST.MF"); - if (!java.nio.file.Files.exists(manifestPath)) { - return; - } + assumeTrue(java.nio.file.Files.exists(manifestPath), + "Manifest file not yet generated - run jar task first"); try (InputStream is = java.nio.file.Files.newInputStream(manifestPath)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/dev/jbang/TestBuildVersion.java` around lines 28 - 30, Replace the silent early return in TestBuildVersion (the check using java.nio.file.Files.exists(manifestPath)) with a JUnit assumption so the test is skipped when the manifest is absent: use Assumptions.assumeTrue(...) (import org.junit.jupiter.api.Assumptions) with a clear message referencing the manifest, and keep the rest of the test logic unchanged so the presence of the manifest is required for the assertions to run.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build.gradle`:
- Around line 553-564: The tagVersion task always creates and pushes a git tag
(using tagName = 'v' + project.version) which fails on reruns; update task
tagVersion to first check for existence of tagName locally and remotely (e.g.,
via git tag -l or git ls-remote --tags origin) and only run the git tag and git
push commands when the tag does not already exist, ensuring the exec calls that
create/push the tag are skipped when tagName is present so the task becomes
idempotent for retries.
- Around line 247-253: The build should fail fast when the computed version is
the nisse fallback (e.g., 0.0.1-{count}) to avoid publishing with an invalid
baseline; add a validation step that examines the resolved project.version after
the nisseConfig.jgit settings (countingVersion, countingStartPatch,
appendSnapshot, countingPattern) and throws a GradleException if it matches the
fallback pattern (e.g., /^0\.0\.1-\d+$/) or if no semantic baseline tag was
found, and wire that validation task to run before publish/assemble tasks (or
alternatively set a proper versionHintPattern or create an initial semantic tag
like v0.1.0) so the build fails instead of silently publishing the fallback
version.
---
Nitpick comments:
In `@src/test/java/dev/jbang/TestBuildVersion.java`:
- Around line 33-36: Add an assertion in TestBuildVersion that the manifest
JBang-Version matches the compiled BuildConfig.VERSION to ensure they come from
the same nisse plugin output; locate the existing jbangVersion variable (from
manifest.getMainAttributes().getValue("JBang-Version")) and add an
assertEquals(BuildConfig.VERSION, jbangVersion, "BuildConfig.VERSION must match
JBang-Version in MANIFEST.MF"), ensuring you keep the existing null/unspecified
checks.
- Around line 28-30: Replace the silent early return in TestBuildVersion (the
check using java.nio.file.Files.exists(manifestPath)) with a JUnit assumption so
the test is skipped when the manifest is absent: use Assumptions.assumeTrue(...)
(import org.junit.jupiter.api.Assumptions) with a clear message referencing the
manifest, and keep the rest of the test logic unchanged so the presence of the
manifest is required for the assertions to run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 207d9dae-5ab8-4f7f-890b-df3e01069f5b
📒 Files selected for processing (5)
.github/workflows/main-build.yml.github/workflows/step-ci-build.ymlbuild.gradlesettings.gradlesrc/test/java/dev/jbang/TestBuildVersion.java
…test improvements - afterEvaluate now throws GradleException if version matches 0.0.1-x fallback - tagVersion checks if tag exists: skips if same SHA, fails if different SHA - manifestContainsVersion uses assumeTrue instead of silent return - Added consistency assertion between BuildConfig.VERSION and manifest JBang-Version
|
I pushed changes so jbang's build .gradle now deals with the tagging/ version setting that is not optimal but what's needed for now to enable use of worktrees with git. worth the extra maintainance. @cstamas it would be handy if nisse could do this but also feels outofbounds for nisse. I found https://github.com/ajoberstar/gradle-git but it got split up and now at sourcehut which is a mess to browse/look at thus not going for that now :) |
2 participants
Just to experiment.
To make it work, locally:
mvn installmain branch of https://github.com/maveniverse/nisse (this PR makes jbang build use local repo for plugins too, so it has to be installed for gradle to find it)Summary by CodeRabbit
Chores
Tests