Skip to content

Move away from legacy maven-compat - #151

Closed
cstamas wants to merge 2 commits into
masterfrom
switch-to-resolver
Closed

cstamas wants to merge 2 commits into
masterfrom
switch-to-resolver

Conversation

@cstamas

@cstamas cstamas commented Sep 26, 2022

Copy link
Copy Markdown
Member

And switch to resolver to resolver annotation processors with dependencies.

And switch to resolver to resolver annotation processors
with dependencies.
@cstamas cstamas self-assigned this Sep 26, 2022
Comment thread pom.xml
<version>3.3.0</version>
<scope>test</scope>
</dependency>
<!-- maven-compat: is must due AbstractMojoTestCase :( -->

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I found that if you don not need to Inject parameters into the Mojo and thus do not test the Mojos run method, one can also go with PlexusTestcase see:
https://github.com/eclipse-tycho/tycho/blob/fdda7227e6cd714c2e120f70dd624cde2cca4ce1/tycho-p2/tycho-p2-facade/src/test/java/org/eclipse/tycho/p2/facade/test/MetadataSerializableImplTest.java

Still, I think this is a bit of a "blind-spot" that there is not a JUnit5 / Non-Compat replacement for a MojoTestCase.

}
else
{
failed.add( resolved );

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can this actually happen or would this not just throw a DependencyResolutionException already?

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.

I believe you are right. If the one the dependencies cannot be resolved, DependencyResolutionException is thrown on line 1858, so the manual handling of failed artifacts should no be needed?

Caused by: org.apache.maven.plugin.MojoExecutionException: Resolution of annotationProcessorPath dependencies failed: Could not find artifact org.issue:annotation-processor2:jar:1.0-SNAPSHOT in local.central (file:///home/psiroky/.m2/repository)
	at org.apache.maven.plugin.compiler.AbstractCompilerMojo.resolveProcessorPathEntries(AbstractCompilerMojo.java:1884)
	at org.apache.maven.plugin.compiler.AbstractCompilerMojo.execute(AbstractCompilerMojo.java:792)
	at org.apache.maven.plugin.compiler.CompilerMojo.execute(CompilerMojo.java:205)
	at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:137)
	at org.apache.maven.lifecycle.internal.MojoExecutor.doExecute2(MojoExecutor.java:370)
	... 20 common frames omitted
Caused by: org.eclipse.aether.resolution.DependencyResolutionException: Could not find artifact org.issue:annotation-processor2:jar:1.0-SNAPSHOT in local.central (file:///home/psiroky/.m2/repository)
	at org.eclipse.aether.internal.impl.DefaultRepositorySystem.resolveDependencies(DefaultRepositorySystem.java:374)
	at org.apache.maven.plugin.compiler.AbstractCompilerMojo.resolveProcessorPathEntries(AbstractCompilerMojo.java:1858)
	... 24 common frames omitted
Caused by: org.eclipse.aether.resolution.ArtifactResolutionException: Could not find artifact org.issue:annotation-processor2:jar:1.0-SNAPSHOT in local.central (file:///home/psiroky/.m2/repository)
	at org.eclipse.aether.internal.impl.DefaultArtifactResolver.resolve(DefaultArtifactResolver.java:431)
	at org.eclipse.aether.internal.impl.DefaultArtifactResolver.resolveArtifacts(DefaultArtifactResolver.java:235)
	at org.eclipse.aether.internal.impl.DefaultRepositorySystem.resolveDependencies(DefaultRepositorySystem.java:357)
	... 25 common frames omitted
Caused by: org.eclipse.aether.transfer.ArtifactNotFoundException: Could not find artifact org.issue:annotation-processor2:jar:1.0-SNAPSHOT in local.central (file:///home/psiroky/.m2/repository)
	at org.eclipse.aether.connector.basic.ArtifactTransportListener.transferFailed(ArtifactTransportListener.java:48)
	at org.eclipse.aether.connector.basic.BasicRepositoryConnector$TaskRunner.run(BasicRepositoryConnector.java:369)
	at org.eclipse.aether.util.concurrency.RunnableErrorForwarder.lambda$wrap$0(RunnableErrorForwarder.java:73)
	at org.eclipse.aether.connector.basic.BasicRepositoryConnector$DirectExecutor.execute(BasicRepositoryConnector.java:627)
	at org.eclipse.aether.connector.basic.BasicRepositoryConnector.get(BasicRepositoryConnector.java:262)
	at org.eclipse.aether.internal.impl.DefaultArtifactResolver.performDownloads(DefaultArtifactResolver.java:520)
	at org.eclipse.aether.internal.impl.DefaultArtifactResolver.resolve(DefaultArtifactResolver.java:408)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Correct. In addition, result.getExceptions() will contain the list of all gathered exceptions (if there are more than one).

@psiroky

psiroky commented Jan 18, 2023 •

Copy link
Copy Markdown
Contributor

@cstamas @laeubi Hello! It would be great to get this one merged. Is there anything I can help with?

It seems like getting rid of AbstractMojoTestCase might be bigger endeavor and possibly outside of scope here? CompilerMojoTestCase is using quite a few method from there, so either we would need to (temporarily?) copy those directly into this class, or completely re-work the test to not rely on those.

To clarify why I am even commenting here: I would like to take a look at some of the bugs / feature requests related to annotationProcessorPaths, so having this one merged seems like a good per-requisite.

Edit: Just FYI - I tried to locally rebase on top of the latest master and all the tests are passing, so the change still looks good.

@cstamas

cstamas commented Jan 19, 2023

Copy link
Copy Markdown
Member Author

Am fine if anyone (@psiroky ?) picks up these changes and continue with them (as there is no need for else branch for example), no need for any attribution, just go on with it.

@andrzejj0

Copy link
Copy Markdown

Hi, I can help as well. I've done similar jobs for versions, enforcer and mrm already.

@cstamas

cstamas commented Jan 19, 2023

Copy link
Copy Markdown
Member Author

Toss a coin then 🪙

I am currently unable to continue this PR, and I'd hate anyone being hindered by sitting and waiting for this PR to land...

@andrzejj0

Copy link
Copy Markdown

@psiroky let me know if you want to do it, otherwise I'll do it

@psiroky

psiroky commented Jan 19, 2023

Copy link
Copy Markdown
Contributor

@cstamas @ajarmoniuk I'll take a look then. I think it will be a nice way way to get a bit more understanding how the dependency resolution stuff works. I would possibly reach out to @ajarmoniuk in case of any issues / questions (if that's ok).

psiroky added a commit to psiroky/maven-compiler-plugin that referenced this pull request Jan 19, 2023
…hs' dependencies

 * heavily based on work by @cstamas (PR apache#151)

 * added IT to verify the build fails if one the dependencies
   cannot be resolved (does not exist)
psiroky added a commit to psiroky/maven-compiler-plugin that referenced this pull request Jan 19, 2023
…hs' dependencies

 * heavily based on work by @cstamas (PR apache#151)

 * added IT to verify the build fails if one the dependencies
   cannot be resolved (does not exist)
@psiroky

psiroky commented Jan 19, 2023

Copy link
Copy Markdown
Contributor

I think we can close this one in favor of #169. Thanks @cstamas for the initial changes!

psiroky added a commit to psiroky/maven-compiler-plugin that referenced this pull request Jan 19, 2023
…hs' dependencies

 * heavily based on work by @cstamas (PR apache#151)

 * added IT to verify the build fails if one the dependencies
   cannot be resolved (does not exist)
@cstamas

cstamas commented Jan 19, 2023

Copy link
Copy Markdown
Member Author

Superseded by #169

@cstamas cstamas closed this Jan 19, 2023
@cstamas
cstamas deleted the switch-to-resolver branch January 19, 2023 17:20
psiroky added a commit to psiroky/maven-compiler-plugin that referenced this pull request Jan 19, 2023
…hs' dependencies

 * heavily based on work by @cstamas (PR apache#151)

 * added IT to verify the build fails if one the dependencies
   cannot be resolved (does not exist)
slawekjaranowski pushed a commit that referenced this pull request Jan 22, 2023
…hs' dependencies

 * heavily based on work by @cstamas (PR #151)

 * added IT to verify the build fails if one the dependencies
   cannot be resolved (does not exist)
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.

4 participants