Skip to content

[MNG-8713] SourceRoot.directory() default value should include the module when present #2278

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 20, 2025
Merged

[MNG-8713] SourceRoot.directory() default value should include the module when present #2278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
90933fe
The `SourceRoot.directory()` default value should include the module …
desruisseaux Apr 27, 2025
7765b65
Apply editorial changes suggested in the pull request.
desruisseaux May 15, 2025
21a1a1d
Change the default directory of multi-modules project.
desruisseaux May 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions api/maven-api-core/src/main/java/org/apache/maven/api/SourceRoot.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,26 @@ public interface SourceRoot {
* The path is relative to the <abbr>POM</abbr> file.
*
* <h4>Default implementation</h4>
* The default value is <code>src/{@linkplain #scope() scope}/{@linkplain #language() language}</code>
* as a relative path. Implementation classes may override this default with an absolute path instead.
* The default value depends on whether a {@linkplain #module() module name} is specified in this source root:
* <ul>
* <li>
* If no module name, then the default directory is
Copy link
Contributor

Choose a reason for hiding this comment

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

shifting from main/java to java/main feels like a really big change. Is there a very compelling reason to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reminder: this pull request does not change the convention of non-modular projects or projects that do not use the module source hierarchy: for them, the default stay src/main/java. Only for projects that use the module source hierarchy, we would "flip" from (nothing that exist currently in Maven) to src/java/org.my.module/main.

The reason for putting ${lang} before ${module} is because the module names tend to be language-dependent. For example, Java uses URL domain names (e.g. org.apache.maven) while Python uses shorter module names, without org or com prefix. If a Maven subproject mixes Java and Python modules, placing the modules of the two languages in the same directory is confusing.

The reason for putting ${scope} after ${module} is for grouping together the files related to each module. This is the same division as Maven subprojects (previously named "modules"): each subproject contains main, test, resource and pom.xml files. Likewise, each Java module would contain its own main, test, resource files, with module-info somewhat playing the role of pom.xml. In other words, we are still splitting the projects in modules, but shifting from a world where modules are managed by Maven to a world were modules are managed by Java.

There is precedents: ${module}/${scope} is the directory layout of NetBeans's Ant modular project, of Christian Stein's blog (except for ${lang}) and of the example given in Oracle's documentation (except for src and ${lang}).

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense to me. @elharo ?

Copy link
Contributor

@elharo elharo May 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I am not convinced. That feels very wrong to me, and I expect users will be unnecessarily confused. What we've had in the past is src/main/java/package-structure and the natural extension of that is src/main/java/module/package-structure

If other languages come into play (which I really don't think will happen or is something we need to worry about) then src/main/python/module or whatever

Changing anything to src/java/main introduces inconsistency we don't have now.

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 sure that's the right angle to look at.

If a given project has multiple modules, those modules kinda act as subprojects, so it makes sense to group their files together, rather than splitting main / tests. In that case, each test directory is testing the code of the module, so having them close to each other makes more sense 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.

Indeed, the rational is as Guillaume said. While we try to avoid disruption for users who are not interested in JPMS, those who want to embrace fully JPMS will have to change some habits anyway, some of them out of our control (e.g., the directory layout of javac output).

There is a difficulty with resources, which are considered as a language and therefore would be stored apart with a src/${lang}/${module}/${scope} layout if the users do not specify the directory themselves. An alternative staying closer to current Maven practice may be src/${module}/${scope}/${lang}.

Copy link
Contributor

Choose a reason for hiding this comment

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

This layout is closer to the usual maven layout and may be less disruptive. It just adds the module before the scope. I like it better actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The inconvenient of the alternative proposal is that if a subproject has two or more languages, the modules of different languages will be mixed in the same directory. However, it may not be important if, as suggested in other comments, we choose to not let the multi-languages issue influences the decision.

Note: this difficulty come from the fact that in the current non-modular layout, resources is implicitly understood as "resources of the Java artifact". It is not really an independent "language".

If there is no objection or alternative proposal, I will change the layout to src/${module}/${scope}/${lang} in a few days.

* <code>src/{@linkplain #scope() scope}/{@linkplain #language() language}</code>.
* </li><li>
* If a module name is present, then the default directory is
* <code>src/{@linkplain #module() module}/{@linkplain #scope() scope}/{@linkplain #language() language}</code>.
* </li>
* </ul>
*
* The default value is relative.
* Implementation may override with absolute path instead.
*/
default Path directory() {
return Path.of("src", scope().id(), language().id());
Path src = Path.of("src");
return module().map(src::resolve)
.orElse(src)
.resolve(scope().id())
.resolve(language().id());
}

/**
Expand Down
12 changes: 9 additions & 3 deletions api/maven-api-model/src/main/mdo/maven.mdo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2011,9 +2011,15 @@
usage (main code, tests, <i>etc.</i>) is specified by the {@code scope} element.

<h2>Default source directories</h2>
If no source directories are specified, the defaults are {@code src/${scope}/${lang}} where
{@code ${scope}} is the value of the {@link #scope} element (typically {@code main} or {@code test}) and
{@code ${lang}} is the value of the {@link #lang} element (typically {@code java} or {@code resources}).
If no source directories are specified, the default values depend on whether module names are specified:
<ul>
<li>{@code src/${scope}/${lang}} if no module is specified</li>
<li>{@code src/${module}/${scope}/${lang}} if a module is specified</li>
</ul>
where
{@code ${scope}} is the value of the {@link #scope} element (typically {@code main} or {@code test}),
{@code ${lang}} is the value of the {@link #lang} element (typically {@code java} or {@code resources}),
and {@code ${module}} is the optional {@link #module} element.
]]>
</description>
<version>4.1.0+</version>
Expand Down
6 changes: 5 additions & 1 deletion impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSourceRoot.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,11 @@ public DefaultSourceRoot(final Session session, final Path baseDir, final Source
if (value != null) {
directory = baseDir.resolve(value);
} else {
directory = baseDir.resolve("src").resolve(scope.id()).resolve(language.id());
Path src = baseDir.resolve("src");
if (moduleName != null) {
src = src.resolve(language.id());
}
directory = src.resolve(scope.id()).resolve(language.id());
}

value = nonBlank(source.getTargetVersion());
Expand Down