Unify source tracking with SourceHandlingContext (#11612) #11632
Conversation
Replace boolean flags (hasMain, hasTest, etc.) with flexible set-based tracking for all language/scope combinations. - Rename ResourceHandlingContext to SourceHandlingContext - Add duplicate detection with WARNING for enabled sources - Add hasSources() method for checking language/scope combinations - Rename 'src' variable to 'sourceRoot' for clarity 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use Java 17+ Stream.toList() instead of Collectors.toList(). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Normalize path separators to forward slashes before comparing directory paths in tests. On Windows, Path.toString() returns backslashes, causing contains() checks with forward slashes to fail. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| private final Set<String> modules; | ||
| private final boolean modularProject; | ||
| private final ModelBuilderResult result; | ||
| private final Set<SourceKey> declaredSources = new HashSet<>(); |
There was a problem hiding this comment.
It is a very minor details, but moving this initialization inside the constructor would allow to specify an initial capacity as new HashSet<>(4 * modules.size()) (assuming that each module is likely to have a main, a test, main resources and test resources).
| boolean shouldAddSource(SourceRoot sourceRoot) { | ||
| if (!sourceRoot.enabled()) { | ||
| // Disabled sources are always added - they're filtered out by getEnabledSourceRoots() | ||
| LOGGER.debug( |
There was a problem hiding this comment.
Should this one be at the trace level?
| } | ||
|
|
||
| SourceKey key = new SourceKey( | ||
| sourceRoot.language(), sourceRoot.scope(), sourceRoot.module().orElse(null), sourceRoot.directory()); |
There was a problem hiding this comment.
We may consider adding .toRealPath() after directory() for making the test more exhaustive.
impl/maven-core/src/main/java/org/apache/maven/project/SourceHandlingContext.java
Show resolved
Hide resolved
impl/maven-core/src/main/java/org/apache/maven/project/SourceHandlingContext.java
Show resolved
Hide resolved
| <lang>java</lang> | ||
| <module>org.foo.moduleB</module> | ||
| </source> | ||
| <!-- No test sources defined - testSourceDirectory should be used --> |
There was a problem hiding this comment.
This is ambiguous: how could a single testSourceDirectory be used for two modules? I suggest to allow <sourceDirectory> and <testSourceDirectory> only for non-modular projects, and emit a warning or error for modular projects.
| /** | ||
| * Tests mixed source configuration where: | ||
| * - Modular sources are defined for main Java (should override sourceDirectory) | ||
| * - Classic testSourceDirectory is used (should be preserved since no modular test sources) |
There was a problem hiding this comment.
See comment in mixed-sources: this is ambiguous if we have more than one module. I suggest to modify the test for verifying that <testSourceDirectory> has been ignored and a warning emitted.
Instead (maybe in a phase 3), for each module declared in a <source> with main scope but no test scope, generate automatically the <source> with test scope in a way similar to what is currently done for resources.
| <lang>java</lang> | ||
| <module>org.foo.moduleA</module> | ||
| </source> | ||
| <!-- Non-modular source (no module attribute) --> |
There was a problem hiding this comment.
Should not be allowed. Either a project is fully modular or fully classic, but not a mix of both. The compiler plugin will not be able to handle that.
| } | ||
|
|
||
| /** | ||
| * Tests mixed modular/non-modular sources within the same {@code <sources>} element. |
There was a problem hiding this comment.
See comment in sources-mixed-modules: a project can be fully modular or fully classic, but not a mix of both. An error should be emitted. In this case, I think that it should be an error rather than a warning, because the compiler plugin will not know what to do with such mixed project.
2 participants
Summary
hasMain,hasTest,hasMainResources, etc.) with flexible set-based tracking usingSourceHandlingContextResourceHandlingContexttoSourceHandlingContextto reflect unified handling of all source typeshasSources(Language, ProjectScope)method for checking source existenceCollectors.toList()with.toList()(Java 17+ idiom)Closes #11612 (Phase 2: build-sources-validation)
Test plan
ProjectBuilderTestpasses (19 tests)testModularSourcesInjectResourceRoots- verifies module-aware resource injectiontestModularSourcesWithExplicitResourcesIssuesWarning- verifies legacy resource warningstestMixedSourcesModularMainClassicTest- mixed modular/classic configurationtestSourcesMixedModulesWithinSources- mixed modules within<sources>testMultipleDirectoriesSameModule- multiple directories per moduletestDuplicateEnabledSources- duplicate detection with WARNING🤖 Generated with Claude Code