gnodet-bot commented on code in PR #12069: URL: https://github.com/apache/maven/pull/12069#discussion_r4248519713
########## its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh12064SiteReactorDependenciesTest.java: ########## @@ -0,0 +1,55 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.it; + +import java.io.File; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertFalse; + +/** + * This is a test set for <a href="https://github.com/apache/maven/issues/12064";>GH-12064</a>. + */ +class MavenITgh12064SiteReactorDependenciesTest extends AbstractMavenIntegrationTestCase { + + @Test + void testSiteLifecycleResolvesReactorDependencies() throws Exception { + File testDir = extractResources("/gh-12064-site-reactor-dependencies"); + + Verifier verifier = newVerifier(testDir.getAbsolutePath()); Review Comment: With the `Path` fix above, update `newVerifier` to take the path directly (preferred, non-deprecated overload): ```suggestion Verifier verifier = newVerifier(testDir); ``` ########## impl/maven-core/src/main/java/org/apache/maven/ReactorReader.java: ########## @@ -217,13 +220,49 @@ private File determineBuildOutputDirectoryForArtifact(final MavenProject project if (projectHasOutputFromPreviousSession || projectCompiledDuringThisSession) { return outputDirectory; } + + if (isSiteGoalRequested() && "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 = getEmptyArtifactPath(artifact); + return emptyArtifactCache.computeIfAbsent(target, path -> { + if (Files.isRegularFile(path)) { + return path.toFile(); + } + try { + Files.createDirectories(path.getParent()); + try (JarOutputStream ignored = new JarOutputStream(Files.newOutputStream(path))) { + // create an empty jar for site reports that inspect reactor artifacts before package + } + return path.toFile(); + } catch (IOException e) { + LOGGER.warn("Unable to create empty reactor artifact for '{}'.", artifact, e); + return null; + } + }); + } + + private boolean isSiteGoalRequested() { + return session.getGoals().stream().anyMatch(ReactorReader::isSiteGoal); + } + + private static boolean isSiteGoal(String goal) { + return "pre-site".equals(goal) + || "site".equals(goal) + || "post-site".equals(goal) + || "site-deploy".equals(goal) + || goal.endsWith(":site") Review Comment: 🟡 **Over-broad matching** — `goal.endsWith(":site")` matches any plugin goal ending in `:site` (e.g. `mycompany-plugin:site`), not just `maven-site-plugin:site`. Consider restricting to the site plugin's prefix: ```suggestion || "site:site".equals(goal) ``` Or use the full FQGN form check: verify whether this is intentional (allowing 3rd-party site report plugins) or an unintended breadth. ########## its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh12064SiteReactorDependenciesTest.java: ########## @@ -0,0 +1,55 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.it; + +import java.io.File; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertFalse; + +/** + * This is a test set for <a href="https://github.com/apache/maven/issues/12064";>GH-12064</a>. + */ +class MavenITgh12064SiteReactorDependenciesTest extends AbstractMavenIntegrationTestCase { + + @Test + void testSiteLifecycleResolvesReactorDependencies() throws Exception { + File testDir = extractResources("/gh-12064-site-reactor-dependencies"); Review Comment: 🔴 **Compilation error** — `extractResources()` returns `Path`, not `File`. This won't compile. All other ITs use `Path testDir = extractResources(...)` and call `newVerifier(Path)`. The `File`-based `newVerifier(String)` overload is `@Deprecated`. ```suggestion Path testDir = extractResources("/gh-12064-site-reactor-dependencies"); ``` ########## impl/maven-core/src/main/java/org/apache/maven/ReactorReader.java: ########## @@ -217,13 +220,49 @@ private File determineBuildOutputDirectoryForArtifact(final MavenProject project if (projectHasOutputFromPreviousSession || projectCompiledDuringThisSession) { return outputDirectory; } + + if (isSiteGoalRequested() && "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 = getEmptyArtifactPath(artifact); + return emptyArtifactCache.computeIfAbsent(target, path -> { + if (Files.isRegularFile(path)) { + return path.toFile(); + } + try { + Files.createDirectories(path.getParent()); + try (JarOutputStream ignored = new JarOutputStream(Files.newOutputStream(path))) { + // create an empty jar for site reports that inspect reactor artifacts before package + } + return path.toFile(); + } catch (IOException e) { + LOGGER.warn("Unable to create empty reactor artifact for '{}'.", artifact, e); + return null; Review Comment: 🟡 **Silent failure** — `return null` after logging a warning means the artifact becomes unresolvable with no exception thrown. The `computeIfAbsent` call caches `null`, so subsequent requests for the same path will also return `null` silently. Consider throwing an `UncheckedIOException` so the build fails fast with a clear message, rather than silently degrading into a resolution failure downstream. ########## impl/maven-core/src/main/java/org/apache/maven/ReactorReader.java: ########## @@ -502,6 +555,13 @@ private Path getArtifactPath( + extension); } + private Path getEmptyArtifactRepo() { + if (emptyArtifactRepository == null) { Review Comment: 🔵 **Benign data race** — `emptyArtifactRepository` is a plain field (not `volatile`), so concurrent threads could initialize it multiple times under JMM. In practice this is harmless since all paths produce the same value, and it's consistent with the existing `getProjectLocalRepo()` pattern on line 565. Noted for completeness. ########## impl/maven-core/src/main/java/org/apache/maven/ReactorReader.java: ########## @@ -217,13 +220,49 @@ private File determineBuildOutputDirectoryForArtifact(final MavenProject project if (projectHasOutputFromPreviousSession || projectCompiledDuringThisSession) { return outputDirectory; } + + if (isSiteGoalRequested() && "jar".equals(artifact.getExtension()) && "jar".equals(type)) { Review Comment: 🔵 **Minor**: `isSiteGoalRequested()` creates a new stream on every call, but the session goals are immutable. Consider caching the result: ```suggestion if (isSiteGoalRequested() && "jar".equals(artifact.getExtension()) && "jar".equals(type)) { ``` This is harmless for normal builds (few reactor modules), but worth noting for large reactors. -- 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]
