You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2023/01/16 07:27:37 UTC
[GitHub] [maven] mthmulders commented on a diff in pull request #954: [MNG-7629] Change reactor reader to copy packaged artifacts and reuse them across builds if needed
mthmulders commented on code in PR #954:
URL: https://github.com/apache/maven/pull/954#discussion_r1070879137
##########
maven-core/src/main/java/org/apache/maven/ReactorReader.java:
##########
@@ -324,4 +300,113 @@ private static boolean isTestArtifact(Artifact artifact) {
return ("test-jar".equals(artifact.getProperty("type", "")))
|| ("jar".equals(artifact.getExtension()) && "tests".equals(artifact.getClassifier()));
}
+
+ private File find(Artifact artifact) {
+ Path target = getArtifactPath(artifact);
+ return Files.isRegularFile(target) ? target.toFile() : null;
+ }
+
+ public void processProject(MavenProject project) {
+ List<Artifact> artifacts = new ArrayList<>();
+
+ artifacts.add(RepositoryUtils.toArtifact(new ProjectArtifact(project)));
+ if (!"pom".equals(project.getPackaging())) {
+ org.apache.maven.artifact.Artifact mavenMainArtifact = project.getArtifact();
+ artifacts.add(RepositoryUtils.toArtifact(mavenMainArtifact));
+ }
+ for (org.apache.maven.artifact.Artifact attached : project.getAttachedArtifacts()) {
+ artifacts.add(RepositoryUtils.toArtifact(attached));
+ }
+
+ for (Artifact artifact : artifacts) {
+ if (artifact.getFile() != null && artifact.getFile().isFile()) {
+ Path target = getArtifactPath(artifact);
+ try {
+ LOGGER.debug("Copying {} to project local repository", artifact);
+ Files.createDirectories(target.getParent());
+ Files.copy(
+ artifact.getFile().toPath(),
+ target,
+ StandardCopyOption.REPLACE_EXISTING,
+ StandardCopyOption.COPY_ATTRIBUTES);
+ } catch (IOException e) {
+ LOGGER.warn("Error while copying artifact to project local repository", e);
+ }
+ }
+ }
+ }
+
+ private Path getArtifactPath(Artifact artifact) {
+ String groupId = artifact.getGroupId();
+ String artifactId = artifact.getArtifactId();
+ String version = artifact.getBaseVersion();
+ String classifier = artifact.getClassifier();
+ String extension = artifact.getExtension();
+ Path repo = getProjectLocalRepo();
+ return repo.resolve(groupId)
+ .resolve(artifactId)
+ .resolve(artifactId
+ + "-" + version
+ + (classifier != null && !classifier.isEmpty() ? "-" + classifier : "")
+ + (extension != null && !extension.isEmpty() ? "." + extension : ""));
+ }
+
+ private Path getProjectLocalRepo() {
+ Path root = session.getRequest().getMultiModuleProjectDirectory().toPath();
+ return root.resolve("target").resolve("project-local-repo");
+ }
+
+ private MavenProject getProject(Artifact artifact) {
+ return getProjects()
+ .getOrDefault(artifact.getGroupId(), Collections.emptyMap())
+ .getOrDefault(artifact.getArtifactId(), Collections.emptyMap())
+ .getOrDefault(artifact.getBaseVersion(), null);
+ }
+
+ private Map<String, Map<String, Map<String, MavenProject>>> getProjects() {
+ // compute the projects mapping
+ if (projects == null) {
+ List<MavenProject> allProjects = session.getAllProjects();
+ if (allProjects != null) {
+ Map<String, Map<String, Map<String, MavenProject>>> map = new HashMap<>();
Review Comment:
I'd appreciate if you could add a comment explaining what all the string values mean. Something like
```java
// groupId -> (artifactId -> (version -> project)))
```
Just so that we poor humans have to parse all the code below in order to understand the data structure ;-)
##########
maven-core/src/main/java/org/apache/maven/ReactorReader.java:
##########
@@ -20,41 +20,33 @@
import javax.inject.Inject;
import javax.inject.Named;
+import javax.inject.Singleton;
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashSet;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Optional;
-import java.util.function.Function;
+import java.nio.file.StandardCopyOption;
+import java.util.*;
Review Comment:
Did you deliberately replace with a wildcard import? Personally, I'm not a big fan of them, but I don't know if we have a (formal) code style that says we can or should not use it.
##########
maven-core/src/main/java/org/apache/maven/ReactorReader.java:
##########
@@ -324,4 +300,113 @@ private static boolean isTestArtifact(Artifact artifact) {
return ("test-jar".equals(artifact.getProperty("type", "")))
|| ("jar".equals(artifact.getExtension()) && "tests".equals(artifact.getClassifier()));
}
+
+ private File find(Artifact artifact) {
+ Path target = getArtifactPath(artifact);
+ return Files.isRegularFile(target) ? target.toFile() : null;
+ }
+
+ public void processProject(MavenProject project) {
+ List<Artifact> artifacts = new ArrayList<>();
+
+ artifacts.add(RepositoryUtils.toArtifact(new ProjectArtifact(project)));
+ if (!"pom".equals(project.getPackaging())) {
+ org.apache.maven.artifact.Artifact mavenMainArtifact = project.getArtifact();
+ artifacts.add(RepositoryUtils.toArtifact(mavenMainArtifact));
+ }
+ for (org.apache.maven.artifact.Artifact attached : project.getAttachedArtifacts()) {
+ artifacts.add(RepositoryUtils.toArtifact(attached));
+ }
+
+ for (Artifact artifact : artifacts) {
+ if (artifact.getFile() != null && artifact.getFile().isFile()) {
+ Path target = getArtifactPath(artifact);
+ try {
+ LOGGER.debug("Copying {} to project local repository", artifact);
+ Files.createDirectories(target.getParent());
+ Files.copy(
+ artifact.getFile().toPath(),
+ target,
+ StandardCopyOption.REPLACE_EXISTING,
+ StandardCopyOption.COPY_ATTRIBUTES);
+ } catch (IOException e) {
+ LOGGER.warn("Error while copying artifact to project local repository", e);
Review Comment:
Shouldn't this be an error? If copying doesn't succeed, the build might well fail further down the line.
##########
maven-core/src/main/java/org/apache/maven/ReactorReader.java:
##########
@@ -324,4 +300,113 @@ private static boolean isTestArtifact(Artifact artifact) {
return ("test-jar".equals(artifact.getProperty("type", "")))
|| ("jar".equals(artifact.getExtension()) && "tests".equals(artifact.getClassifier()));
}
+
+ private File find(Artifact artifact) {
+ Path target = getArtifactPath(artifact);
+ return Files.isRegularFile(target) ? target.toFile() : null;
+ }
+
+ public void processProject(MavenProject project) {
+ List<Artifact> artifacts = new ArrayList<>();
+
+ artifacts.add(RepositoryUtils.toArtifact(new ProjectArtifact(project)));
+ if (!"pom".equals(project.getPackaging())) {
+ org.apache.maven.artifact.Artifact mavenMainArtifact = project.getArtifact();
+ artifacts.add(RepositoryUtils.toArtifact(mavenMainArtifact));
+ }
+ for (org.apache.maven.artifact.Artifact attached : project.getAttachedArtifacts()) {
+ artifacts.add(RepositoryUtils.toArtifact(attached));
+ }
+
+ for (Artifact artifact : artifacts) {
+ if (artifact.getFile() != null && artifact.getFile().isFile()) {
+ Path target = getArtifactPath(artifact);
+ try {
+ LOGGER.debug("Copying {} to project local repository", artifact);
Review Comment:
I'm a bit in doubt here - should we log at debug or at info? It might be of interest to the user, after all?
##########
maven-core/src/main/java/org/apache/maven/ReactorReader.java:
##########
@@ -324,4 +300,113 @@ private static boolean isTestArtifact(Artifact artifact) {
return ("test-jar".equals(artifact.getProperty("type", "")))
|| ("jar".equals(artifact.getExtension()) && "tests".equals(artifact.getClassifier()));
}
+
+ private File find(Artifact artifact) {
+ Path target = getArtifactPath(artifact);
+ return Files.isRegularFile(target) ? target.toFile() : null;
+ }
+
+ public void processProject(MavenProject project) {
+ List<Artifact> artifacts = new ArrayList<>();
+
+ artifacts.add(RepositoryUtils.toArtifact(new ProjectArtifact(project)));
+ if (!"pom".equals(project.getPackaging())) {
+ org.apache.maven.artifact.Artifact mavenMainArtifact = project.getArtifact();
+ artifacts.add(RepositoryUtils.toArtifact(mavenMainArtifact));
+ }
+ for (org.apache.maven.artifact.Artifact attached : project.getAttachedArtifacts()) {
+ artifacts.add(RepositoryUtils.toArtifact(attached));
+ }
+
+ for (Artifact artifact : artifacts) {
+ if (artifact.getFile() != null && artifact.getFile().isFile()) {
+ Path target = getArtifactPath(artifact);
+ try {
+ LOGGER.debug("Copying {} to project local repository", artifact);
+ Files.createDirectories(target.getParent());
+ Files.copy(
+ artifact.getFile().toPath(),
+ target,
+ StandardCopyOption.REPLACE_EXISTING,
+ StandardCopyOption.COPY_ATTRIBUTES);
+ } catch (IOException e) {
+ LOGGER.warn("Error while copying artifact to project local repository", e);
+ }
+ }
+ }
+ }
+
+ private Path getArtifactPath(Artifact artifact) {
+ String groupId = artifact.getGroupId();
+ String artifactId = artifact.getArtifactId();
+ String version = artifact.getBaseVersion();
+ String classifier = artifact.getClassifier();
+ String extension = artifact.getExtension();
+ Path repo = getProjectLocalRepo();
+ return repo.resolve(groupId)
+ .resolve(artifactId)
+ .resolve(artifactId
+ + "-" + version
+ + (classifier != null && !classifier.isEmpty() ? "-" + classifier : "")
+ + (extension != null && !extension.isEmpty() ? "." + extension : ""));
Review Comment:
Is it actually possible for `extension` to be `null`?
##########
maven-core/src/main/java/org/apache/maven/ReactorReader.java:
##########
@@ -324,4 +300,113 @@ private static boolean isTestArtifact(Artifact artifact) {
return ("test-jar".equals(artifact.getProperty("type", "")))
|| ("jar".equals(artifact.getExtension()) && "tests".equals(artifact.getClassifier()));
}
+
+ private File find(Artifact artifact) {
+ Path target = getArtifactPath(artifact);
+ return Files.isRegularFile(target) ? target.toFile() : null;
+ }
+
+ public void processProject(MavenProject project) {
Review Comment:
I think `processProject` is a bit of a vague name. As far as I can see, the method name is not part of a generic interface - so why not call it something like `installIntoProjectLocalRepository()`?
##########
maven-core/src/main/java/org/apache/maven/ReactorReader.java:
##########
@@ -237,21 +223,11 @@ private boolean isPackagedArtifactUpToDate(MavenProject project, File packagedAr
long outputFileLastModified =
Files.getLastModifiedTime(outputFile).toMillis();
if (outputFileLastModified > artifactLastModified) {
- File alternative = determineBuildOutputDirectoryForArtifact(project, artifact);
- if (alternative != null) {
- LOGGER.warn(
- "File '{}' is more recent than the packaged artifact for '{}'; using '{}' instead",
- relativizeOutputFile(outputFile),
- project.getArtifactId(),
- relativizeOutputFile(alternative.toPath()));
- } else {
- LOGGER.warn(
- "File '{}' is more recent than the packaged artifact for '{}'; "
- + "cannot use the build output directory for this type of artifact",
- relativizeOutputFile(outputFile),
- project.getArtifactId());
- }
- return false;
+ LOGGER.warn(
+ "File '{}' is more recent than the packaged artifact for '{}', please run a full `mvn verify` build",
Review Comment:
Should this suggestion read `verify` or `package`?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org