Add module-aware resource handling for modular sources #11700
+1,109
−33
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ | |
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.util.AbstractMap; | ||
| import java.util.ArrayList; | ||
|
|
@@ -63,7 +64,6 @@ | |
| import org.apache.maven.api.model.Plugin; | ||
| import org.apache.maven.api.model.Profile; | ||
| import org.apache.maven.api.model.ReportPlugin; | ||
| import org.apache.maven.api.services.ArtifactResolver; | ||
| import org.apache.maven.api.services.ArtifactResolverException; | ||
| import org.apache.maven.api.services.ArtifactResolverRequest; | ||
|
|
@@ -520,7 +520,7 @@ List<ProjectBuildingResult> doBuild(List<File> pomFiles, boolean recursive) { | |
| return pomFiles.stream() | ||
| .map(pomFile -> build(pomFile, recursive)) | ||
| .flatMap(List::stream) | ||
| .toList(); | ||
| } finally { | ||
| Thread.currentThread().setContextClassLoader(oldContextClassLoader); | ||
| } | ||
|
|
@@ -566,7 +566,7 @@ private List<ProjectBuildingResult> build(File pomFile, boolean recursive) { | |
| project.setCollectedProjects(results(r) | ||
| .filter(cr -> cr != r && cr.getEffectiveModel() != null) | ||
| .map(cr -> projectIndex.get(cr.getEffectiveModel().getId())) | ||
| .toList()); | ||
|
Contributor
There was a problem hiding this comment. ditto; good but not needed in this PR |
||
|
|
||
| DependencyResolutionResult resolutionResult = null; | ||
| if (request.isResolveDependencies()) { | ||
|
|
@@ -660,46 +660,75 @@ private void initProject(MavenProject project, ModelBuilderResult result) { | |
| return build.getDirectory(); | ||
| } | ||
| }; | ||
| // Extract modules from sources to detect modular projects | ||
| Set<String> modules = extractModules(sources); | ||
| boolean isModularProject = !modules.isEmpty(); | ||
|
|
||
| logger.trace( | ||
| "Module detection for project {}: found {} module(s) {} - modular project: {}.", | ||
| project.getId(), | ||
| modules.size(), | ||
| modules, | ||
| isModularProject); | ||
|
|
||
| // Create source handling context for unified tracking of all lang/scope combinations | ||
| SourceHandlingContext sourceContext = | ||
| new SourceHandlingContext(project, baseDir, modules, isModularProject, result); | ||
|
|
||
| // Process all sources, tracking enabled ones and detecting duplicates | ||
| for (var source : sources) { | ||
| var sourceRoot = DefaultSourceRoot.fromModel(session, baseDir, outputDirectory, source); | ||
|
Contributor
There was a problem hiding this comment. Personally I dislike var. It prioritizes trivial typing speed over code readability
Contributor
There was a problem hiding this comment. Personally I agree with you, but perhaps its more a question of programming style. for (var problem : e.getResult().getProblemCollector()...
for (var transformer : transformers) {
Contributor
Author
There was a problem hiding this comment. I don't think that there is a consensus. I tend to use MyFooWithLongName foo = new MyFooWithLongName();However, when we don't see the type on the same line (as in |
||
| // Track enabled sources for duplicate detection and hasSources() queries | ||
| // Only add source if it's not a duplicate enabled source (first enabled wins) | ||
| if (sourceContext.shouldAddSource(sourceRoot)) { | ||
| project.addSourceRoot(sourceRoot); | ||
| } | ||
| } | ||
|
|
||
| /* | ||
| * `sourceDirectory`, `testSourceDirectory` and `scriptSourceDirectory` | ||
| * are ignored if the POM file contains at least one enabled <source> element | ||
| * for the corresponding scope and language. This rule exists because | ||
| * Maven provides default values for those elements which may conflict | ||
| * with user's configuration. | ||
| * | ||
| * Additionally, for modular projects, legacy directories are unconditionally | ||
| * ignored because it is not clear how to dispatch their content between | ||
| * different modules. A warning is emitted if these properties are explicitly set. | ||
| */ | ||
| if (!sourceContext.hasSources(Language.SCRIPT, ProjectScope.MAIN)) { | ||
| project.addScriptSourceRoot(build.getScriptSourceDirectory()); | ||
| } | ||
| if (isModularProject) { | ||
| // Modular projects: unconditionally ignore legacy directories, warn if explicitly set | ||
| warnIfExplicitLegacyDirectory( | ||
| build.getSourceDirectory(), | ||
| baseDir.resolve("src/main/java"), | ||
| "<sourceDirectory>", | ||
| project.getId(), | ||
| result); | ||
| warnIfExplicitLegacyDirectory( | ||
| build.getTestSourceDirectory(), | ||
| baseDir.resolve("src/test/java"), | ||
| "<testSourceDirectory>", | ||
| project.getId(), | ||
| result); | ||
| } else { | ||
| // Classic projects: use legacy directories if no sources defined in <sources> | ||
| if (!sourceContext.hasSources(Language.JAVA_FAMILY, ProjectScope.MAIN)) { | ||
| project.addCompileSourceRoot(build.getSourceDirectory()); | ||
| } | ||
| if (!sourceContext.hasSources(Language.JAVA_FAMILY, ProjectScope.TEST)) { | ||
| project.addTestCompileSourceRoot(build.getTestSourceDirectory()); | ||
| } | ||
| } | ||
|
|
||
| // Validate that modular and classic sources are not mixed within <sources> | ||
| sourceContext.validateNoMixedModularAndClassicSources(); | ||
|
|
||
| // Handle main and test resources using unified source handling | ||
| sourceContext.handleResourceConfiguration(ProjectScope.MAIN); | ||
| sourceContext.handleResourceConfiguration(ProjectScope.TEST); | ||
| } | ||
|
|
||
| project.setActiveProfiles( | ||
|
|
@@ -870,6 +899,49 @@ private void initProject(MavenProject project, ModelBuilderResult result) { | |
| project.setRemoteArtifactRepositories(remoteRepositories); | ||
| } | ||
|
|
||
| /** | ||
| * Warns about legacy directory usage in a modular project. Two cases are handled: | ||
| * <ul> | ||
| * <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 | ||
|
Contributor
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.
Contributor
Author
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.
Contributor
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.
Contributor
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 |
||
| * how to dispatch their content between different modules. | ||
| */ | ||
| private void warnIfExplicitLegacyDirectory( | ||
| String configuredDir, | ||
| Path defaultDir, | ||
| String elementName, | ||
| String projectId, | ||
| ModelBuilderResult result) { | ||
| if (configuredDir != null) { | ||
| Path configuredPath = Path.of(configuredDir).toAbsolutePath().normalize(); | ||
| Path defaultPath = defaultDir.toAbsolutePath().normalize(); | ||
| if (!configuredPath.equals(defaultPath)) { | ||
| // Case 2: Explicit configuration differs from default - always warn | ||
| String message = String.format( | ||
| "Legacy %s is ignored in modular project %s. " | ||
| + "In modular projects, source directories must be defined via <sources> " | ||
| + "with a module element for each module.", | ||
| elementName, projectId); | ||
| logger.warn(message); | ||
| result.getProblemCollector() | ||
| .reportProblem(new org.apache.maven.impl.model.DefaultModelProblem( | ||
| message, Severity.WARNING, Version.V41, null, -1, -1, null)); | ||
| } else if (Files.isDirectory(defaultPath)) { | ||
| // Case 1: Default configuration, but the default directory exists on filesystem | ||
| String message = String.format( | ||
| "Legacy %s '%s' exists but is ignored in modular project %s. " | ||
| + "In modular projects, source directories must be defined via <sources>.", | ||
| elementName, defaultPath, projectId); | ||
| logger.warn(message); | ||
| result.getProblemCollector() | ||
| .reportProblem(new org.apache.maven.impl.model.DefaultModelProblem( | ||
| message, Severity.WARNING, Version.V41, null, -1, -1, null)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void initParent(MavenProject project, ModelBuilderResult result) { | ||
| Model parentModel = result.getParentModel(); | ||
|
|
||
|
|
@@ -1011,8 +1083,8 @@ private DependencyResolutionResult resolveDependencies(MavenProject project) { | |
| } | ||
| } | ||
|
|
||
| private static List<String> getProfileIds(List<Profile> profiles) { | ||
| return profiles.stream().map(Profile::getId).toList(); | ||
| } | ||
|
|
||
| private static ModelSource createStubModelSource(Artifact artifact) { | ||
|
|
@@ -1093,6 +1165,22 @@ public Set<Entry<K, V>> entrySet() { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Extracts unique module names from the given list of source elements. | ||
| * A project is considered modular if it has at least one module name. | ||
| * | ||
| * @param sources list of source elements from the build | ||
| * @return set of non-blank module names | ||
| */ | ||
| private static Set<String> extractModules(List<org.apache.maven.api.model.Source> sources) { | ||
| return sources.stream() | ||
| .map(org.apache.maven.api.model.Source::getModule) | ||
| .filter(Objects::nonNull) | ||
| .map(String::trim) | ||
| .filter(s -> !s.isBlank()) | ||
| .collect(Collectors.toSet()); | ||
| } | ||
|
|
||
| private Model injectLifecycleBindings( | ||
| Model model, | ||
| ModelBuilderRequest request, | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a separate small change