[MNG-8015] Control the type of path where each dependency can be placed

[desruisseaux]

Introduces new API described in JIRA issue MNG-8015:

• PathType enumeration-like class.
• JavaPathType subtype with values such as MODULES, CLASSES, DOCLET, TAGLETS, etc.
• PATH_TYPES dependency property key (as a side-effect, the DependencyProperties API is modified for making it type-safe).
• DependencyResolverRequest API for allowing plugins to specify the PathType that the plugin wants.
• DependencyResolverResult.getDispatchedPath() method for getting the result: the paths to dependencies for each path type.

Open issues

Commit 9c5e9ff (which add implementation) introduces a dependency to Java 9 or later (the ModuleDescriptor class). If Maven 4 decides to stay executable on Java 8, this part will need to be refactored in a multi-version JAR file. If Maven 4 decides to require Java 9 or later, the source and target options in the pom.xml file should be replaced by a release option.

The implementation has no JUnit tests yet, so it may contain bug. We may want to write JUnit tests before to integrate this pull request. Waiting to see if a discussion causes a modification of this proposal before to write tests.

[desruisseaux]

CI build has been successful on Java 17 and 21, but failed on Java 11 because of the use of the {@return ...} javadoc tag. This is a Javadoc issue only. Resolution depends on Maven PMC decision about which Java version to require.

[cstamas]

If am not mistaken, this feature is available since Java 16? Given master is still Java 8 (build time requirement is same I think)... I'd stick with it.

So please use Java 8 conformant javadoc...

[gnodet]

I don't think the whole intern thing is needed at all.
I would suppose all references to Keys will be done using DependencyProperties.XXX, so I don't see a good use case for users to create duplicate instances.

[desruisseaux]

The intern() mechanism is for the org.apache.maven.internal.impl.DefaultDependency constructor. That constructor takes in argument an instance of org.eclipse.aether.graph.Dependency and copies its properties. But the Eclipse's properties are <String, String> entries, so the String keys need to be converted to DependencyProperties.Key instances. This is the purpose of DependencyProperties.Key.forName(…), which provides String → Key conversions. The intern() stuff is a side-effect of the need to support that conversion. It could be removed if DefaultDependency wasn't merely a wrapper for org.eclipse.aether.graph.Dependency.

[gnodet]

Do we need extensibility here ? Or could we use an enum ? @cstamas ?

[desruisseaux]

Even if we restrict the scope to standard Java tools, the extensibility is still needed for supporting the --patch-module option, which can be seen as class-paths specific to each module. So a new enumeration value needs to be created (but not internalized) for each module to patch. An alternative design would be to make JavaPathType an Enum and support the --path-module option in a separated implementation of PathType. Whether it would make the API simpler or not is unclear to me.

[cstamas]

We need extensibility, as tomorrow somebody could come up with custom package that would need some special handling... story is same as in Mave3 and ArtifactHandlers... they have to be extensible, that is the whole point.

[gnodet]

Would it make sense to remove the intern() method and redefine forName as:

        @Nullable
        public static <T> Key<T> forName(@Nonnull String name, @Nullable Class<T> defaultType, boolean intern) {
            Key<?> key;
            synchronized (INTERNS) {
                key = INTERNS.get(name);
                if (defaultType != null) {
                    if (key == null) {
                        key = new Key<>(name, defaultType);
                        if (intern) {
                            INTERNS.put(name, key);
                        }
                    } else if (key.valueType() != defaultType && intern) {
                        throw new IllegalStateException("Key " + name + " already exists for a different class of values.");
                    }
                }
            }
            return (Key) key;
        }

The constructor could be made private, so that the only entry point is the above method.

[desruisseaux]

Yes, we can do that.

[desruisseaux]

Actually the above code does not fit exactly the DefaultDependencies needs, because it forces the key to be of the type specified by defaultType. This is the correct thing to do for above method because its return value is Key<T>. However, DefaultDependencies needs to first check for a key with whatever type an existing key may have. So we cannot escape the need to have two methods, one with return value Key<?> and another one with return value Key<V>.

I will replace the existing forName method with a simpler one like below, checking only for existing key:

Optional<Key<?>> forName(String);

As a replacement of intern(), I considered adding the following method:

<V> Key<V> createAndCache(String name, Class<V> valueType);

However it would not work if some plugin wants to define its own subclass of Key, while intern() allows this flexibility. I'm not sure if it is a flexibility worth to have however.

[desruisseaux]

Applied most of above comments (still thinking for a solution about the two remaining ones). The build continues to fail on Java 11, again because of Javadoc, but this new error is more annoying. The error message is "Header used out of sequence: <H4>". This error happens when HTML title headers are used in Javadoc for clear separation of sections. Javadoc verifies that the numbers are used in sequential order: <H4> inside <H3>, themselves inside <H2>, etc. A difficulty is that Javadoc uses <H1>, <H2>, etc. for its own purpose, so the first level available in the Javadoc of a method starts at <H4>. Javadoc verifies that we use the proper level, which is good. The problem is that the expected level changed somewhere between Java 11 and Java 17 (I think it was around Java 13). Java 13 (approximately) and later expects <H4>. Java 11 expected something else. All versions raise an error when the HTML header level is not what they expected.

So it is impossible (as far as I know) to have a code compilable on both Java 11 and Java 17 when HTML headers are used in Javadoc and Javadoc checks are enabled. If compilation or Javadoc generation passes on one, it fails on the other. Workaround can be one of the following:

• Do not use HTML headers.
• Disable Javadoc checks.
• Requires Java 17 for compilation but target Java 11 or 8 with --release option.

Which approach should we take? The last one would be the one allowing most gradual transition to newest Java versions (by allowing the use of most recent conventions right now) while preserving compatibility.

[gnodet]

@desruisseaux I've pushed a small rework of this PR on top of #1391 on my fork.
The squashed and rebased commit can be found at gnodet@c535260
The branch merged fork is https://github.com/gnodet/maven/commits/explicit-module-path

The main change is that I removed the DependenciesProperties in the other PR, so there are some adjustments which I'm not completely sure are working.... But the idea is that Type is extensible. I think PathType should be extensible too, but I could not find any location where that would be needed (apart when creating custom Types)....

[desruisseaux]

Thanks, it look good to me. I will try to add a few JUnit tests today or tomorrow.

[desruisseaux]

Added Javadoc on shortcut methods in Session interface. Sometime I had to guess, and there is questions previously flagged by @todo tag in b72d1ce. The test case gives the impression that the answer to those questions is that the dependency given in argument is included in the returned list, but it would be nice to said that in the javadoc if it is the case.

Added a convenience method:

@Nonnull
Map<PathType, List<Path>> resolveDependencies(
        @Nonnull DependencyCoordinate dependencyCoordinate,
        @Nonnull PathScope scope,
        @Nonnull Collection<PathType> desiredTypes);

Expanded an existing test case: after fetching the dependency paths, fetch again the same dependency paths but this time dispatched by type.

• First invoke above convenience method with Arrays.asList(JavaPathType.CLASSES, JavaPathType.MODULES):
  • The pom dependency is excluded, because it is placed neither on the class-path or module-path.
  • The junit dependency appears on the module-path because it has as Automatic-Module-Name attribute.
  • All other dependencies are on the class-path.
• Invoke the same method again, but this time with only Arrays.asList(JavaPathType.CLASSES):
  • The junit dependency is now on the class-path.
  • This is the mechanism by which a plugin can tell that it is not interested in JPMS.

Note: A test if failing in the "Maven Core ITs suite" module:

Error: MavenITmng5135AggregatorDepResolutionModuleExtensionTest.testit:61-
AbstractMavenIntegrationTestCase.assertTrue:359 [test-classes, classes]
==> expected: <true> but was: <false>

I don't know what is happening here. A search for AbstractMavenIntegrationTestCase in my repository clone found nothing.

[gnodet]

@desruisseaux fwiw, I've merge master branch into this branch at https://github.com/apache/maven/tree/explicit-module-path

[desruisseaux]

Thanks. Closing this pull request in favour of #1401 then.