[MNG-8015] Control the type of path where each dependency can be placed by desruisseaux · Pull Request #1378 · apache/maven

desruisseaux · 2024-01-13T13:29:16Z

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.

- Refactor some `MavenProject` methods for reducing code duplication. Their behavior should be as before. - Add `@Override` annotations.

Refactor `DependencyProperties` as a type-safe map of objects instead of a map of strings. This is in anticipation for the introduction of a `PathType` property in future commits. The type of property values is specified by `DependencyProperties.Key.valueType()`.

- Add a `PathType` enumeration (actually a class for making the enumeration extensible). - Add a `DependencyResolverResult.getDispatchedPath()` method, unimplemented for now. - Move `DefaultDependencyResolverResult` as a top-level class for further developments. - Some changes in `DefaultDependencyResolver` in anticipation for further developments.

- Add a `DependencyProperties.PATH_TYPES` property. - Deprecate `DependencyProperties.FLAG_CLASS_PATH_CONSTITUENT` and `isAddedToClasspath()` methods. - Add documentation for explaining better the context-sensitive interpretation of path constituents.

- Added implementation of `DependencyResolverResult.getDispatchedPaths()`. - The code in `DefaultDependencyResolver` has been refactored, but the new code should be equivalent to the previous code for the collections that existed before this commit. The significant changes are the new code for computing the new `dispatchedPaths` map.

Add types: - Type.MODULAR_JAR: unconditionally on the module-path. - Type.CLASSPATH_JAR: unconditionally on the class-path. - And more.

desruisseaux · 2024-01-13T13:42:28Z

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 · 2024-01-13T16:29:27Z

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...

maven-artifact/src/main/java/org/apache/maven/artifact/handler/ArtifactHandler.java

api/maven-api-core/src/main/java/org/apache/maven/api/DependencyProperties.java

gnodet · 2024-01-17T07:16:49Z

api/maven-api-core/src/main/java/org/apache/maven/api/DependencyProperties.java

+         * @see String#intern()
+         */
+        @SuppressWarnings("unchecked")
+        public Key<V> intern() {


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.

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.

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

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.

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.

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.

Yes, we can do that.

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.

api/maven-api-core/src/main/java/org/apache/maven/api/JavaPathType.java

api/maven-api-core/src/main/java/org/apache/maven/api/PathType.java

api/maven-api-core/src/main/java/org/apache/maven/api/DependencyProperties.java

api/maven-api-core/src/main/java/org/apache/maven/api/Type.java

api/maven-api-core/src/main/java/org/apache/maven/api/DependencyProperties.java

@return

…ges in method signatures, removal of {@return} Javadoc tags for compatibility with older compilers.

….isAddedToClassPath()` method.

…ting that interface, and the case of the `--patch-module` option an inner class named `Modular` also implementing the `PathType` interface. The latter class generalizes `--path-module` support to other kinds of module-dependent option if some appear in the future.

desruisseaux · 2024-01-20T16:49:09Z

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.

…pendencyProperties.PATH_TYPES`.

…ompilation issues on Java 11. This commit may be reverted in the future if Java 17 become a minimal requirement for compilation.

Changes: * Properties moved to Artifacts from Dependencies * snapshot check delegated to Aether Artifact * new types for build path, build path scope, dependency scope, language * changes propagated

…tc...

- Refactor some `MavenProject` methods for reducing code duplication. Their behavior should be as before. - Add `@Override` annotations.

# Conflicts: # api/maven-api-core/src/main/java/org/apache/maven/api/JavaPathType.java # api/maven-api-core/src/main/java/org/apache/maven/api/Packaging.java # api/maven-api-core/src/main/java/org/apache/maven/api/PathType.java # api/maven-api-core/src/main/java/org/apache/maven/api/services/DependencyResolverRequest.java # maven-core/src/main/java/org/apache/maven/artifact/handler/manager/DefaultArtifactHandlerManager.java # maven-core/src/main/java/org/apache/maven/internal/impl/DefaultDependency.java # maven-core/src/main/java/org/apache/maven/internal/impl/DefaultDependencyProperties.java # maven-core/src/main/java/org/apache/maven/internal/impl/DefaultDependencyResolver.java # maven-core/src/main/java/org/apache/maven/internal/impl/DefaultDependencyResolverResult.java # maven-core/src/main/java/org/apache/maven/internal/impl/DefaultType.java # maven-core/src/main/java/org/apache/maven/internal/impl/DefaultTypeRegistry.java # maven-core/src/main/java/org/apache/maven/internal/impl/PathModularizationCache.java # maven-core/src/main/java/org/apache/maven/internal/impl/types/ClasspathJarTypeProvider.java # maven-core/src/main/java/org/apache/maven/internal/impl/types/EjbClientTypeProvider.java # maven-core/src/main/java/org/apache/maven/internal/impl/types/EjbTypeProvider.java # maven-core/src/main/java/org/apache/maven/internal/impl/types/JarTypeProvider.java # maven-core/src/main/java/org/apache/maven/internal/impl/types/JavadocTypeProvider.java # maven-core/src/main/java/org/apache/maven/internal/impl/types/MavenPluginTypeProvider.java # maven-core/src/main/java/org/apache/maven/internal/impl/types/ModularJarTypeProvider.java # maven-core/src/main/java/org/apache/maven/internal/impl/types/TestJarTypeProvider.java

gnodet · 2024-01-31T22:37:12Z

@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 · 2024-02-03T13:43:28Z

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

Note: contains some "todo" about aspects that may need clarification.

…ce method and expand an existing test case.

However, those question still need to be answered.

desruisseaux · 2024-02-04T21:53:59Z

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 · 2024-02-07T00:34:09Z

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

desruisseaux · 2024-02-07T09:34:49Z

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

jira-importer · 2025-07-07T09:26:59Z

Resolve #9864

desruisseaux added 6 commits January 13, 2024 14:10

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

c0a838d

- Refactor some `MavenProject` methods for reducing code duplication. Their behavior should be as before. - Add `@Override` annotations.

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

a646ab4

Add types: - Type.MODULAR_JAR: unconditionally on the module-path. - Type.CLASSPATH_JAR: unconditionally on the class-path. - And more.

cstamas reviewed Jan 13, 2024

View reviewed changes

maven-artifact/src/main/java/org/apache/maven/artifact/handler/ArtifactHandler.java Show resolved Hide resolved

cstamas reviewed Jan 13, 2024

View reviewed changes

api/maven-api-core/src/main/java/org/apache/maven/api/DependencyProperties.java Outdated Show resolved Hide resolved

cstamas reviewed Jan 13, 2024

View reviewed changes

api/maven-api-core/src/main/java/org/apache/maven/api/DependencyProperties.java Outdated Show resolved Hide resolved

cstamas requested a review from gnodet January 15, 2024 19:44

gnodet requested changes Jan 17, 2024

View reviewed changes

desruisseaux added 3 commits January 20, 2024 14:16

First round of pull request comments resolution: typos and minor chan…

de2d353

…ges in method signatures, removal of {@return} Javadoc tags for compatibility with older compilers.

Remove the deprecated FLAG_CLASS_PATH_CONSTITUENT key and the `Type…

ab8782f

….isAddedToClassPath()` method.

desruisseaux added 2 commits January 21, 2024 16:31

Replace PathType[] by Set<PathType> for the property value of `De…

67bd4b9

…pendencyProperties.PATH_TYPES`.

Replace HTML headings by paragraph and bold characters for avoiding c…

3d55f24

…ompilation issues on Java 11. This commit may be reverted in the future if Java 17 become a minimal requirement for compilation.

cstamas mentioned this pull request Jan 24, 2024

[MNG-8026] Maven drives regarding scopes #1391

Merged

cstamas and others added 10 commits January 31, 2024 00:41

Step 2:

943fb04

Changes: * Properties moved to Artifacts from Dependencies * snapshot check delegated to Aether Artifact * new types for build path, build path scope, dependency scope, language * changes propagated

Step2b

37fb446

Remove unrelated

944c391

Remove unused

dcc0498

Add TODO

22b31ad

Undo ArtifactProperties and Artifact#isSnapshot changes for now

dc4b1e7

Fix UT

3e3aa04

Introduce "extensible enum" and provides registries, SPI providers, e…

2536929

…tc...

Use the new scopes

cf08727

Fix UT

ff413ae

gnodet and others added 7 commits January 31, 2024 18:31

Remove DependencyProperties interface

56b6b5a

Use a plain enum for DependencyScope which is not extensible

734910a

Remove unused BuildPath

3d2df72

Rename BuildPathScope -> PathScope

3b2bc64

Rename BuildPathScope -> PathScope

e40f20f

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

c535260

- Refactor some `MavenProject` methods for reducing code duplication. Their behavior should be as before. - Add `@Override` annotations.

Fix javadoc

fe3975f

gnodet self-requested a review February 2, 2024 08:55

desruisseaux added 3 commits February 4, 2024 18:50

Add Javadoc in Session interface and some @Nonnull annotations.

dd87384

Note: contains some "todo" about aspects that may need clarification.

Add `Session.resolveDependencies(..., Collection<PathType>) convenien…

cb2386c

…ce method and expand an existing test case.

Remove @todo tags not recognized by Javadoc.

b72d1ce

However, those question still need to be answered.

gnodet mentioned this pull request Feb 7, 2024

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

Merged

desruisseaux closed this Feb 7, 2024

Conversation

desruisseaux commented Jan 13, 2024

Open issues

Uh oh!

desruisseaux commented Jan 13, 2024

Uh oh!

cstamas commented Jan 13, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

desruisseaux commented Jan 20, 2024

Uh oh!

gnodet commented Jan 31, 2024

Uh oh!

desruisseaux commented Feb 3, 2024

Uh oh!

desruisseaux commented Feb 4, 2024

Uh oh!

gnodet commented Feb 7, 2024

Uh oh!

desruisseaux commented Feb 7, 2024

Uh oh!

jira-importer commented Jul 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants