Skip to content

Fix site lifecycle reactor dependency resolution#12069

Open
Will-thom wants to merge 1 commit into
apache:masterfrom
Will-thom:fix-site-reactor-dependencies-12064
Open

Fix site lifecycle reactor dependency resolution#12069
Will-thom wants to merge 1 commit into
apache:masterfrom
Will-thom:fix-site-reactor-dependencies-12064

Conversation

@Will-thom
Copy link
Copy Markdown

Fixes #12064

This updates Maven's reactor workspace resolution for site lifecycle builds so reports can resolve reactor JAR dependencies even when the producer module has not been compiled or packaged first.

The change keeps compile/default lifecycle behavior unchanged and adds an integration test covering a multi-module site build where a consumer module depends on a producer module. The test verifies that mvn site succeeds without running compile explicitly and without producing producer/target/classes.

This should be a candidate for backport to maven-3.10.x.

@slawekjaranowski
Copy link
Copy Markdown
Member

@Will-thom please preserve in PR description last paragraph from template:

To make clear that you license your contribution under the Apache License Version 2.0, January 2004 you have to acknowledge this by using the following check-box.
...

Copy link
Copy Markdown
Contributor

@gnodet gnodet left a comment

Choose a reason for hiding this comment

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

Claude Code on behalf of Guillaume Nodet

Thanks for the fix. The approach of synthesizing an empty JAR so site reports can resolve reactor dependencies is creative, but I have a few concerns about correctness and potential side effects.

Main concern: The hasSiteLifecyclePhase(project) check queries whether the producer project has already entered a site lifecycle phase. But determineBuildOutputDirectoryForArtifact is typically called when resolving dependencies of a consumer project. At that point, the producer module may not have started its own site phase yet (reactor order means the producer's mojos may not have fired). The lifecyclePhases set is populated by MojoStarted events, which only fire when a mojo actually runs on that project. So this guard could fail to trigger exactly when it's needed.

Could you verify that the producer project has indeed entered its site phase before the consumer tries to resolve it? If not, you may need a different detection mechanism (e.g., checking the session's goals list rather than per-project phase tracking).

Secondary concerns:

  • The empty JAR is written to project-local-repo and could persist across builds. A subsequent mvn compile package would find this empty JAR in the project-local-repo and might treat it as up-to-date, leading to a broken build.
  • synchronized(this) on the entire ReactorReader instance is coarse-grained; a per-path lock (or ConcurrentHashMap.computeIfAbsent) would be more appropriate for a workspace reader that can be called concurrently.

return outputDirectory;
}

if (hasSiteLifecyclePhase(project) && "jar".equals(artifact.getExtension()) && "jar".equals(type)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

Suggested change
if (hasSiteLifecyclePhase(project) && "jar".equals(artifact.getExtension()) && "jar".equals(type)) {
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.

private File ensureEmptyArtifactFile(final Artifact artifact) {
Path target = getArtifactPath(artifact);
synchronized (this) {
if (Files.isRegularFile(target)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

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 ...
    });
}

try (JarOutputStream ignored = new JarOutputStream(Files.newOutputStream(target))) {
// create an empty jar for site reports that inspect reactor artifacts before package
}
return target.toFile();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

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.

Site and dependencies in multi module project

3 participants