gnodet commented on code in PR #13403:
URL: https://github.com/apache/maven/pull/13403#discussion_r4240848951
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Packaging.java:
##########
@@ -89,4 +89,15 @@ default Language language() {
*/
@Nonnull
Map<String, PluginContainer> plugins();
+
+ /**
+ * {@return whether this packaging produces a main artifact}.
+ * Packaging types such as {@code pom} or {@code bom}, or projects with a
module source hierarchy,
+ * do not produce a main artifact.
Review Comment:
⚠️ **Design concern — project-level state leaking into a type-level API**
The Javadoc states _"projects with a module source hierarchy do not produce
a main artifact"_ but `Packaging` is annotated `@Immutable` and represents a
packaging **type** (e.g. `jar`, `bom`), not a project instance. Whether a
specific project uses a module source hierarchy is project-level, runtime,
mutable state that cannot be encoded in a type descriptor.
`producesMainArtifact()` should reflect only what the packaging **type**
implies (e.g. `pom` and `bom` never produce a main artifact, `jar` always
does). The module-source-hierarchy override belongs on `Project` — something
like `Project.producesMainArtifact()` — where project-level state is naturally
in scope. Putting it on `Packaging` forces `DefaultProject.packaging()` to
return a lying wrapper whose `producesMainArtifact()` disagrees with what
`PackagingRegistry` returns for the same type ID.
##########
impl/maven-core/src/main/java/org/apache/maven/ReactorReader.java:
##########
@@ -463,12 +464,29 @@ private Stream<Artifact> getProjectArtifacts(MavenProject
project) {
// pom artifact
Stream.of(new ProjectArtifact(project)),
// main project artifact if not a pom
- "pom".equals(project.getPackaging()) ? Stream.empty()
: Stream.of(project.getArtifact())),
+ !producesMainArtifact(project) ? Stream.empty() :
Stream.of(project.getArtifact())),
// attached artifacts
project.getAttachedArtifacts().stream());
return artifacts.map(RepositoryUtils::toArtifact);
}
+ boolean producesMainArtifact(MavenProject project) {
+ if ("pom".equals(project.getPackaging())) {
+ return false;
+ }
+ try {
+ if (session != null && session.getSession() != null) {
+ org.apache.maven.api.Packaging pkg =
session.getSession().requirePackaging(project.getPackaging());
+ if (!pkg.producesMainArtifact()) {
+ return false;
+ }
+ }
+ } catch (Exception e) {
+ // fallback
+ }
Review Comment:
🐛 **Silent `catch (Exception e)` swallows all exceptions including
programming errors**
```java
} catch (Exception e) {
// fallback
}
```
Catching the root `Exception` type silences `ClassCastException`,
`NullPointerException`, `IllegalStateException`, and any future checked
exceptions — the entire block becomes invisible when it fails. If
`requirePackaging` throws because the session is in a bad state or the
packaging type is unknown, the code silently falls through to the
`SourceQueries` check, which may return an incorrect result with no diagnostic.
At minimum, log the exception at `WARN` level so the fallback is observable:
```suggestion
} catch (Exception e) {
// fallback — log to aid debugging
LOGGER.warn("Unable to determine packaging for '{}', falling
back to source-hierarchy check: {}",
project.getPackaging(), e.getMessage());
}
```
Or narrow the catch to the specific exception type thrown by
`requirePackaging` (e.g. `NoSuchElementException` / a Maven-specific lookup
exception).
##########
impl/maven-core/src/main/java/org/apache/maven/project/SourceQueries.java:
##########
@@ -36,6 +37,28 @@
public final class SourceQueries {
private SourceQueries() {}
+ /**
+ * Returns whether the given project uses a modular source hierarchy,
either
+ * by declaring modular source elements in its build model or via enabled
+ * modular source roots.
+ *
+ * @param project the project to check
+ * @return {@code true} if the project uses a modular source hierarchy
+ * @since 4.1.0
+ */
+ public static boolean usesModuleSourceHierarchy(MavenProject project) {
+ if (project.getBuild() != null && project.getBuild().getDelegate() !=
null) {
+ if
(usesModuleSourceHierarchy(project.getBuild().getDelegate().getSources())) {
+ return true;
+ }
+ }
+ Collection<SourceRoot> roots = project.getSourceRoots();
+ return roots != null
Review Comment:
💡 **Redundant `null` guard on `getSourceRoots()`**
```java
Collection<SourceRoot> roots = project.getSourceRoots();
return roots != null
```
`MavenProject.getSourceRoots()` returns the field directly and the field is
initialised in the no-arg constructor — it is never `null`. The `roots != null`
guard is dead code. Simplify to:
```suggestion
return project.getSourceRoots().stream()
.filter(SourceRoot::enabled)
.anyMatch(sr -> sr.module().isPresent());
```
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultProject.java:
##########
@@ -108,9 +112,34 @@ public List<ProducedArtifact> artifacts() {
@Nonnull
@Override
public Packaging packaging() {
+ if (packaging != null && packaging.producesMainArtifact() &&
SourceQueries.usesModuleSourceHierarchy(project)) {
+ return new DelegatingPackaging(packaging, false);
+ }
Review Comment:
⚠️ **`packaging()` allocates a new `DelegatingPackaging` wrapper on every
call**
```java
if (packaging != null && packaging.producesMainArtifact() &&
SourceQueries.usesModuleSourceHierarchy(project)) {
return new DelegatingPackaging(packaging, false);
}
return packaging;
```
Every call to `packaging()` re-evaluates
`SourceQueries.usesModuleSourceHierarchy(project)`, which streams
`project.getSourceRoots()`, and allocates a fresh `DelegatingPackaging` when
the condition holds. In a large multi-module reactor this will be called
repeatedly. A cached field (lazily populated, invalidated on source-root
mutation if needed) would be safer and more efficient.
More fundamentally: returning `new DelegatingPackaging(packaging, false)`
means callers that do `project.packaging() == project.packaging()` or
`project.packaging() == session.requirePackaging("jar")` get inconsistent
results, which is exactly the kind of breakage the `@Immutable` annotation on
`Packaging` is meant to prevent.
##########
impl/maven-core/src/main/java/org/apache/maven/ReactorReader.java:
##########
@@ -463,12 +464,29 @@ private Stream<Artifact> getProjectArtifacts(MavenProject
project) {
// pom artifact
Stream.of(new ProjectArtifact(project)),
// main project artifact if not a pom
- "pom".equals(project.getPackaging()) ? Stream.empty()
: Stream.of(project.getArtifact())),
+ !producesMainArtifact(project) ? Stream.empty() :
Stream.of(project.getArtifact())),
// attached artifacts
project.getAttachedArtifacts().stream());
return artifacts.map(RepositoryUtils::toArtifact);
}
+ boolean producesMainArtifact(MavenProject project) {
Review Comment:
💡 **Package-private visibility — intentional?**
`producesMainArtifact(MavenProject)` has no access modifier
(package-private). The test class `ReactorReaderTest` is in the same package,
which is why the tests compile — but is the package-private visibility
intentional, or should this be `private`? If it's meant to be testable via the
package, add a brief comment explaining the intent; otherwise `private` is more
appropriate for an internal implementation detail.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
