Skip resource filtering when no resources are configured - #520
efegokdemir wants to merge 4 commits into
Conversation
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Thanks for the contribution — the idea is right (skip unnecessary work when no resources are configured). Two issues need to be addressed before this can be merged.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
|
||
| content = new File(basedir, 'build.log').text | ||
|
|
||
| assert !content.contains('Copying 0 resource') |
There was a problem hiding this comment.
The assertion is a no-op — it passes even without the fix.
The message "Copying X resource" is logged inside DefaultMavenResourcesFiltering.filterResources() once per resource directory, inside the for (Resource resource : ...) loop. When the resources list is empty, the loop body never executes, so "Copying 0 resource" is never logged — with or without the early-return guard.
You need an assertion that actually distinguishes the fixed behaviour from the unfixed behaviour. For example, you could assert that the plugin was silently skipped (e.g. verify the output directory was not created, or add a log message like "No resources configured, skipping." in the guard and assert its presence).
There was a problem hiding this comment.
Updated in 5c63772. The previous assertion was ineffective because the logging loop never runs for an empty resource list even without the guard. The IT now asserts the new explicit skip log, which is absent without this behavior.
| # under the License. | ||
|
|
||
| invoker.goals = resources:resources | ||
| invoker.maven.version = 3.0+ |
There was a problem hiding this comment.
invoker.maven.version = 3.0+ is wrong here. This plugin version (4.0.0-beta-2-SNAPSHOT) requires Maven 4 (mavenVersion property is 4.0.0-rc-7 in the POM). With 3.0+ the IT would be selected to run against a Maven 3 installation where the plugin cannot work.
Check what the other ITs in this module use — you likely don't need this line at all (the invoker plugin inherits a default from the parent), or it should specify the Maven 4 requirement.
There was a problem hiding this comment.
Updated in 5c63772. Removed the incorrect invoker.maven.version = 3.0+; the IT now inherits the plugin project Maven prerequisite, which is Maven 4 (mavenVersion is 4.0.0-rc-6 on this branch).
|
|
||
| protected void doExecute() throws MojoException { | ||
| if (getResources().isEmpty()) { | ||
| return; |
There was a problem hiding this comment.
Nit: consider adding a debug or info log so the early exit is observable:
| return; | |
| if (getResources().isEmpty()) { | |
| getLog().info("No resources configured, skipping."); | |
| return; | |
| } |
This is consistent with DefaultMavenResourcesFiltering.filterResources() which logs "No resources configured skip copying/filtering" for the null case.
There was a problem hiding this comment.
Added the concise No resources configured, skipping. info message in the early-return branch, matching the existing observable skip behavior in the filtering implementation.
Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
All three findings from the previous review are addressed:
- verify.groovy — assertion now checks for the new
"No resources configured, skipping."log message, which is only emitted by the guard. Effective and distinguishes fixed from unfixed behavior. - invoker.properties — removed the incorrect
invoker.maven.version = 3.0+. The IT now inherits the project default, consistent with the other ITs in this repo. - ResourcesMojo.java — info log added in the early-return branch, matching the suggestion.
The guard correctly applies to all three mojo subclasses (ResourcesMojo, TestResourcesMojo, CopyResourcesMojo) via inheritance.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
The empty-resources IT was not actually testing the 'no resources configured' code path because Maven's model always provides a default src/main/resources source root. By explicitly declaring <resources/> in the POM, the project model has zero resource roots, which causes the mojo to take the early-exit 'No resources configured, skipping.' path as intended.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
New commit 2edcac1 fixes the IT to actually exercise the early-exit guard: without <resources /> in the POM, Maven's Super POM provides a default src/main/resources root, so getResources().isEmpty() was never true and the skip path was untested. With the explicit empty <resources />, the IT now correctly triggers the guard and asserts the skip message. All three findings from the initial review remain addressed.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
The <build><resources/> element in the POM does not clear the plugin's resources parameter in Maven 4 — getEnabledSourceRoots() still returns the default src/main/resources. Move the empty <resources/> declaration into the plugin <configuration> to directly set the plugin parameter to an empty list, which triggers the skip path in doExecute().
3 participants
Summary
Avoid invoking the resource filtering component when the project has no resources configured. This prevents the misleading
Copying 0 resourcelog entry described in #311 while preserving normal filtering for configured resources.An integration fixture covers the empty-resource configuration and asserts that the zero-resource message is absent.
Fixes #311
Testing
mvn -Dspotless.skip=true -DskipTests=false test— passed (24 tests)git diff --check— passedmvn -Prun-its -Dinvoker.test=empty-resources -Dspotless.skip=true -DskipTests=false verify— blocked locally because Maven 3.9.16 does not satisfy this branch’s Maven 4.0.0-rc-6 prerequisiteSpotless was skipped because the repository formatter is not available for the local toolchain.