paulrutter commented on code in PR #566:
URL: https://github.com/apache/felix-dev/pull/566#discussion_r4194471440
##########
tools/maven-bundle-plugin/src/main/java/org/apache/felix/bundleplugin/BundleAllPlugin.java:
##########
@@ -229,16 +225,16 @@ protected BundleInfo bundleAll( MavenProject project, int
maxDepth ) throws Mojo
continue;
}
- throw new MojoExecutionException( "Artifact was not found in
the repo" + node.getArtifact(), e );
+ throw new MojoExecutionException( "Artifact was not found in
the repo" + nodeArtifact, e );
}
- node.getArtifact().setFile( artifact.getFile() );
+ nodeArtifact.setFile( artifact.getFile() );
Review Comment:
Nit: `toArtifact` builds a new `Artifact` on every call, so this `setFile`
no longer persists on the node as `node.getArtifact().setFile(...)` did. It is
not read again in this method, so it's fine today. It is a silent behaviour
change though, so a comment or removing the dead store would help. The same
call for the root node is the aliasing problem noted on `toArtifact`.
##########
tools/maven-bundle-plugin/src/main/java/org/apache/felix/bundleplugin/BundlePlugin.java:
##########
@@ -956,6 +954,37 @@ public boolean updateExcludesInDeps( MavenProject project,
List<Dependency> depe
}
+ /**
+ * Collects the dependency graph of the project after conflict resolution:
the root stands for the project, its
+ * children for the direct dependencies, and so on. Only POMs are
downloaded, no artifact files. This is what
+ * maven-dependency-tree's {@code DependencyGraphBuilder} did on top of
the same Maven API.
+ */
+ protected DependencyNode collectDependencyGraph( MavenProject project )
throws DependencyResolutionException
+ {
+ DependencyResolutionRequest request =
+ new DefaultDependencyResolutionRequest( project,
session.getRepositorySession() );
+ request.setResolutionFilter( ( node, parents ) -> false );
+ return projectDependenciesResolver.resolve( request
).getDependencyGraph();
+ }
+
+ /**
+ * The artifact of a node of {@link
#collectDependencyGraph(MavenProject)}, with the scope and optional flag of its
+ * dependency; for the root, which has no dependency, the artifact of the
project.
+ */
+ protected static Artifact toArtifact( DependencyNode node, MavenProject
project )
+ {
+ org.eclipse.aether.graph.Dependency dependency = node.getDependency();
+ if ( dependency == null )
+ {
+ return project.getArtifact();
Review Comment:
For the root node this returns the live `project.getArtifact()`, not a copy.
In `BundleAllPlugin.bundleAll` the root is the first node popped, so
`nodeArtifact.setFile( artifact.getFile() )` now writes onto the project's own
main artifact. With maven-dependency-tree the root artifact was a converted
copy, so this was harmless.
Scenario: `bundle:bundleall` on a project whose current coordinates already
exist in the local or remote repo, e.g. a released version being re-run.
`resolveArtifact` succeeds and the project's main artifact file is pointed at
the repository copy. Later steps that read `project.getArtifact().getFile()`
(attach, install, `bundleRoot` logging) then see the wrong file.
Suggest returning a copy for the root, e.g. `RepositoryUtils.toArtifact( new
DefaultArtifact(...) )` or `ArtifactUtils.copyArtifact( project.getArtifact()
)`. Alternatively, skip `setFile` when the node is the root.
--
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]
