-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fail on legacy config in modular projects #11702
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fail on legacy config in modular projects #11702
Conversation
In modular projects, legacy directories and resources that would be silently ignored now trigger an ERROR instead of WARNING: - Explicit <sourceDirectory>/<testSourceDirectory> differing from defaults - Default src/main/java or src/test/java existing on filesystem - Explicit <resources>/<testResources> differing from Super POM defaults This prevents silent loss of user-configured sources/resources. AC8 supersedes AC7 which originally used WARNING. Fixes apache#11701 See apache#11701 (comment) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
70a7358 to
c1b431b
Compare
|
Reviews are welcome, in particular from @elharo and @desruisseaux - thanks! |
elharo
left a comment
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.
not obvious to me how claude code is being used here. Generally I would not expect simply pointing it at this issue without human editing to produce correct results.
| * | ||
| * Additionally, for modular projects, legacy directories are unconditionally | ||
| * ignored because it is not clear how to dispatch their content between | ||
| * rejected because it is not clear how to dispatch their content between |
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.
I don't think this is what we were looking for. The build fails completely, not simply a warning
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.
Proposal: replace "A warning is emitted if …" by "The build fails if …".
| * </ul> | ||
| * Legacy directories are unconditionally ignored in modular projects because it is not clear | ||
| * how to dispatch their content between different modules. | ||
| * In both cases, the legacy directory conflicts with modular sources and must not be used. |
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.
not "and must not be used" but rather the complete build fails.
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.
not "and must not be used" but rather the complete build fails.
Isn't what the next line below is saying?
| scopeId, | ||
| sourcesConfig); | ||
| String message = String.format( | ||
| "Legacy %s element must not be used because %s resources are configured via %s in <sources>.", |
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.
must not be used --> cannot be used in modular projects
| if (hasExplicitLegacyResources(resources, scopeId)) { | ||
| String message = "Legacy " + legacyElement | ||
| + " element is ignored because modular sources are configured. " | ||
| + " element must not be used because modular sources are configured. " |
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.
must not --> cannot
| scopeId, | ||
| sourcesConfig); | ||
| String message = String.format( | ||
| "Legacy %s element must not be used because %s resources are configured via %s in <sources>.", |
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.
cannot
| * Acceptance Criterion: AC2 (unified source tracking for all lang/scope combinations) | ||
| * Acceptance Criteria: | ||
| * - AC2 (unified source tracking for all lang/scope combinations) | ||
| * - AC8 (legacy directories error - supersedes AC7 which originally used WARNING) |
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.
not sure how the AC are being used, but shouldn't AC7 be changed rather than superseded?
| * In 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 (differ from Super POM defaults). | ||
| * In modular projects, legacy directories must not occur because it is not clear |
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.
In modular projects, legacy directories must not occur --> Legacy directories cannot be used in modular projects
- Use "cannot" instead of "must not" in error messages - Update Javadoc: "The build fails" instead of "A warning is emitted"
| /* | ||
| * `sourceDirectory`, `testSourceDirectory` and `scriptSourceDirectory` | ||
| * are ignored if the POM file contains at least one enabled <source> element | ||
| * are not used if the POM file contains at least one enabled <source> element |
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.
again here don't we need the build to fail in this case?
Summary
Affected configurations in modular projects:
<sourceDirectory>/<testSourceDirectory>differing from defaultssrc/main/javaorsrc/test/javaexisting on filesystem<resources>/<testResources>differing from Super POM defaultsTest plan
ProjectBuilderTest#testModularSourcesWithExplicitResourcesIssuesErrorpassesProjectBuilderTest#testMixedSourcesModularMainClassicTestpassesFixes #11701
See #11701 (comment)
🤖 Generated with Claude Code