Skip to content

Conversation

@ascheman
Copy link
Contributor

@ascheman ascheman commented Feb 6, 2026

Summary

  • Legacy directories and resources that conflict with modular sources now trigger an ERROR instead of WARNING
  • Prevents silent loss of user-configured sources/resources
  • AC8 supersedes AC7 which originally used WARNING

Affected configurations in modular projects:

  • 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

Test plan

  • ProjectBuilderTest#testModularSourcesWithExplicitResourcesIssuesError passes
  • ProjectBuilderTest#testMixedSourcesModularMainClassicTest passes

Fixes #11701
See #11701 (comment)

🤖 Generated with Claude Code

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>
@ascheman ascheman force-pushed the bugfix/11701-fail-on-modular-sources-with-legacy-dirs branch from 70a7358 to c1b431b Compare February 6, 2026 10:05
@ascheman ascheman marked this pull request as ready for review February 6, 2026 13:27
@ascheman
Copy link
Contributor Author

ascheman commented Feb 6, 2026

Reviews are welcome, in particular from @elharo and @desruisseaux - thanks!

Copy link
Contributor

@elharo elharo left a 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
Copy link
Contributor

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

Copy link
Contributor

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.
Copy link
Contributor

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.

Copy link
Contributor

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>.",
Copy link
Contributor

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. "
Copy link
Contributor

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>.",
Copy link
Contributor

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)
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that it is a change that was forgot in the documentation. The actual code raises an error as I read it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this more closely, I think we've uncovered an inconsistency:

Current behavior:

  • Resources (<resources>/<testResources>): ERROR for both modular and classic projects when <sources> conflicts with legacy config (handled in SourceHandlingContext)
  • Source directories (<sourceDirectory>/<testSourceDirectory>): ERROR only for modular projects, silently skipped for classic projects

AC8 specifically scopes the error behavior to modular projects:

"Legacy directories trigger ERROR in modular projects"

This leaves classic projects with <sources> in an inconsistent state - their legacy resources cause an error, but legacy source directories are silently ignored.

Proposal: If @desruisseaux and @elharo agree, we could add:

AC9: For classic (non-modular) projects, legacy <sourceDirectory> and <testSourceDirectory> also trigger an ERROR when <sources> elements are configured for the same scope/language. This ensures consistency with resource handling and prevents silent configuration loss.

Should I create a follow-up issue for this, or would you prefer to extend the current PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting that. I suggest to extend the current pull request if this is fine for you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fully up to speed on all of this. I try to avoid modules. However, the general principle I would follow is that if the client project contains code or resources that the developer likely intended to be included and for some reason we are not including it, then Maven should hard fail the build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you agree, we could even simplify AC8 (and won't need a new AC9) as the existence of <sources> would then mean that any existence of legacy configurations should fail the build (independent of the existence of modules):

Revised AC8 Definition

AC8 (Legacy directory validation for projects using <sources>):

For any project using <sources>, the build fails with an ERROR if:

  1. An explicit legacy directory configuration (<sourceDirectory> or <testSourceDirectory>) exists, OR

  2. A <source> element specifies a custom <directory> AND the default legacy directory (e.g., src/main/java) exists physically on the filesystem

Key Point

The presence of <sources> is the trigger, not whether the project is modular or non-modular.

When a project uses <sources>, it opts into the Maven 4.1.0 source model entirely. Legacy directory configurations become invalid regardless of modularity.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems very good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems very good to me.

OK, I will implement it this way then.

Copy link
Contributor Author

@ascheman ascheman Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yet another use case: Assume we have the following structure:

<build>
    <sources>
      <source>
        <scope>main</scope>
        <lang>resources</lang>
        <directory>src/main/custom-resources</directory>
      </source>
      <!-- No <source lang="java"> - expects implicit src/main/java? -->
    </sources>
  </build>

I would think in a legacy directory structure but 4.1.0-model configuration (but no modules), the legacy directories should still be (implicitly) used, unless explicitly overridden. What do you think, @desruisseaux ?

Said that: the classic values were defined as default by the super POM. Do we expect similar defaults by the super POM for <sources> as well (currently not configured)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when the <module> element is not used I think that we can continue to use the legacy directories.

Regarding the default values specified in the super POM, I suggest to not do any change for now (we may revisit in some future version). The default values of <source> elements are implemented in Java code for now, and they are designed in such a way that, when no <module> element is used, they match the default values of the super POM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Legacy resources in modular build should fail

3 participants