Add module-aware resource handling for modular sources #11700
Conversation
This is a combination of pull requests apache#11505 and apache#11632 on master branch. Summary: Maven 4.x introduces a unified <sources> element that supports modular project layouts (src/<module>/<scope>/<lang>). However, resource handling did not follow the modular layout - resources were always loaded from the legacy <resources> element which defaults to src/main/resources. This change implements automatic module-aware resource injection. - For modular projects without resource configuration in <sources>, automatically inject resource roots following the modular layout: src/<module>/main/resources and src/<module>/test/resources - Resources configured via <sources> take priority over legacy <resources> - Issue warnings (as ModelProblem) when explicit legacy resources are ignored
elharo
left a comment
elharo
left a comment
There was a problem hiding this comment.
I'm a little uncomfortable that large fixes like this are getting submitted without corresponding issues.
| .map(pomFile -> build(pomFile, recursive)) | ||
| .flatMap(List::stream) | ||
| .collect(Collectors.toList()); | ||
| .toList(); |
There was a problem hiding this comment.
This could be a separate small change
| .filter(cr -> cr != r && cr.getEffectiveModel() != null) | ||
| .map(cr -> projectIndex.get(cr.getEffectiveModel().getId())) | ||
| .collect(Collectors.toList())); | ||
| .toList()); |
There was a problem hiding this comment.
ditto; good but not needed in this PR
| } | ||
| } else { | ||
| hasScript |= Language.SCRIPT.equals(language); | ||
| var sourceRoot = DefaultSourceRoot.fromModel(session, baseDir, outputDirectory, source); |
There was a problem hiding this comment.
Personally I dislike var. It prioritizes trivial typing speed over code readability
There was a problem hiding this comment.
Personally I agree with you, but perhaps its more a question of programming style. var is already used in other places of this codebase, e.g., DefaultModelBuilder.java:
for (var problem : e.getResult().getProblemCollector()...
for (var transformer : transformers) {There was a problem hiding this comment.
I don't think that there is a consensus. I tend to use var when it avoids a redundancy on the same line, especially when the redundancy is verbose. For example:
MyFooWithLongName foo = new MyFooWithLongName();However, when we don't see the type on the same line (as in for (var transformer : transformers)), I agree that it is good to keep the type. I usually try to stick to the rule "use var only if we can see the type on the same line", but I may sometime use var by mistake.
| * <li>Case 1: The default legacy directory exists on the filesystem (e.g., src/main/java exists)</li> | ||
| * <li>Case 2: An explicit legacy directory is configured that differs from the default</li> | ||
| * </ul> | ||
| * Legacy directories are unconditionally ignored in modular projects because it is not clear |
There was a problem hiding this comment.
I'm not comfortable with this. If we can't dispatch, I think we need to hard fail the build rather than simply ignoring the content. Let the user figure out how to rearrange the project to make the build pass. A warning is not enough here.
There was a problem hiding this comment.
I agree. If @ascheman also agrees, I guess that we can do this change. But since this pull request is a backport, I would suggest to do the change on master first, then backport here.
There was a problem hiding this comment.
Bleah. 3.x maintenance and 4.x development is hard enough without trying to develop two 4.x versions simultaneously. In any case, sure, let's do the change on master and then backport here, at least until we come to our senses and finish one 4.x version before starting the next.
There was a problem hiding this comment.
Sounds feasible to me. I would think this should be another acceptance criterion (AC8) in the overall concept to document that additional behaviour? It would be consistent with AC6 (where we also fail for mixed modular/classic <sources>).
|
This pull request is a squashed backport of two pull requests from the mater branch, one merged in December 30th and the second one today. The issue describing the second commit on the master branch is #11612, but a more complete description is on the wiki at the link below: https://cwiki.apache.org/confluence/display/MAVEN/Build+Sources+Validation+-+Discussion+Notes |
Useful, but the wiki is not an issue tracker. We seem to be getting in the habit of submitting major PRs without issues. That's an unexpected and IMHO negative side effect of moving to GitHub issues from JIRA. |
As this is a backport (mostly) of #11612: Would it be OK for you if this were explicitly mentioned? |
Only if we don't close the bug until the backports are complete. And even then I'm not sure our release tools are set up to handle single issues that touch multiple versions. |
3 participants
This is a combination of pull requests #11505 and #11632 on master branch.
Summary: Maven 4.x introduces a unified
<sources>element that supports modular project layouts (src/<module>/<scope>/<lang>). However, resource handling did not follow the modular layout - resources were always loaded from the legacy<resources>element which defaults tosrc/main/resources. This change implements automatic module-aware resource injection.<sources>, automatically inject resource roots following the modular layout:src/<module>/main/resourcesandsrc/<module>/test/resources.<sources>take priority over legacy<resources>.ModelProblem) when explicit legacy resources are ignored.