Repository navigation
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
🔄 Backport BotPort PRs:
|
gnodet-bot
left a comment
There was a problem hiding this comment.
✅ Clean, well-tested fix. The PR correctly skips sources JARs in both launcher scripts using idiomatic constructs for each platform.
Shell fix (camel.sh): Uses a POSIX case builtin — clean and portable.
Batch fix (camel.bat): Uses findstr /e with the pipe directly after the echo expression (no trailing space). This is the correct form: a space before | would be passed to findstr and break the end-of-line match.
Tests: Thorough coverage including paths with spaces (camel sh in directory with space, camel bat with a space in the path).
Minor style note: The new test methods in CamelBatDiscoveryTest and CamelShDiscoveryTest import org.assertj.core.api.Assertions.assertThat while the existing tests use JUnit 5 assertEquals/assertTrue/assertFalse. Both work fine, but mixing assertion libraries in the same file is a minor inconsistency — consider aligning with whichever style is prevalent in the surrounding test class (JUnit 5 assertions are already used throughout).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @Croway. The fix itself is small and correct. What I checked:
camel.sh: the glob sortscamel-launcher-X-sources.jarbeforecamel-launcher-X.jar(-sorts before.), and the loop takes the first match. That is the bug, and thecaseskip fixes it. The new test setsLC_ALL=C, so it fails without the fix.camel.bat: before, the last match won. Now the first jar that is not a sources jar wins, which matchescamel.sh. Thezzz-sourcesfixture covers the old "last wins" order, so the Windows test would also fail without the fix.- Both tests use an install path with spaces.
Questions and suggestions (none blocking):
- Where does the sources jar come from? The published archives don't have one.
camel-launcher-4.22.1-bin.tar.gzon Maven Central and the latest4.23.0-SNAPSHOTbin archive only containcamel-launcher-<version>.jarand the Jolokia agent inbin/. The assembly inlauncher-content.xmlcopies${project.build.directory}/*.jar(onlyoriginal-*.jaris excluded). So I guess a local build with a profile that attaches sources (source-jarordeploy) puts the sources jar intarget/before the assembly runs. If so, also excluding*-sources.jarand*-javadoc.jarin that fileSet would fix the root cause: the archive would not carry those jars at all. The script change is still a good guard. -javadoc.jar: Maven Central hascamel-launcher-<version>-javadoc.jartoo, and it sorts before the main jar in the same way. Consider skipping it in both scripts as well (see inline).- The Windows change is not run by CI.
CamelBatDiscoveryTestis@EnabledOnOs(WINDOWS). Thewindows-validatorjob inpackage-native-validation.ymlonly runsWebsiteInstallTest, and the nightly Windows build uses-Dquickly. So it passed there without running the new test, and the PR description says the Windows tests were skipped locally. Has someone runCamelBatDiscoveryTeston Windows? Adding it to the-Dtest=list ofwindows-validatorwould cover this script from now on. - Assertion style (gnodet-bot already raised this, and I agree): both test classes use JUnit assertions only. CLAUDE.md asks to keep the style of the module, so
assertEquals/assertTrue(r.stdout().contains(...))would fit better than adding AssertJ to these two files. - No JIRA: a small JIRA would help track this, since there is also a 4.22.x port (#27827). If the released archives never contain a sources jar (point 1), I am not sure the port is needed.
CI: the Windows and macOS validators passed. The main build is still running.
Checked against the project conventions in CLAUDE.md. This is a rules-and-conventions review and does not replace CodeRabbit, Sourcery or SonarCloud.
Claude Code on behalf of @davsclaus.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 1 of 703 tested, 0 compile-only — current: 1 all testedMaveniverse Scalpel detected 1 affected modules (current approach: 1). Skip-tests mode would test 1 modules (1 direct + 1 downstream), skip tests for 0 (generated code, meta-modules) Modules Scalpel would test (1)
💡 Manual integration tests recommended:
All tested modules (2 modules, 44.3s total)Total reactor time: 44.3s
Top 20 slowest modules:
|
|
Addressed the javadoc suggestions in both scripts and excluded both classifier JARs from shared assembly content. Regression tests include sources/javadoc files sorting before and after the executable JAR and paths with spaces. The new tests now use the surrounding JUnit assertion style. The reported Red Hat downstream distribution does contain a sources JAR in
The CI changes use the pinned Maven wrapper and enable AI-generated by Codex on behalf of @Croway. |
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the two new commits address davsclaus's feedback completely.
What changed:
*-javadoc.jaris now also excluded in bothcamel.sh(case guard extended) andcamel.bat(second/c:argument tofindstr), and from the assembly (launcher-content.xml).CamelBatDiscoveryTestandFakeJavaTestare now included in thewindows-validatorCI step, closing the coverage gap davsclaus flagged.- The
REAL_VERSIONstep in the workflow now uses./mvnw(consistent with the rest) and adds-P apache-snapshots, which is correct since that step runs from a sub-directory (cd dsl/camel-jbang/camel-launcher) and resolves snapshot artifacts.
Correction to my earlier review: I incorrectly claimed the new test methods use org.assertj.core.api.Assertions.assertThat. They don't — both CamelShDiscoveryTest and CamelBatDiscoveryTest use JUnit 5 assertEquals/assertTrue/assertFalse throughout, consistent with the surrounding test class style. My apologies for the noise.
Pre-fix test validation: ✅ Static trace confirms the tests were failing before the fix. FakeJava.newLauncherHome() creates camel-launcher-9.9.9.jar as a stub, then the test adds classifier JARs on top. With LC_ALL=C, - (ASCII 45) sorts before . (ASCII 46), so camel-launcher-9.9.9-sources.jar is picked before camel-launcher-9.9.9.jar without the case guard — the assertTrue(stdout.contains("camel-launcher-9.9.9.jar")) assertion would fail. With the fix, the guard skips all classifier JARs and the stub JAR is selected correctly.
LGTM. The fix is layered correctly (assembly exclusion + runtime guard), well-tested on both platforms, and CI now covers both scripts.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
The latest Windows run selected the executable JAR correctly but exposed a doubled separator in its path, failing both the classifier regression and the existing argument-preservation test. The batch script now stores The 4.22.x Ubuntu packaging step still invoked runner Maven through a child script. The workflow now pins the wrapper distribution's Maven on PATH and propagates the snapshot profile into subprocesses. The Maven-home lookup and subprocess version selection were validated locally, and workflow syntax checks pass. Fresh CI is running for these changes. The separate 4.22.x Java 17 failure was a Maven Central HTTP 502 while downloading GraalVM, rather than a test assertion failure. AI-generated by Codex on behalf of @Croway. |
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the 4th commit is clean.
%%~fi normalization (camel.bat): Correct fix. %%~fi strips the surrounding quotes inherited from the glob pattern "%BASEDIR%\camel-launcher-*.jar" and expands to a fully-qualified canonical path, eliminating the doubled backslash when BASEDIR ends with \. The %%~nxi modifier on the echo/findstr line still correctly extracts just the filename for pattern matching — these two modifiers serve different roles and are both right.
Maven wrapper pinning (package-native-validation.yml): The Pin Maven for packaging subprocesses step is correctly scoped to posix-validator only. The windows-validator job doesn't invoke packaging scripts that call bare mvn; all its Maven calls are explicit ./mvnw or ../../../mvnw with -P apache-snapshots passed directly. The MAVEN_ARGS accumulation is a non-issue — the step runs exactly once per job and duplicate profile activation is a harmless no-op in Maven.
All mvn → ./mvnw migrations are complete in both jobs. Test class assignments are correctly split: CamelShDiscoveryTest in posix-validator, CamelBatDiscoveryTest in windows-validator, FakeJavaTest in both.
LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
Windows validation now passes on the latest commit: all 24 CamelBatDiscoveryTest cases and all 3 FakeJavaTest cases passed, including the classifier-selection regression and the existing argument-preservation test that previously failed. The full Windows validator and macOS validator are green. Windows job. Ubuntu and the general build jobs are still queued; the 4.22.x Windows installer suite is still running. AI-generated by Codex on behalf of @Croway. |
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @Croway, and thanks for chasing the Windows run until it was green. I looked at what changed since my approval (bdbcb46).
I went through my previous points:
-
- Root cause in the assembly: fixed.
launcher-content.xmlnow excludes*-sources.jarand*-javadoc.jar, so the archives cannot carry them.
- Root cause in the assembly: fixed.
-
-javadoc.jar: fixed incamel.sh,camel.batand the assembly, with fixtures that sort before and after the main jar.
-
- Windows script not run by CI: fixed.
windows-validatornow runsCamelBatDiscoveryTestandFakeJavaTest, and the latest run passed all 24 + 3 tests there. The POSIX job runsCamelShDiscoveryTestandFakeJavaTesttoo.
- Windows script not run by CI: fixed.
-
- Assertion style: fixed, the new tests use the JUnit assertions of the surrounding classes.
-
- No JIRA: still open, see below.
What changed since my approval:
camel.bat:set "LAUNCHER_JAR=%%~fi"instead of%%i.BASEDIRcomes from%~dp0and ends with\, so"%BASEDIR%\camel-launcher-*.jar"gave a path with a doubled separator.%%~figives the normalized full path and drops any quotes. The strict path assertion in the tests did not change, and it passes on Windows now. Good fix.package-native-validation.yml: a new step puts the wrapper's MavenbinonPATHand adds-P apache-snapshotstoMAVEN_ARGS. This is needed becausecamel-package.shandcamel-validate.shcall plainmvn. The ubuntu log showsMAVEN_ARGS: -ntp -P apache-snapshotsin the later steps, and both POSIX validators passed.
New findings:
- (non-blocking) The pin step does not check what
help:evaluateprinted. If that output ever has an extra line, a wrong directory goes onPATHwithout an error, and the scripts fall back to the runner'smvnagain. Atest -xwould make the step fail early (see inline). - JIRA vs
chore:. I don't think this is a chore. It changes the shippedcamel/camel.batscripts and the content of the distribution archives, it fixes a user-visible failure (no main manifest attribute), and it is backported to 4.22.x (#27827). A small CAMEL Bug with fix versions for main and 4.22.2 would put it in the release notes of both lines, and the squash title would then beCAMEL-XXXXX: camel-launcher - skip sources and javadoc jars. If you prefer to keep it without a JIRA, please at least use achore:prefix in the squash title, so it fits the commit format. Not blocking for me.
CI: all checks are green on bdbcb46 (build 17 and 25, posix-validator macOS and ubuntu, windows-validator).
Checked against the project conventions in CLAUDE.md. This is a rules-and-conventions review and does not replace CodeRabbit, Sourcery or SonarCloud.
Claude Code on behalf of @davsclaus.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| run: | | ||
| CAMEL_CI_MAVEN_HOME=$(./mvnw -q -B -P apache-snapshots -ntp -N org.apache.maven.plugins:maven-help-plugin:3.5.1:evaluate \ | ||
| -Dexpression=maven.home -DforceStdout) | ||
| echo "$CAMEL_CI_MAVEN_HOME/bin" >> "$GITHUB_PATH" |
There was a problem hiding this comment.
Optional: fail the step if maven.home did not come out as a Maven install, so the packaging scripts never silently fall back to the runner's mvn:
| echo "$CAMEL_CI_MAVEN_HOME/bin" >> "$GITHUB_PATH" | |
| test -x "$CAMEL_CI_MAVEN_HOME/bin/mvn" || { echo "Unexpected maven.home: $CAMEL_CI_MAVEN_HOME" >&2; exit 1; } | |
| echo "$CAMEL_CI_MAVEN_HOME/bin" >> "$GITHUB_PATH" |
When auxiliary sources or javadoc JARs are present beside the executable launcher, wildcard selection can start one of those archives and fail with
no main manifest attribute. The reported downstream distribution contains a sources JAR inbin/.Skip both
*-sources.jarand*-javadoc.jarin the Unix and Windows launch scripts. Exclude them from the shared assembly content, so neither public nor WinGet distributions carry them. Add regression fixtures sorting before and after the executable JAR, with installation paths containing spaces, using the existing JUnit assertion style.Update package-native validation to run the Unix/Windows launcher script suites. Use the project Maven wrapper rather than the runner-provided Maven (the failing Ubuntu image documents Maven 3.10.0), and enable
apache-snapshotsduring test and version-evaluation steps to resolve build plugins consistently with the preceding build steps.Normalize the selected Windows launcher JAR with
%%~fi, avoiding the doubled directory separator that caused two strict path/argument assertions to fail. The tests remain unchanged.Pin the wrapper distribution's
bindirectory on the POSIX job PATH and propagateapache-snapshotsthroughMAVEN_ARGS. Packaging scripts invokemvnin child processes, so changing only direct workflow invocations did not cover those calls.Validation on macOS with Java 21 and the project Maven 3.9.16 wrapper:
../../../mvnw -o -Dtest=CamelShDiscoveryTest,CamelBatDiscoveryTest,FakeJavaTest,WebsiteInstallTest,WebsiteInstallerFixtureTest package formatter:format impsort:sortpasses on both branches (Maven reports 103 tests, zero failures/errors, 36 skipped).target/before packaging; the generated ZIP and TAR archives exclude them and retain the executable launcher and Jolokia agent.sh -n, andgit diff --checkpass.CamelBatDiscoveryTestcases and all 3FakeJavaTestcases passed, including both previously failing path/argument checks. Completed Windows job.PackageNativeValidationTestsuite was stopped after its host Homebrew check stalled; it remains enabled in CI.Backport: #27827.
AI-generated by Codex on behalf of @Croway.