[MCOMPILER-503] Resolve all annotation processor dependencies together #170

psiroky · 2023-01-22T22:34:46Z

resolving the anootation processor paths in multiple requests can easily lead to duplicated dependencies, e.g. if two processors depend on the same transitive dependency. Ultimately, both versions would end-up on processorpath, but of course only one of them would be used (likely dependeing on the order of elements of processorpath)
the previous behavior also did not match the general rules of dependency resolution used by Maven itself and other plugins
this may potentially be a breaking change for certain corner cases, if for example some projects mistakenly rely on the order of elemenets in processorpath

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [MCOMPILER-XXX] - Fixes bug in ApproximateQuantiles,
where you replace MCOMPILER-XXX with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
You have run the integration tests successfully (mvn -Prun-its clean verify).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

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.

I hereby declare this contribution to be licensed under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

psiroky · 2023-01-22T22:42:22Z

I would like to create a test for this, but I am not sure which approach to take. Unit testing this seems somewhat not-relevant, since all the code does is calling maven-resolver, which does the actual heavy lifting.

Creating an integration test project is pretty easy, but how do I for example verify, that duplicated dependencies are not on processor path? Parsing he debug (-X) Maven output (where the modulepath elements are listed) seems pretty brittle. Is there a better way to do this?

And one more question related to ITs, and the dependencies they use: is it OK to depend on a third party dep (say junit) in the IT projects? Those deps would need to be downloaded from Maven central at runtime (test time). I noticed there are ITs doing that, but I would like to clarify if that's just an oversight, or a proper way of writing the tests (since maven-integration-testing seems to be a bit more strict on how the dependencies can be used by tests).

Edit: So I was looking around and found an IT which did almost what I needed, so I decided to use the same approach. The test contains simple annotation processor which checks (when processing the annotations) whether some classes are on classpath or not (that should be enough to figure out if the dependency resolution process is correct - e.g. there may be always other edge cases, but the plugin delegates the resolution to maven-resolver and as such can't be testing all the use cases itself). The test uses custom verify plugin, which does depend on other 3rd party deps -- but I hope that's ok, since the same approach is used in other tests as well.

* resolving the anootation processor paths in multiple requests can easily lead to duplicated dependencies, e.g. if two processors depend on the same transitive dependency. Ultimately, both versions would end-up on processorpath, but of course only one of them would be used (likely dependeing on the order of elements of processorpath) * the previous behavior also did match the general rules of dependency resolution used by Maven itself and other plugins * this may potentially be a breaking change for certain corner cases, if for example some projects mistakenly rely on the order of elemenets in processorpath

cstamas · 2023-01-24T07:10:00Z

One big fat warning about this approach, or in general for "resolving": if you look at old code carefully, you will see it did new CollectRequest( new Dependency( artifact, JavaScopes.RUNTIME ), project.getRemoteProjectRepositories() ) (pay attention to CollectRequest ctor), it used current artifact as root and resolved it's dependencies. What new code does is pushing all the enlisted artifacts as "direct dependencies" of some (imaginary) root (again, according to Javadoc of changed ctor for CollectRequest). This has slightly different meaning and semantics, especially with optional dependencies, see https://github.com/apache/maven-resolver/blob/master/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/selector/OptionalDependencySelector.java#L52-L56

The change in semantics comes from fact that new code will "push" all things +1 regarding depth (as they each are not root anymore, but are direct siblings on some imaginary root).

In this very case (compiler annotation processors) this is completely OK, but when you try to "mimic" what Maven does it will not result in same outcome, for example when Maven resolves a plugin, the plugin artifact is used as root, and hence, this approach in this PR cannot be applied to bootstrap IT here (see apache/maven-integration-testing#236 and example mentioned in it).

psiroky · 2023-01-24T22:21:15Z

@cstamas many thanks for the review (and the details around dependency resolution)! I am wondering - what else do we need to get this one merged?

cstamas · 2023-01-24T22:30:59Z

we need Slawek 😄 @slawekjaranowski

psiroky · 2023-01-24T22:37:46Z

we need Slawek

I see 😆. I thought you have the merge permission as well, sorry about the fuzz.

cstamas · 2023-01-24T22:41:03Z

I do, but two pairs of eyes are better than m one

slawekjaranowski · 2023-01-24T23:00:56Z

I don't see where plugin from annotation-verify is executed.

We can put such plugin in separate setup-xxx test with install goal and next use plugin in your IT test.

By the way ... configuration for invoker in project is to fix - I will take care about it.

psiroky · 2023-01-24T23:30:19Z

The plugin is used in the annotation-user project: https://github.com/apache/maven-compiler-plugin/pull/170/files#diff-82b7fbab158b18d75e40832a9a3edfee28aba32e67314a155f1bbc923f9b84e9R58. Just to make sure the annotation processor was actually executed and created those dummy files.

I agree we should reuse the plugin, instead of copying it (it is now in three tests I think). Do you want me to do that in this PR, or in the next one?

Edit: I pushed another commit with the extraction of the annotation-verify-plugin. The changes are not that big, mostly deleting the duplicated stuff. If needed I can also put that into new PR.

pom.xml

src/it/setup_annotation-verify-plugin/pom.xml

…e ITs

psiroky · 2023-01-29T17:02:09Z

@cstamas @slawekjaranowski hey, just checking in - is there anything else that needs to happen to get this one merged?

(I definitely don't want to be pushing you or anything, I am sure you have more than enough on your plate. Just trying to figure out if there are other changes needed or so.)

Stephan202

Thanks for resolving MCOMPILER-503, @psiroky 🙏

Stephan202 · 2023-01-30T05:15:58Z

...ion-verify-plugin/src/main/java/org.apache.maven.plugins.compiler.it/SourcePathReadGoal.java

@@ -80,13 +79,12 @@ private void assertGeneratedSourceFileFor( String sourceClass, List<String> sour
                try
                {
                    String[] nameParts = sourceClass.split( "\\." );
-                    String content = FileUtils.fileRead( f );
+                    String content = new String( Files.readAllBytes( f.toPath() ) );


I wonder: shouldn't this code call one of the two-arg String constructors and pass in the charset specified by project.build.sourceEncoding?

Yes, that would be ideal. The plugin is only used by ITs in this repo, which are fully under "our" control, so the risk of relying on the platform encoding seems small. In any case, I will take a look at this in my next PR.

The "fix" is now part of #173.

It could be definitely further improved, e.g. issuing a WARNing as maven-resources-plugin does when the encoding is not set. That being said, considering this plugin is only used in the ITs (and all of them are under our control), the solution seems fine to me (it will respect the source encoding if specified in the test project and will fail horribly if e.g. the encoding is unknown / wrong).

psiroky force-pushed the MCOMPILER-503 branch from dd58376 to 8c7c530 Compare January 24, 2023 06:19

cstamas approved these changes Jan 24, 2023

View reviewed changes

slawekjaranowski self-requested a review January 24, 2023 22:49

psiroky commented Jan 25, 2023

View reviewed changes

pom.xml Show resolved Hide resolved

psiroky commented Jan 25, 2023

View reviewed changes

pom.xml Show resolved Hide resolved

slawekjaranowski approved these changes Jan 26, 2023

View reviewed changes

src/it/setup_annotation-verify-plugin/pom.xml Show resolved Hide resolved

src/it/setup_annotation-verify-plugin/pom.xml Outdated Show resolved Hide resolved

[MCOMPILER-503] Extract and reuse annotation-verify-plugin in multipl…

627914d

…e ITs

psiroky force-pushed the MCOMPILER-503 branch from 7b38fc5 to 627914d Compare January 26, 2023 17:30

olamy added the bug Something isn't working label Jan 30, 2023

olamy merged commit 3f95d39 into apache:master Jan 30, 2023

Stephan202 reviewed Jan 30, 2023

View reviewed changes

psiroky deleted the MCOMPILER-503 branch January 30, 2023 09:15

gnodet added this to the 3.11.0 milestone Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MCOMPILER-503] Resolve all annotation processor dependencies together #170

[MCOMPILER-503] Resolve all annotation processor dependencies together #170

psiroky commented Jan 22, 2023 •

edited

Loading

psiroky commented Jan 22, 2023 •

edited

Loading

cstamas commented Jan 24, 2023 •

edited

Loading

psiroky commented Jan 24, 2023

cstamas commented Jan 24, 2023 •

edited

Loading

psiroky commented Jan 24, 2023

cstamas commented Jan 24, 2023

slawekjaranowski commented Jan 24, 2023

psiroky commented Jan 24, 2023 •

edited

Loading

psiroky commented Jan 29, 2023

Stephan202 left a comment

Stephan202 Jan 30, 2023

psiroky Jan 30, 2023

psiroky Jan 30, 2023

[MCOMPILER-503] Resolve all annotation processor dependencies together #170

[MCOMPILER-503] Resolve all annotation processor dependencies together #170

Conversation

psiroky commented Jan 22, 2023 • edited Loading

psiroky commented Jan 22, 2023 • edited Loading

cstamas commented Jan 24, 2023 • edited Loading

psiroky commented Jan 24, 2023

cstamas commented Jan 24, 2023 • edited Loading

psiroky commented Jan 24, 2023

cstamas commented Jan 24, 2023

slawekjaranowski commented Jan 24, 2023

psiroky commented Jan 24, 2023 • edited Loading

psiroky commented Jan 29, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Jan 30, 2023

Choose a reason for hiding this comment

psiroky Jan 30, 2023

Choose a reason for hiding this comment

psiroky Jan 30, 2023

Choose a reason for hiding this comment

psiroky commented Jan 22, 2023 •

edited

Loading

psiroky commented Jan 22, 2023 •

edited

Loading

cstamas commented Jan 24, 2023 •

edited

Loading

cstamas commented Jan 24, 2023 •

edited

Loading

psiroky commented Jan 24, 2023 •

edited

Loading