[MNG-6357] Dependency order should be nearest first #281

belingueres · 2019-08-31T15:39:58Z

Save the artifacts temporarily in a Map ordered by tree depth, and only
after finishing iterating over the dependency tree are the artifacts
added to the collection in depth order.

Note: IT-core test needs to be added replacing IT 3813 which does not apply any longer for a maven version containing this change.

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [MNG-XXX] - Fixes bug in ApproximateQuantiles,
where you replace MNG-XXX with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
[] You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

belingueres · 2019-08-31T15:42:46Z

Please review this patch. Tested it and seems to solve:

MCOMPILER-317
Modulepath lists dependencies in wrong order mojohaus/exec-maven-plugin#91

rfscholte · 2019-08-31T15:53:44Z

Looks good at first sight. This code deserves a unittest, that's better than relying on plugins. I don't think the unittest is too hard to write.
I noticed you're not using diamond operators. Looking at the code it should be written with the new Map methods, but that requires Java 8. The moment that Maven will require Java 8 is getting closer

michael-o · 2019-08-31T18:16:15Z

@rfscholte We have more serious problems that introducing fancy Java 8 features.

Tibor17 · 2019-08-31T18:35:33Z

maven-core/src/main/java/org/apache/maven/RepositoryUtils.java

@@ -112,6 +113,27 @@ private static String nullify( String string )
    public static void toArtifacts( Collection<org.apache.maven.artifact.Artifact> artifacts,
                                    Collection<? extends DependencyNode> nodes, List<String> trail,
                                    DependencyFilter filter )
+    {
+        Map<Integer, List<org.apache.maven.artifact.Artifact>> artifactsByDepth =
+            new TreeMap<Integer, List<org.apache.maven.artifact.Artifact>>();


you can use diamond <>. It is Java 1.7 feature.

If i open IDEA, it shows me these issues related to Java version.

Tibor17 · 2019-08-31T18:36:32Z

maven-core/src/main/java/org/apache/maven/RepositoryUtils.java

+        Map<Integer, List<org.apache.maven.artifact.Artifact>> artifactsByDepth =
+            new TreeMap<Integer, List<org.apache.maven.artifact.Artifact>>();
+
+        ArrayList<org.apache.maven.artifact.Artifact> firstLevelArtifacts =


aaa, Collection or List better than ArrayList.

Tibor17 · 2019-08-31T18:37:06Z

maven-core/src/main/java/org/apache/maven/RepositoryUtils.java

+            new TreeMap<Integer, List<org.apache.maven.artifact.Artifact>>();
+
+        ArrayList<org.apache.maven.artifact.Artifact> firstLevelArtifacts =
+            new ArrayList<org.apache.maven.artifact.Artifact>( nodes.size() );


Tibor17 · 2019-08-31T18:37:25Z

maven-core/src/main/java/org/apache/maven/RepositoryUtils.java

+                List<org.apache.maven.artifact.Artifact> artifactsCurrentDepth = artifactsByDepth.get( currentDepth );
+                if ( artifactsCurrentDepth == null )
+                {
+                    artifactsCurrentDepth = new ArrayList<org.apache.maven.artifact.Artifact>();


jmhodges · 2021-12-29T05:08:08Z

The merge conflict is pretty straight-forward to fix (it's a simple collision in the pom.xml). Would love to see this go in! Happy to provide a new PR if that's preferable

Save the artifacts temporarily in a Map ordered by tree depth, and only after finishing iterating over the dependency tree are the artifacts added to the collection in depth order.

- used diamond <> operator. - changed ArrayList for List in variable declaration. - Added unit test.

- Re added maven-resolver-test-util as a test dependency. - Changed tests to JUnit 5.

cowwoc · 2023-02-18T18:32:03Z

Per https://maven.apache.org/download.cgi Maven 4.x requires JDK 8. What is needed to move this PR forward?

Tibor17 reviewed Aug 31, 2019

View reviewed changes

jmhodges mentioned this pull request Dec 29, 2021

[MNG-6357] Dependency order should be nearest first apache/maven-integration-testing#47

Open

belingueres added 3 commits December 30, 2021 18:02

[MNG-6357] Dependency order should be nearest first

d2df778

Save the artifacts temporarily in a Map ordered by tree depth, and only after finishing iterating over the dependency tree are the artifacts added to the collection in depth order.

Changes based on review:

12fe6fc

- used diamond <> operator. - changed ArrayList for List in variable declaration. - Added unit test.

Modifications after rebase

2f29c36

- Re added maven-resolver-test-util as a test dependency. - Changed tests to JUnit 5.

belingueres force-pushed the MNG-6357 branch from ab9deb4 to 2f29c36 Compare January 3, 2022 21:40

hboutemy force-pushed the master branch from a075afa to 6e50886 Compare March 24, 2024 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MNG-6357] Dependency order should be nearest first #281

[MNG-6357] Dependency order should be nearest first #281

belingueres commented Aug 31, 2019

belingueres commented Aug 31, 2019

rfscholte commented Aug 31, 2019

michael-o commented Aug 31, 2019

Tibor17 Aug 31, 2019

Tibor17 Aug 31, 2019

Tibor17 Aug 31, 2019

Tibor17 Aug 31, 2019

Tibor17 Aug 31, 2019

jmhodges commented Dec 29, 2021

cowwoc commented Feb 18, 2023

[MNG-6357] Dependency order should be nearest first #281

Are you sure you want to change the base?

[MNG-6357] Dependency order should be nearest first #281

Conversation

belingueres commented Aug 31, 2019

belingueres commented Aug 31, 2019

rfscholte commented Aug 31, 2019

michael-o commented Aug 31, 2019

Tibor17 Aug 31, 2019

Choose a reason for hiding this comment

Tibor17 Aug 31, 2019

Choose a reason for hiding this comment

Tibor17 Aug 31, 2019

Choose a reason for hiding this comment

Tibor17 Aug 31, 2019

Choose a reason for hiding this comment

Tibor17 Aug 31, 2019

Choose a reason for hiding this comment

jmhodges commented Dec 29, 2021

cowwoc commented Feb 18, 2023