Skip to content

camel-launcher: skip sources and javadoc jars - #27826

Open
Croway wants to merge 4 commits into
apache:mainfrom
Croway:fix/launcher-skip-sources-jar
Open

Croway wants to merge 4 commits into
apache:mainfrom
Croway:fix/launcher-skip-sources-jar

Conversation

@Croway

@Croway Croway commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

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 in bin/.

Skip both *-sources.jar and *-javadoc.jar in 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-snapshots during 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 bin directory on the POSIX job PATH and propagate apache-snapshots through MAVEN_ARGS. Packaging scripts invoke mvn in 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:sort passes on both branches (Maven reports 103 tests, zero failures/errors, 36 skipped).
  • Sources and javadoc JAR fixtures were placed in target/ before packaging; the generated ZIP and TAR archives exclude them and retain the executable launcher and Jolokia agent.
  • Workflow YAML, embedded Bash syntax, sh -n, and git diff --check pass.
  • Windows CI passed on the latest commit: all 24 CamelBatDiscoveryTest cases and all 3 FakeJavaTest cases passed, including both previously failing path/argument checks. Completed Windows job.
  • A broader local run of the unchanged PackageNativeValidationTest suite was stopped after its host Homebrew check stalled; it remains enabled in CI.

Backport: #27827.

AI-generated by Codex on behalf of @Croway.

@github-actions

Copy link
Copy Markdown
Contributor

🌟 Thank you for your contribution to the Apache Camel project! 🌟
🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

  • First-time contributors require MANUAL approval for the GitHub Actions to run
  • You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot although they are normally detected and executed by CI.
  • You can label PRs using skip-tests and test-dependents to fine-tune the checks executed by this PR.
  • Build and test logs are available in the summary page. Only Apache Camel committers have access to the summary.

⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

@github-actions github-actions Bot added the dsl label Oct 14, 2026
@gnodet

gnodet commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

🔄 Backport Bot

Port PRs:

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 davsclaus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Croway. The fix itself is small and correct. What I checked:

  • camel.sh: the glob sorts camel-launcher-X-sources.jar before camel-launcher-X.jar (- sorts before .), and the loop takes the first match. That is the bug, and the case skip fixes it. The new test sets LC_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 matches camel.sh. The zzz-sources fixture 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):

  1. Where does the sources jar come from? The published archives don't have one. camel-launcher-4.22.1-bin.tar.gz on Maven Central and the latest 4.23.0-SNAPSHOT bin archive only contain camel-launcher-<version>.jar and the Jolokia agent in bin/. The assembly in launcher-content.xml copies ${project.build.directory}/*.jar (only original-*.jar is excluded). So I guess a local build with a profile that attaches sources (source-jar or deploy) puts the sources jar in target/ before the assembly runs. If so, also excluding *-sources.jar and *-javadoc.jar in that fileSet would fix the root cause: the archive would not carry those jars at all. The script change is still a good guard.
  2. -javadoc.jar: Maven Central has camel-launcher-<version>-javadoc.jar too, and it sorts before the main jar in the same way. Consider skipping it in both scripts as well (see inline).
  3. The Windows change is not run by CI. CamelBatDiscoveryTest is @EnabledOnOs(WINDOWS). The windows-validator job in package-native-validation.yml only runs WebsiteInstallTest, 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 run CamelBatDiscoveryTest on Windows? Adding it to the -Dtest= list of windows-validator would cover this script from now on.
  4. 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.
  5. 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.

Comment thread dsl/camel-jbang/camel-launcher/src/main/resources/bin/camel.sh Outdated
Comment thread dsl/camel-jbang/camel-launcher/src/main/resources/bin/camel.bat Outdated
@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

🧪 CI tested the following changed modules:

  • dsl/camel-jbang/camel-launcher

🔬 Scalpel shadow comparison — Scalpel: 1 of 703 tested, 0 compile-only — current: 1 all tested

Maveniverse 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)
  • camel-launcher-container ← downstream of org.apache.camel:camel-launcher

ℹ️ Shadow mode — Scalpel observes but does not affect test execution. Learn more

💡 Manual integration tests recommended:

You modified dsl/camel-jbang/camel-launcher. The related integration tests in dsl/camel-jbang/camel-jbang-it are excluded from CI. Consider running them manually:

mvn verify -f dsl/camel-jbang/camel-jbang-it -Djbang-it-test
All tested modules (2 modules, 44.3s total)

Total reactor time: 44.3s

Module Duration Status
Camel :: Launcher 43.4s SUCCESS
Camel :: Launcher :: Container 0.9s SUCCESS

Top 20 slowest modules:

  • Camel :: Launcher (43.4s)
  • Camel :: Launcher :: Container (0.9s)

⚙️ View full build and test results

@Croway Croway changed the title camel-launcher: skip sources jars in launch scripts camel-launcher: skip sources and javadoc jars Oct 14, 2026
@Croway

Croway commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

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 bin/. I agree that this does not establish that the Maven Central binary archives contain one; the script guard protects installations where auxiliary artifacts are present, and the assembly exclusions prevent packaging them. The 4.22.x scripts and assembly have the same behavior, so the companion backport remains useful. Keeping this bounded change without a new JIRA; the linked PRs track both targets.

windows-validator now runs CamelBatDiscoveryTest and FakeJavaTest in addition to installer tests. Actual Windows execution is pending the fresh CI run. Both branches pass local launcher/installer tests, packaging, and formatting; ZIP/TAR packaging was checked with auxiliary JAR fixtures present in target/.

The CI changes use the pinned Maven wrapper and enable apache-snapshots in the test steps. The 4.22.x port also includes the existing CAMEL-24485 Windows timeout/process-handling fix.

AI-generated by Codex on behalf of @Croway.

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on my earlier review — the two new commits address davsclaus's feedback completely.

What changed:

  • *-javadoc.jar is now also excluded in both camel.sh (case guard extended) and camel.bat (second /c: argument to findstr), and from the assembly (launcher-content.xml).
  • CamelBatDiscoveryTest and FakeJavaTest are now included in the windows-validator CI step, closing the coverage gap davsclaus flagged.
  • The REAL_VERSION step 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.

@Croway

Croway commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

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 %%~fi (the normalized full path); the strict test assertions remain unchanged.

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 gnodet-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Croway

Croway commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

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 davsclaus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

    1. Root cause in the assembly: fixed. launcher-content.xml now excludes *-sources.jar and *-javadoc.jar, so the archives cannot carry them.
    1. -javadoc.jar: fixed in camel.sh, camel.bat and the assembly, with fixtures that sort before and after the main jar.
    1. Windows script not run by CI: fixed. windows-validator now runs CamelBatDiscoveryTest and FakeJavaTest, and the latest run passed all 24 + 3 tests there. The POSIX job runs CamelShDiscoveryTest and FakeJavaTest too.
    1. Assertion style: fixed, the new tests use the JUnit assertions of the surrounding classes.
    1. No JIRA: still open, see below.

What changed since my approval:

  • camel.bat: set "LAUNCHER_JAR=%%~fi" instead of %%i. BASEDIR comes from %~dp0 and ends with \, so "%BASEDIR%\camel-launcher-*.jar" gave a path with a doubled separator. %%~fi gives 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 Maven bin on PATH and adds -P apache-snapshots to MAVEN_ARGS. This is needed because camel-package.sh and camel-validate.sh call plain mvn. The ubuntu log shows MAVEN_ARGS: -ntp -P apache-snapshots in the later steps, and both POSIX validators passed.

New findings:

  1. (non-blocking) The pin step does not check what help:evaluate printed. If that output ever has an extra line, a wrong directory goes on PATH without an error, and the scripts fall back to the runner's mvn again. A test -x would make the step fail early (see inline).
  2. JIRA vs chore:. I don't think this is a chore. It changes the shipped camel / camel.bat scripts 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 be CAMEL-XXXXX: camel-launcher - skip sources and javadoc jars. If you prefer to keep it without a JIRA, please at least use a chore: 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"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working dsl

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants