gnodet commented on PR #12069: URL: https://github.com/apache/maven/pull/12069#issuecomment-4724214947
Thanks for working on this! A few thoughts after reviewing: **The underlying issue is real** — `mvn site` failing on multi-module projects with inter-module dependencies is a pain point, and requiring `mvn compile site` is not great UX. However, I have some concerns about the approach: **Silent failures for common configurations:** The PR injects empty JARs into the reactor when a site goal is detected. This works fine for basic site generation (project-info-reports, etc.), but many projects have Javadoc, JXR, SpotBugs, or other reports enabled in their POM that inspect actual bytecode or source paths resolved from the classpath. With this change, those reports would silently produce empty/incomplete output instead of failing with a clear error. Today the failure is explicit — you know you need `mvn compile site`. With the empty JAR workaround, it passes but the output is wrong. **Scope of the change:** Patching `ReactorReader` (a low-level artifact resolution component) with site-specific logic feels like the wrong layer. The site goal detection via hardcoded strings (`pre-site`, `post-site`, `:site`, etc.) is fragile and couples artifact resolution to lifecycle knowledge it shouldn't have. **Possible alternative:** Would it be better to address this at the lifecycle/phase level — e.g., having the site lifecycle declare a dependency on the compile phase, or handling this in the site plugin itself? This feels more like a feature/enhancement than a bug fix, and probably not a candidate for rc-6. What do you think? -- 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]
