perNyfelt commented on code in PR #182:
URL:
https://github.com/apache/maven-resolver-ant-tasks/pull/182#discussion_r3856954155
##########
src/main/java/org/apache/maven/resolver/internal/ant/AntRepoSys.java:
##########
@@ -752,6 +753,13 @@ public CollectResult collectDependencies(
}
if (dependencies != null) {
+ if (dependencies.getPom() != null) {
+ // Maven collects for the project artifact; without a root
the project cannot be told apart
+ // from its own dependencies when it shows up in its graph.
+ Model model = dependencies.getPom().getModel(task);
+ collectRequest.setRootArtifact(
Review Comment:
**low** — two things about the new root artifact.
(a) The rationale in the comment above ("without a root the project cannot
be told apart from its own dependencies when it shows up in its graph") doesn't
hold for this resolver. In `maven-resolver-impl` 2.0.21,
`request.getRootArtifact()` is consumed in exactly two places — building the
root `DefaultDependencyNode`, and seeding `DefaultDependencyCollectionContext`
— and no shipped `DependencySelector`, `DependencyManager` or
`DependencyTraverser` ever reads `DependencyCollectionContext.getArtifact()`.
It changes nothing about cycle detection.
(b) Where it *is* visible, it diverges from Maven: the extension is
hard-coded to `"pom"`, whereas `DefaultProjectDependenciesResolver` uses
`RepositoryUtils.toArtifact(project.getArtifact())`, i.e. the project's
packaging. A POM with `<packaging>jar</packaging>` now logs its graph root as
`g:a:pom:v` under verbose `<resolve>` output where Maven shows `g:a:jar:v`, and
a user-supplied graph transformer inspecting the root sees the wrong extension.
Since parity is the point of the commit, `model.getPackaging()` is the value
to use.
##########
src/test/java/org/apache/maven/resolver/internal/ant/MavenParityTest.java:
##########
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.resolver.internal.ant;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.TreeSet;
+
+import junit.framework.JUnit4TestAdapter;
+import org.apache.maven.resolver.internal.ant.types.Dependencies;
+import org.apache.maven.resolver.internal.ant.types.Pom;
+import org.apache.maven.resolver.internal.ant.types.RemoteRepositories;
+import org.apache.maven.resolver.internal.ant.types.RemoteRepository;
+import org.apache.tools.ant.Project;
+import org.apache.tools.ant.Task;
+import org.eclipse.aether.artifact.Artifact;
+import org.eclipse.aether.collection.CollectResult;
+import org.eclipse.aether.graph.DependencyFilter;
+import org.eclipse.aether.graph.DependencyNode;
+import org.eclipse.aether.util.filter.DependencyFilterUtils;
+import org.eclipse.aether.util.graph.visitor.NodeListGenerator;
+import
org.eclipse.aether.util.graph.visitor.PreorderDependencyNodeConsumerVisitor;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Resolves a POM through the Ant tasks and compares the outcome with what
{@code mvn dependency:list}
+ * reports for the same POM, per classpath scope.
+ * <p>
+ * The expectations next to the fixture POM are recorded Maven output, not
hand-written; their headers carry
+ * the Maven version they came from and the command that re-records them. A
difference here is a difference
+ * between the two tools, which is the thing worth failing a build over.
+ */
+public class MavenParityTest {
+ public static junit.framework.Test suite() {
+ return new JUnit4TestAdapter(MavenParityTest.class);
+ }
+
+ private static final File FIXTURE_DIR =
+ new File(new File("").getAbsoluteFile(),
"src/test/resources/ant/MavenParity");
+
+ private Project project;
+
+ private Task task;
+
+ @Before
+ public void setUp() {
Review Comment:
**low** — `setUp` leaves `user.home` pointing at the real home directory and
never neutralizes the developer's `~/.m2/settings.xml`. Its sibling
`PomRepositoriesTest` deliberately installs an empty `settings.xml` for exactly
this reason.
The test then resolves 15+ artifacts from Central and asserts exact
versions. A developer or CI agent whose settings carry
`<offline>true</offline>`, or a mirror that doesn't proxy Central, gets three
failures whose message claims Ant and Maven disagree about the classpath —
pointing at a parity bug that isn't there.
Applying the same `setUserSettings` / `setGlobalSettings` isolation as
`PomRepositoriesTest` (the fixture already exists) makes the test measure what
it claims to measure.
##########
src/test/java/org/apache/maven/resolver/internal/ant/MavenParityTest.java:
##########
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.resolver.internal.ant;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.TreeSet;
+
+import junit.framework.JUnit4TestAdapter;
+import org.apache.maven.resolver.internal.ant.types.Dependencies;
+import org.apache.maven.resolver.internal.ant.types.Pom;
+import org.apache.maven.resolver.internal.ant.types.RemoteRepositories;
+import org.apache.maven.resolver.internal.ant.types.RemoteRepository;
+import org.apache.tools.ant.Project;
+import org.apache.tools.ant.Task;
+import org.eclipse.aether.artifact.Artifact;
+import org.eclipse.aether.collection.CollectResult;
+import org.eclipse.aether.graph.DependencyFilter;
+import org.eclipse.aether.graph.DependencyNode;
+import org.eclipse.aether.util.filter.DependencyFilterUtils;
+import org.eclipse.aether.util.graph.visitor.NodeListGenerator;
+import
org.eclipse.aether.util.graph.visitor.PreorderDependencyNodeConsumerVisitor;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Resolves a POM through the Ant tasks and compares the outcome with what
{@code mvn dependency:list}
+ * reports for the same POM, per classpath scope.
+ * <p>
+ * The expectations next to the fixture POM are recorded Maven output, not
hand-written; their headers carry
+ * the Maven version they came from and the command that re-records them. A
difference here is a difference
+ * between the two tools, which is the thing worth failing a build over.
+ */
+public class MavenParityTest {
+ public static junit.framework.Test suite() {
+ return new JUnit4TestAdapter(MavenParityTest.class);
+ }
+
+ private static final File FIXTURE_DIR =
+ new File(new File("").getAbsoluteFile(),
"src/test/resources/ant/MavenParity");
+
+ private Project project;
+
+ private Task task;
+
+ @Before
+ public void setUp() {
+ project = new Project();
+ project.setProperty("user.home", System.getProperty("user.home"));
+ project.setProperty(
+ "maven.repo.local",
+ new File(new File("").getAbsoluteFile(),
"target/ant/local-repo").getAbsolutePath());
+
+ task = new Task() {};
+ task.setProject(project);
+ }
+
+ @Test
+ public void testCompileClasspathMatchesMaven() throws IOException {
+ assertParity("compile");
+ }
+
+ @Test
+ public void testRuntimeClasspathMatchesMaven() throws IOException {
+ assertParity("runtime");
+ }
+
+ @Test
+ public void testTestClasspathMatchesMaven() throws IOException {
+ assertParity("test");
+ }
+
+ private void assertParity(String classpath) throws IOException {
+ List<String> expected = readExpectation(classpath);
+ List<String> actual = resolve(classpath);
+
+ assertEquals(
+ "the Ant tasks and Maven disagree about the " + classpath + "
classpath of "
+ + new File(FIXTURE_DIR, "pom.xml"),
+ String.join("\n", expected),
+ String.join("\n", actual));
+ }
+
+ private List<String> readExpectation(String classpath) throws IOException {
Review Comment:
**low** — `readExpectation` preserves the fixture's line order, while
`resolve()` returns a `TreeSet` in Java's natural `String` order, and the
assertion compares the two joined strings. The recorded fixtures happen to
agree today, but the re-record command in each expectation header is a bare
`sort`, which is locale-collated.
Re-recording under `en_US.UTF-8` (the default on most dev machines) can emit
an order `TreeSet` won't reproduce — punctuation-sensitive pairs like
`…:junit-jupiter-api:…` vs `…:junit-jupiter:jar:…` are exactly the shape that
diverges. The failure then reads "the Ant tasks and Maven disagree about the
test classpath", for a parity bug that doesn't exist.
Sorting `lines` before returning (or documenting `LC_ALL=C sort` in the
headers) removes the trap.
##########
src/main/java/org/apache/maven/resolver/internal/ant/AntRepoSys.java:
##########
@@ -791,69 +799,37 @@ private void populateCollectRequest(
if (dependencies.getPom() != null) {
Model model = dependencies.getPom().getModel(task);
- if (model.getDependencyManagement() != null) {
- for (org.apache.maven.model.Dependency manDep :
- model.getDependencyManagement().getDependencies()) {
- Dependency dependency = new Dependency();
- dependency.setArtifactId(manDep.getArtifactId());
- dependency.setClassifier(manDep.getClassifier());
- dependency.setGroupId(manDep.getGroupId());
- dependency.setScope(manDep.getScope());
- dependency.setType(manDep.getType());
- dependency.setVersion(manDep.getVersion());
- if (manDep.getSystemPath() != null
- && !manDep.getSystemPath().isEmpty()) {
-
dependency.setSystemPath(task.getProject().resolveFile(manDep.getSystemPath()));
- }
- for (org.apache.maven.model.Exclusion exc :
manDep.getExclusions()) {
- Exclusion exclusion = new Exclusion();
- exclusion.setGroupId(exc.getGroupId());
- exclusion.setArtifactId(exc.getArtifactId());
- exclusion.setClassifier("*");
- exclusion.setExtension("*");
- dependency.addExclusion(exclusion);
- }
- collectRequest.addManagedDependency(
- ConverterUtils.toManagedDependency(dependency,
globalExclusions, session));
- }
+ ArtifactDescriptorResult descriptor =
ConverterUtils.toArtifactDescriptor(session, model);
+
+ if (!descriptor.getRepositories().isEmpty()) {
+ collectRequest.setRepositories(getRemoteRepoMan()
Review Comment:
**medium** — the POM's repositories are aggregated unconditionally,
including when the caller passed an explicit `<remoterepos>` /
`resolver.repositories`.
Because the effective model inherits the super POM,
`descriptor.getRepositories()` always contains Maven Central even for a POM
that declares none — `PomRepositoriesTest` pins exactly that. So the guard
`!descriptor.getRepositories().isEmpty()` is effectively always true.
Concrete scenario: an internal build sets `resolver.repositories` to only a
corporate Nexus and has no `<mirrorOf>*</mirrorOf>`. Today a coordinate missing
from Nexus fails the build; after this change the collect request also carries
Central and the artifact is fetched from there instead. That is new network
egress and a dependency-confusion exposure for builds that were previously
bounded — a `<resolve>` that reached no external host may now do so.
The PR description flags this already. If it ships as-is it wants a property
gate, or at minimum a release note stating that an explicit `<remoterepos>` is
no longer a bound on resolution.
--
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]
