gnodet commented on code in PR #12069:
URL: https://github.com/apache/maven/pull/12069#discussion_r3284326492
##########
impl/maven-core/src/main/java/org/apache/maven/ReactorReader.java:
##########
@@ -217,13 +218,43 @@ private File
determineBuildOutputDirectoryForArtifact(final MavenProject project
if (projectHasOutputFromPreviousSession ||
projectCompiledDuringThisSession) {
return outputDirectory;
}
+
+ if (hasSiteLifecyclePhase(project) &&
"jar".equals(artifact.getExtension()) && "jar".equals(type)) {
+ return ensureEmptyArtifactFile(artifact);
+ }
}
// The fall-through indicates that the artifact cannot be found;
// for instance if package produced nothing or classifier problems.
return null;
}
+ private File ensureEmptyArtifactFile(final Artifact artifact) {
+ Path target = getArtifactPath(artifact);
+ synchronized (this) {
+ if (Files.isRegularFile(target)) {
Review Comment:
Using `synchronized(this)` here locks the entire `ReactorReader` for every
artifact resolution, which could become a bottleneck in large reactor builds.
Since you only need mutual exclusion per target path, consider using a
`ConcurrentHashMap<Path, Path>` with `computeIfAbsent` to avoid global
contention:
```java
private final ConcurrentHashMap<Path, File> emptyArtifactCache = new
ConcurrentHashMap<>();
private File ensureEmptyArtifactFile(final Artifact artifact) {
Path target = getArtifactPath(artifact);
return emptyArtifactCache.computeIfAbsent(target, p -> {
// create empty jar ...
});
}
```
##########
impl/maven-core/src/main/java/org/apache/maven/ReactorReader.java:
##########
@@ -217,13 +218,43 @@ private File
determineBuildOutputDirectoryForArtifact(final MavenProject project
if (projectHasOutputFromPreviousSession ||
projectCompiledDuringThisSession) {
return outputDirectory;
}
+
+ if (hasSiteLifecyclePhase(project) &&
"jar".equals(artifact.getExtension()) && "jar".equals(type)) {
Review Comment:
This checks whether the *producer* project has entered a site lifecycle
phase. But dependency resolution for a consumer module can happen before the
producer's mojos have started, meaning `project.hasLifecyclePhase("pre-site")`
etc. would all return `false` at the time we need it most.
Wouldn't it be more reliable to check the session's top-level goals instead?
Something like:
```suggestion
if (isSiteGoalRequested() &&
"jar".equals(artifact.getExtension()) && "jar".equals(type)) {
```
with a helper that inspects `session.getGoals()` for site-related goals.
That way the check doesn't depend on mojo execution order.
##########
impl/maven-core/src/main/java/org/apache/maven/ReactorReader.java:
##########
@@ -217,13 +218,43 @@ private File
determineBuildOutputDirectoryForArtifact(final MavenProject project
if (projectHasOutputFromPreviousSession ||
projectCompiledDuringThisSession) {
return outputDirectory;
}
+
+ if (hasSiteLifecyclePhase(project) &&
"jar".equals(artifact.getExtension()) && "jar".equals(type)) {
+ return ensureEmptyArtifactFile(artifact);
+ }
}
// The fall-through indicates that the artifact cannot be found;
// for instance if package produced nothing or classifier problems.
return null;
}
+ private File ensureEmptyArtifactFile(final Artifact artifact) {
+ Path target = getArtifactPath(artifact);
+ synchronized (this) {
+ if (Files.isRegularFile(target)) {
+ return target.toFile();
+ }
+ try {
+ Files.createDirectories(target.getParent());
+ try (JarOutputStream ignored = new
JarOutputStream(Files.newOutputStream(target))) {
+ // create an empty jar for site reports that inspect
reactor artifacts before package
+ }
+ return target.toFile();
Review Comment:
This empty JAR is written to the project-local-repo. If the user
subsequently runs `mvn package` (without `clean`),
`findInProjectLocalRepository` will find this empty JAR and
`isPackagedArtifactUpToDate` may consider it valid (if no output directory
exists yet). This could silently produce a broken build.
Consider either:
- Writing the empty JAR to a temporary/distinct location that won't be
picked up by normal artifact resolution, or
- Adding metadata (e.g., a marker file or attribute) so the reactor reader
knows to ignore it in non-site builds.
--
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]
