Skip to content

Commit

Permalink
Hacking
Browse files Browse the repository at this point in the history 
See spring-projectsgh-36724
  • Loading branch information
@snicoll
snicoll committed Aug 7, 2023
1 parent e7706b4 commit 9f5f9fc
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 29 deletions.
4 changes: 4 additions & 0 deletions ...pring-boot-maven-plugin/src/main/java/org/springframework/boot/maven/AbstractAotMojo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ public abstract class AbstractAotMojo extends AbstractDependencyFilterMojo {
@Parameter(property = "spring-boot.aot.compilerArguments")
private String compilerArguments;

protected final MavenSession getSession() {
return this.session;
}

@Override
public void execute() throws MojoExecutionException, MojoFailureException {
if (this.skip) {
Expand Down
54 changes: 25 additions & 29 deletions ...ng-boot-maven-plugin/src/main/java/org/springframework/boot/maven/ProcessTestAotMojo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,26 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;

import org.apache.maven.RepositoryUtils;
import org.apache.maven.artifact.Artifact;
import org.apache.maven.artifact.DefaultArtifact;
import org.apache.maven.artifact.handler.DefaultArtifactHandler;
import org.apache.maven.artifact.resolver.ArtifactResolutionRequest;
import org.apache.maven.artifact.resolver.ArtifactResolutionResult;
import org.apache.maven.artifact.resolver.ResolutionErrorHandler;
import org.apache.maven.plugin.MojoExecutionException;
import org.apache.maven.plugins.annotations.Component;
import org.apache.maven.plugins.annotations.LifecyclePhase;
import org.apache.maven.plugins.annotations.Mojo;
import org.apache.maven.plugins.annotations.Parameter;
import org.apache.maven.plugins.annotations.ResolutionScope;
import org.apache.maven.repository.RepositorySystem;
import org.eclipse.aether.RepositorySystem;
import org.eclipse.aether.collection.CollectRequest;
import org.eclipse.aether.resolution.ArtifactResult;
import org.eclipse.aether.resolution.DependencyRequest;
import org.eclipse.aether.resolution.DependencyResult;
import org.eclipse.aether.util.artifact.JavaScopes;
import org.eclipse.aether.util.filter.DependencyFilterUtils;

/**
* Invoke the AOT engine on tests.
Expand Down Expand Up @@ -97,20 +103,6 @@ public class ProcessTestAotMojo extends AbstractAotMojo {
@Parameter(defaultValue = "${project.build.directory}/spring-aot/main/classes", required = true)
private File generatedClasses;

/**
* Local artifact repository used to resolve JUnit platform launcher jars.
*/
@SuppressWarnings("deprecation")
@Parameter(defaultValue = "${localRepository}", required = true, readonly = true)
private org.apache.maven.artifact.repository.ArtifactRepository localRepository;

This comment has been minimized.

Sign in to view

Copy link
@cstamas

cstamas Aug 7, 2023

This triggers the warning, so yes, good change.

/**
* Remote artifact repositories used to resolve JUnit platform launcher jars.
*/
@SuppressWarnings("deprecation")
@Parameter(defaultValue = "${project.remoteArtifactRepositories}", required = true, readonly = true)
private List<org.apache.maven.artifact.repository.ArtifactRepository> remoteRepositories;

This comment has been minimized.

Sign in to view

Copy link
@cstamas

cstamas Aug 7, 2023

Same thing (but does not triggers warning): Aether RemoteRepository should be used instead (as below)

@Component
private RepositorySystem repositorySystem;

Expand Down Expand Up @@ -165,10 +157,10 @@ private URL[] addJUnitPlatformLauncher(URL[] classPath) throws Exception {
String version = getJUnitPlatformVersion();
DefaultArtifactHandler handler = new DefaultArtifactHandler("jar");
handler.setIncludesDependencies(true);
ArtifactResolutionResult resolutionResult = resolveArtifact(new DefaultArtifact(JUNIT_PLATFORM_GROUP_ID,
Set<Artifact> artifacts = resolveArtifact(new DefaultArtifact(JUNIT_PLATFORM_GROUP_ID,
JUNIT_PLATFORM_LAUNCHER_ARTIFACT_ID, version, null, "jar", null, handler));
Set<URL> fullClassPath = new LinkedHashSet<>(Arrays.asList(classPath));
for (Artifact artifact : resolutionResult.getArtifacts()) {
for (Artifact artifact : artifacts) {
fullClassPath.add(artifact.getFile().toURI().toURL());
}
return fullClassPath.toArray(URL[]::new);
Expand All @@ -185,16 +177,20 @@ private String getJUnitPlatformVersion() throws MojoExecutionException {
return version;
}

private ArtifactResolutionResult resolveArtifact(Artifact artifact) throws Exception {
ArtifactResolutionRequest request = new ArtifactResolutionRequest();
request.setArtifact(artifact);
request.setLocalRepository(this.localRepository);
request.setResolveTransitively(true);
request.setCollectionFilter(new RuntimeArtifactFilter());
request.setRemoteRepositories(this.remoteRepositories);
ArtifactResolutionResult result = this.repositorySystem.resolve(request);
this.resolutionErrorHandler.throwErrors(request, result);
return result;
private Set<Artifact> resolveArtifact(Artifact artifact) throws Exception {
CollectRequest collectRequest = new CollectRequest();
collectRequest.setRoot(RepositoryUtils.toDependency(artifact, null));
collectRequest.setRepositories(this.project.getRemotePluginRepositories());
DependencyRequest request = new DependencyRequest();
request.setCollectRequest(collectRequest);
request.setFilter(DependencyFilterUtils.classpathFilter(JavaScopes.RUNTIME));
DependencyResult dependencyResult = this.repositorySystem
.resolveDependencies(getSession().getRepositorySession(), request);
return dependencyResult.getArtifactResults()
.stream()
.map(ArtifactResult::getArtifact)
.map(RepositoryUtils::toArtifact)
.collect(Collectors.toSet());
}

}

7 comments on commit 9f5f9fc

@snicoll
Copy link
Owner Author

@snicoll snicoll commented on 9f5f9fc Aug 7, 2023

Choose a reason for hiding this comment

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

@cstamas could I use your expertise again? I am trying to remove use of localRepository and looking at what was done for the surefire plugin but I am not sure how/if it's taking the local repository into account.

@cstamas
Copy link

@cstamas cstamas commented on 9f5f9fc Aug 7, 2023

Choose a reason for hiding this comment

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

Resolver is unable to function without local repository... So am unsure what you ask for. Can you describe a bit more the intent and maybe link some related changes (in surefire)?

@snicoll
Copy link
Owner Author

@snicoll snicoll commented on 9f5f9fc Aug 7, 2023

Choose a reason for hiding this comment

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

Thanks for the quick reply!

Resolver is unable to function without local repository.

I didn't know that. I just want to make sure that it's not going to download something if it's in the configured local repository of the project. In short, I am trying to figure out if the change here is what we should be doing to remove the deprecation for ${localRepository}.

@cstamas
Copy link

@cstamas cstamas commented on 9f5f9fc Aug 7, 2023

Choose a reason for hiding this comment

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

Ah! It's more about the type ArtifactRepository that is deprecated and should not be used to get access to local repo. RepositorySystemSession has what you need. You can grab it by injecting MavenSession for example...

@cstamas
Copy link

@cstamas cstamas commented on 9f5f9fc Aug 7, 2023

Choose a reason for hiding this comment

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

Also avoid APIs that receive ArtifactRepository type, as it is usual sign they are also yo be avoided... Am not at desk, answering only via phone right now, will take a peek later

@snicoll
Copy link
Owner Author

@snicoll snicoll commented on 9f5f9fc Aug 7, 2023

Choose a reason for hiding this comment

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

Thanks. The diff here is all I need and I am wondering if that's idiomatic code or if something is missing.

@cstamas
Copy link

@cstamas cstamas commented on 9f5f9fc Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Change looks good. Aether (almost) all entry methods expect session, and session if you look, carries local repo as well...

Please sign in to comment.