Repository navigation
Conversation
AggregatorSourceJarMojo.doExecute() packages sources only for pom packaging and otherwise returns without a word, so a project that reaches the goal with some other packaging gets no source archive and no indication of why. Warn instead, naming the project and its actual packaging, in the style of the classifier warning packageSources already emits.
| if (Type.POM.equals(type)) { | ||
| packageSources(reactorProjects); | ||
| } else { | ||
| getLog().warn("NOT aggregating sources as this goal requires a project with [" + Type.POM |
There was a problem hiding this comment.
I'm unsure about this. Maybe debug level? I don't know.
|
Debug would leave it effectively silent, which is the thing #306 is about — nobody runs with Warn felt right because it only fires when the goal was asked to do something it cannot do, so it isn't noise on a normal build. Your issue also suggested "a warning or info message", which is where I took it from. That said, info is a reasonable middle if warn feels too loud — it still shows on a default run. Say the word and I'll switch it. |
|
@elharo just following up, did you have a preference between warn and info for this, or is warn fine as is? Happy to switch it either way, just want to close this out. |
aggregate is often declared in a parent POM and inherited by jar
modules, where the skip is expected, so warn would flag every one of
them. info matches the plugin's other expected-skip messages ("Skipping
source per configuration", "NOT adding java-sources ...") and stays
visible at the default log level, unlike debug.
|
Good call. aggregate is often declared in a parent POM and inherited by jar modules, and a warning would fire on every one of them. I switched it to info, which is what the plugin already uses for its other expected skips like "Skipping source per configuration" and "NOT adding java-sources". It still shows at the default log level, so the skip is no longer silent, which is what #306 was about. |
elharo
left a comment
There was a problem hiding this comment.
The more I look at this the more I'm convinced this case should simply fail, i.e. throw an exception.
Unless there's some reason devs need to aggregate a non-pom project that aggregates nothing, but I can't think of one.
There is nothing to aggregate in a project without POM packaging, so stop the build with a message naming the project and its packaging instead of skipping.
|
Agreed, since the goal is an aggregator it only runs once at the top of the build, so failing there shouldn't break anyone's inherited modules. It now throws a MojoException naming the project and its packaging, and the test checks for that. I updated the title and description to match. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved issues were identified, and both packaging branches are covered by tests.
Review effort: Lite
Findings: None
What changed in this PR
Updates the aggregate source goal to fail clearly for non-POM packaging instead of silently skipping.
Changes:
- Adds descriptive validation for unsupported packaging.
- Adds tests for POM and non-POM behavior.
| File | Description |
|---|---|
src/test/java/org/apache/maven/plugins/source/AggregatorSourceJarMojoTest.java |
Tests successful aggregation and failure behavior. |
src/main/java/org/apache/maven/plugins/source/AggregatorSourceJarMojo.java |
Validates packaging before aggregation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@elharo Please assign appropriate label to PR according to the type of change. |
Fixes #306
Problem
AggregatorSourceJarMojo.doExecute()packages sources only forpompackaging:Any other packaging falls off the end of the method. No archive is produced and nothing is logged, so there is no way to tell the skip apart from a build that simply did not reach the goal.
Fix
A non-POM project has nothing to aggregate, so the goal now throws a
MojoExceptionnaming the project and its actual packaging. The goal isaggregator = true, so it runs once at the top of the build rather than on each inherited module.Testing
AggregatorSourceJarMojoTestcovers both branches, overridingpackageSourcesto record the projects it receives rather than building an archive:pompackaging still packages the reactor projects, and logs nothingmvn test: 12 tests, all passing.