[MNG-6357] Dependency order should be nearest first by belingueres · Pull Request #281 · apache/maven

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

                                    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

+        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

+            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

+                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

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?

jira-importer · 2025-07-06T09:07:49Z

Resolve #7501

gnodet

AI Code Review — PR #281

Verdict: ⚠️ Needs rebase

This PR changes RepositoryUtils.toArtifacts() to produce artifacts ordered by dependency tree depth (breadth-first / nearest-first) instead of depth-first. The approach using a TreeMap<Integer, List<Artifact>> keyed by depth is logically correct and addresses a legitimate issue (MNG-6357).

However, the PR is 4+ years stale and cannot merge.

Findings

#	Severity	File	Details
1	🔴 High	`RepositoryUtils.java`	Stale path — File moved to `impl/maven-core/...` by MNG-8346. Merge state is CONFLICTING.
2	🔴 High	`RepositoryUtilsTest.java`	Test file conflict — A `RepositoryUtilsTest.java` already exists on master (added by commit `25c80d8ece`). The PR's test class would need to be merged into the existing test file. Also, `@author` tags were removed from the codebase (commit `0b3246381d`).

Behavioral concern

This change affects every caller of RepositoryUtils.toArtifacts() — including plugin classpath construction and project artifact resolution. While breadth-first ordering is arguably more correct, any code that implicitly depends on the current depth-first ordering could break. The PR description acknowledges that IT-3813 "does not apply any longer," suggesting awareness of this risk. A full integration test run is essential after rebasing.

Recommendation

If MNG-6357 is still desired: (1) rebase onto current master, (2) merge tests into the existing RepositoryUtilsTest.java, (3) remove @author tag, (4) run the full integration test suite to validate no regressions.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

Reviewed 3 PRs: apache#144 (COMMENT), apache#1125 (COMMENT), apache#281 (COMMENT). 36 total reviews posted. ~23 PRs remaining. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

belingueres · 2026-07-15T02:13:19Z

Let me know if this patch is still needed to do the recommended changes.

gnodet

This PR (MNG-6357, opened August 2019) changes dependency ordering to breadth-first (nearest-first). After 7 years, the codebase has evolved significantly, making this PR unmergeable in its current state:

Target files relocated: All three changed files target maven-core/ which was relocated to impl/maven-core/ by MNG-8346. GitHub confirms the merge state is DIRTY/CONFLICTING.
Extensive code drift: RepositoryUtils.java has been modified by 21+ commits since August 2019, including two full reformats (MNG-7601/MNG-7650), a Resolver 2.0 upgrade (MNG-7994), and a package move to org.apache.maven.impl (MNG-8508). The PR's diff is based on a 2019 snapshot that no longer applies.
Missing required fallback: The original issue MNG-6357 explicitly states "there should be a system property to get the original order, just in case somebody needs it." This PR unconditionally replaces the ordering with no toggle or escape hatch.

Additionally, the recent comment from the author (July 2026) asks "Let me know if this patch is still needed." Given that MNG-6357 links to MNG-7852 ("is superseded by") and Maven 4.0 has undergone a major reorganization, the maintainers may want to clarify whether this issue is still relevant in Maven 4.x.

If the change is still desired, a fresh PR against impl/maven-core/ with current code style, the system-property fallback, and integration tests would be needed.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

belingueres · 2026-08-04T01:02:35Z

Hi @gnodet, I rebased the code.
Added code to configure which algorithm to use (setArtifactOrdering).
In addition to TreeMap based BFS originally proposed, I added a more memory efficient BFS2 alternative to choose from using List of Lists and a stack to keep the dependency trail.
Added assertions to verify the dependency trail lists.

elharo

What is this actually doing?

I'm a hard no on anything that changes the dependency graphs Maven builds. That's a compatibility disaster. It is very likely the dependency resolution algorithm Maven chose decades ago was the wrong choice, but that choice is made and cannot be changed now.

elharo · 2026-09-03T17:42:51Z

+        try {
+            return parser.parseLiteral(dependencyGraph);
+        } catch (IOException e) {
+            fail("Failed the parsing of the dependency node graph");


Failed to parse the

elharo · 2026-09-03T17:43:22Z

 import org.eclipse.aether.graph.Dependency;
+import org.eclipse.aether.graph.DependencyFilter;
+import org.eclipse.aether.graph.DependencyNode;
+import org.eclipse.aether.internal.test.util.DependencyGraphParser;


shouldn't depend on internal class from another package

elharo · 2026-09-03T17:44:01Z

+         */
+        BFS(RepositoryUtils::toArtifactsBFS),
+        /**
+         * Breadth-first traversal of the dependency graph, using a List of List to store artifacts by depth, and a stack to keep the dependency trail. This is the new behavior.


list of lists

elharo · 2026-09-03T17:45:28Z

        }
    }

+    private static void toArtifactsBFS(


why do we have several different BFS methods? what's different about them? Better names or comments or both are needed.

I left only one now

elharo · 2026-09-03T17:45:54Z

+        }
+    }
+
+    public static void toArtifactsBFS2(


This is public and the others are private?

fixed to private

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

Closed

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

jira-importer mentioned this pull request Feb 23, 2025

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

Open

gnodet reviewed Jul 10, 2026

View reviewed changes

gnodet reviewed Jul 25, 2026

View reviewed changes

belingueres force-pushed the MNG-6357 branch from 2f29c36 to 401cb38 Compare August 4, 2026 00:54

slachiewicz added the enhancement New feature or request label Aug 24, 2026

slachiewicz added this to the 4.1.0 milestone Aug 28, 2026

elharo self-requested a review August 28, 2026 12:41

elharo requested changes Sep 3, 2026

View reviewed changes

Fix apache#7501 - [MNG-6357] Dependency order should be nearest first

8019391

belingueres force-pushed the MNG-6357 branch from 401cb38 to 8019391 Compare September 5, 2026 23:20

Uh oh!

Conversation

belingueres commented Aug 31, 2019

Uh oh!

belingueres commented Aug 31, 2019

Uh oh!

rfscholte commented Aug 31, 2019

Uh oh!

michael-o commented Aug 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmhodges commented Dec 29, 2021

Uh oh!

cowwoc commented Feb 18, 2023

Uh oh!

jira-importer commented Jul 6, 2025

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

AI Code Review — PR #281

Findings

Behavioral concern

Recommendation

Uh oh!

belingueres commented Jul 15, 2026

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

belingueres commented Aug 4, 2026

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

belingueres Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

belingueres Sep 5, 2026 •

edited

Loading