arturobernalg commented on PR #13591: URL: https://github.com/apache/maven/pull/13591#issuecomment-6283063291
The previous push broke CI: all `integration-tests` jobs failed on `MavenITmng3297DependenciesNotLeakedToMojoTest`, which asserts exactly the opposite of what I assumed — that a mojo with no dependency requirement must see **zero** project dependencies, not whatever the previous mojo in the build happened to leave there. A `null` artifact filter is itself a meaningful write: `ensureDependenciesAreResolved` sets it to clear any filter an earlier mojo set, so the next mojo doesn't see stale/leaked state. Skipping that write when the mojo has no requirement (as I did to let it skip the lock) left the previous mojo's filter in place instead, which is exactly the MNG-3297 regression. So the premise "a mojo with no requirement doesn't write to the project" was wrong — it always writes, the write is just `null` when there's no requirement, and that `null` write still needs to happen, and still needs the same protection against a concurrent sibling execution. I've reverted to taking the lock and replacing the filter for every mojo, i.e. the original version of this PR, which I confirmed was CI-green across all 9 OS/JDK integration-tests jobs before this review round started. I don't see a way to narrow the lock further without either dropping the MNG-3297 guarantee or digging into separating "configure" from "execute" inside `pluginManager.executeMojo()`, which felt like a bigger, riskier change than this PR should take on. Let me know if you see an angle I'm missing. -- 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]
