[MNG-8026] Maven drives regarding scopes #1391

cstamas · 2024-01-24T11:34:05Z

Changes:

Step1: basically moved (copied, as resolver bits are deprecated) all the classes dealing with scopes (and their interpretation) to Maven.
Step 2: 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.

https://issues.apache.org/jira/browse/MNG-8026

...rovider/src/main/java/org/apache/maven/repository/internal/scopes/MavenDependencyScopes.java

...-provider/src/main/java/org/apache/maven/repository/internal/MavenRepositorySystemUtils.java

cstamas · 2024-01-24T17:15:05Z

The most important: someApiCall(project + buildPathScope) == buildPath but buildPath class is not done fully yet (just there as placeholder).

The from it, classpath = buildPath(classpathFilter), and modulepath= buildPath(modulepathFilter) and so on, and classpath filter should filter out this based on information from #1378

api/maven-api-core/src/main/java/org/apache/maven/api/services/LanguageManager.java

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

gnodet · 2024-01-24T20:51:19Z

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

+     * @return the artifact properties, never {@code null}
+     */
+    @Nonnull
+    ArtifactProperties getArtifactProperties();


I don't get this change. An Artifact is obtained from coordinates and as no knowledge on how it will be used. The same pointer could point to the same artifact, but with different Type, hence different properties. I think those do belong to the resolved. Dependency

Exactly, happens similarly as in Resolver:

IF "ad-hoc" resolution happened (i.e. plugin used resolve service) it is plugin that knows why it did that

IF artifact entered via project (as dependency) then you have dependency -> type -> artifact so properties are set.

We may need something more for this, still refining.

well, then they should be kept on the Dependency

I disagree here: Dependency is what you have in POMs (either project or transitive deps). So, when you resolve, you end up with bunch of Artifacts, NOT Dependencyies. Basically, that is why this is needed on Artifact and not Dependency (where is also needed, as properties from type should and could be still augmented).

Or in other words: see this PR apache/maven-resolver#411
Basically Resolver can be now asked to:

create Artifact as new Artifact("g:a:v", artifactTypeRegistry.get("annotation-processor")). (and this ctor is not new, in a way, you could do the same even with existing ctors, this was just a shorthand to make possible to use type with string "coords").

the to resolve it to have it's full transitive hull

and you again end up with "bunch of Artifacts", but you still want to sift thru them, filter them by properties

No dependency in scope, but you are STILL interested in Artifact properties....

Or similarly for ANY artifact, as if artifact you are resolving may have type="modular-jar", then the needed info will be still present in artifact.properties. Again, no dependencies involved, etc.

Am pretty much confident that this change is correct.

...we already have some properties (is added to classpath etc) no?

@rmannibucau

That method is one of the main sources of confusion 😄 This is what happens:

Maven2 had ArtifactHandler#isAddedToClasspath() which meant exactly that: "is artifact to be added to classpath" as Maven2 was aware only of classpath, nothing else.

in Maven3 resolver came, and introduced this flag: https://github.com/apache/maven-resolver/blob/master/maven-resolver-api/src/main/java/org/eclipse/aether/artifact/ArtifactProperties.java#L54-L62 (note the javadoc nor any string literal does not mention classpath or anything).

In development of Maven3 early, at one point these two were "shortcut into one": https://github.com/apache/maven/blob/master/maven-core/src/main/java/org/apache/maven/RepositoryUtils.java#L247-L268

so essentially the "classpath" notion in core coming from Maven2 and "buildpath" abstraction, intentionally defined in Resolver as such (new in Maven3), was blurred to mean "same thing", which was obviously not the intent.

Typical example: the POM packaging, see definition here:
https://github.com/apache/maven/blob/master/maven-core/src/main/java/org/apache/maven/internal/impl/types/PomTypeProvider.java

Long time ago, before BOMs were a thing, people used it to "group dependencies" (see for example this blog: https://blog.sonatype.com/2009/10/maven-tips-and-tricks-grouping-dependencies/).

In essence, you create a POM that has your grouped dependencies and deploy that. So you have:

G:A:pom:V +- D1 +- D2 +- D3

On consuming side, when you declare in your project dependency (yes, a dependency, not import as today you do with BOMs) like G:A:pom:V, hence as:

<dependency> <groupId>G</groupId> <artifactId>A</artifactId> <version>V</version> <type>pom</type> </dependency>

On resolving (for @gnodet collect+resolve) this project "compile" build path scope, assuming is the only dependency you have, you would end up with this list of artifacts: [D1, D2, D3] despite resolution started as P (root) where P had 1 dependency G:A:pom:V.

Simply put, the Type definition (lack of "constitutes build path") causes that intermediate "grouping POM" be completely omitted (left out) from the results, as it is not part of the build path.

Side note: the long time ago pattern of doing pom for deps is still accurate today and bom created more mess than helped there - but unrelated to the PR, just wanted to highlight this is still used for goods and not legacy.

Ok but it means we need a dependency handler as we have an artifact handler (think it is a bit complex).
Ultimately, do we need dependency outside the model (literally ) or can it not just be artifacts?
I understand what you mean but it seems like a lot of work for not much gain, no? Can't we merge it somehow in new api?

Side note: the long time ago pattern of doing pom for deps is still accurate today and bom created more mess than helped there - but unrelated to the PR, just wanted to highlight this is still used for goods and not legacy.

Ok but it means we need a dependency handler as we have an artifact handler (think it is a bit complex). Ultimately, do we need dependency outside the model (literally ) or can it not just be artifacts? I understand what you mean but it seems like a lot of work for not much gain, no? Can't we merge it somehow in new api?

There's no ArtifactHandler in the v4 api. It's called Type and it represents the usage of an Artifact as a Dependency. There's nothing to handle afaik on an Artifact: it's just a pointer to a file in a repository. How it's used needs to be taken care of and that's done in the Dependency which is associated to a Type and Scope and DependencyProperties. If we move things to the Artifact or merge Artifact and Dependency, I think that's a source of confusion.

Resolver also encodes "type" in Artifact#properties. It also exposed since eons ctors for DefaultArtifact where you could pass in ArtifactType. The type you declare (i.e. if in project, you have packaging -> type) or if "ad-hoc" by root dependency node type (where callar knows what it does).

Or in short:

artifact is a file decorated with coordinates and properties

dependency is an artifact having extras like scope, type, optionality using which you can "derive" an artifact as above

true, in Resolver "dependency" is ONLY a thing that comes from POM (ArtifactDescriptor) but it is being distilled to artifact (as in second bullet -> first bullet).

Basically "everything is an artifact", and "dependency is an artifact that enters the game as dependency". This is how Resolver abstracts.

Doing collect+resolve operation and ending up with list of sole Dependencies is what you say?

When you collect+resolve, you are interested in all, so even root (again, this is for "ad-hoc" operations, like javadoc may tell: "resolve me this taglet and its dependencies", in that case, javadoc plugin KNOWS root is taglet, but will still need to handle the rest of transitive hull, and place them or use that as needed).

@gnodet if I understand you correctly, sans "root", you consider everything else (the transitive hull) as "dependency"? Which kinda makes sense, as they DID enter the result collection as "dependencies of root"?

Resolver also encodes "type" in Artifact#properties. It also exposed since eons ctors for DefaultArtifact where you could pass in ArtifactType. The type you declare (i.e. if in project, you have packaging -> type) or if "ad-hoc" by root dependency node type (where callar knows what it does).

Or in short:

artifact is a file decorated with coordinates and properties

dependency is an artifact having extras like scope, type, optionality using which you can "derive" an artifact as above

true, in Resolver "dependency" is ONLY a thing that comes from POM (ArtifactDescriptor) but it is being distilled to artifact (as in second bullet -> first bullet).

Basically "everything is an artifact", and "dependency is an artifact that enters the game as dependency". This is how Resolver abstracts.

Yes, and I think that's where some confusion comes from. Everything is an artifact means that when you have an Artifact object, you don't really know what it is: it can be 5 different things:

a pointer to a file (with a version range): ArtifactCoordinate

a resolved artifact in a repository (a definite version): Artifact

a pointer to a dependency (version range + scope + type)

a resolved dependency: the resolver actually has org.eclipse.aether.graph.Dependency and that's what the v4 api wraps

an artifact generated by the project

Doing collect+resolve operation and ending up with list of sole Dependencies is what you say?

This is also a confusing part. It's conceptually a 3 step process: collect + flatten + resolve. The collect grabs the input and computes the graph of Node. Flattens will output a list of Node. Then the resolution step becomes the same as the artifact resolution because each node can be turned into an artifact coordinate, and we then just download the actual artifacts.

In the v4 api, we have ArtifactResolver which gives an Artifact. The DependencyCollector returns the graph with Nodes. The DependencyResolver performs both flatten which gives an ordered list of Nodes, and resolve which returns an ordered Map<Dependency, Path> which I think is what is needed to compute the build paths.

When you collect+resolve, you are interested in all, so even root (again, this is for "ad-hoc" operations, like javadoc may tell: "resolve me this taglet and its dependencies", in that case, javadoc plugin KNOWS root is taglet, but will still need to handle the rest of transitive hull, and place them or use that as needed).

@gnodet if I understand you correctly, sans "root", you consider everything else (the transitive hull) as "dependency"? Which kinda makes sense, as they DID enter the result collection as "dependencies of root"?

Yes, everything is a dependency. It's only the root which can be a project or artifact, but when you collect the dependencies for a project or artifact, you actually resolve all its dependencies.

I think there may be a small piece missing, which is the ability to access the graph from a dependency, i.e. adding a Node getNode() on the org.apache.maven.api.Dependency to get back to the graph.

At some point, I did envision to call it ResolvedDependency which would avoid the confusion with the model dependency, which is is translated into a DependencyCoordinate, the collected / flattened / resolved.

To sum up: the v3 api lacks a bunch of things, that's why plugins have to use the resolver to get fine grain access to the resolution output. The v4 api aims to expose those bits, but those aren't really new, the org.apache.maven.api.Dependency simply wraps org.eclipse.aether.graph.Dependency. That was clearly a missing piece in the v3 api.

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

gnodet · 2024-01-24T21:01:20Z

maven-core/src/main/java/org/apache/maven/internal/impl/DefaultArtifact.java

@@ -95,7 +97,12 @@ public String getClassifier() {

    @Override
    public boolean isSnapshot() {
-        return DefaultModelVersionParser.checkSnapshot(artifact.getVersion());
+        return artifact.isSnapshot();


We had this discussion weeks ago, I think we agreed the snapshot knowledge was not on the artifact. We now have a pluggable VersionScheme, I think it belongs there.

Anyway, I think those changes related to snapshots are unrelated and would be better in a separate PR to ease the discussion.

Agreed.

My point is to have one "source of truth". So if we change this once, we change that one spot. Currently, resolver Artifact.isSnapshot should be it IMHO. But agree, let's move it out from this PR, does not belong here.

cstamas · 2024-01-24T21:10:08Z

There is one maven-core UT failing as DefaultPackagingManager seems not set somewhere.

rmannibucau · 2024-01-24T21:18:40Z

@cstamas I see the 1-1 with what exists but I have some hard time - blame the hour maybe ;) - to see how we can make it consistent with the type work which is pending, typically build path will not be a build path but more a resolved artifact set totally unrelated to any path now, no? So for me scope becomes just a resolution stage more than anything else. Am I missing something?

cstamas · 2024-01-24T21:28:52Z

It was always "build path" but Maven internally called it wrongly "classpath" 😉 (and/or was what was inherited from Maven2)

Basically what we do here is laying down new API blocks for dep scopes and build path scopes.

The other PR is about "filling in the properties".

And ultimately, you as for "main-compile" or "test-compile" build path, Maven calls into Resolver and it resolves those for you, and you end up with a bunch of artifacts (that are coming via project, so are dependency, hence, had type).

Then, those collections are "split", filtered by various properties present in their artifact.properties that makes them "fall" into proper bucket, like classpath, modulepath, etc

gnodet · 2024-01-24T21:16:36Z

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

+import org.apache.maven.api.annotations.Nonnull;
+
+/**
+ * Build path. Build path is calculated for given {@link Project} and {@link BuildPathScope}.


It should be noted what the build path is and what's the purpose. It's not a common java notion, unlike class path and module path.
Especially the "build" term could lead to think it's role is build time, but that's not the case and it can be used at build time or run time.

gnodet · 2024-01-24T21:18:13Z

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

+    }
+
+    @Nonnull
+    Collection<Artifact> getArtifacts();


A Node would be required.
From that one, we can derive a List<Dependency> and eventually a List<Artifact>. But that's related to my remark around artifact properties which I think are still dependency properties.

gnodet · 2024-01-24T21:18:57Z

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

+     * @return the artifact properties, never {@code null}
+     */
+    @Nonnull
+    ArtifactProperties getArtifactProperties();


well, then they should be kept on the Dependency

rmannibucau · 2024-01-25T06:20:44Z

@cstamas does it mean we should fix the naming for mvn 4 and align it on what it is? Resolverpaths or alike since it is unrelated to any actual build and becomes even more ambiguous with jpms?

gnodet · 2024-01-25T06:44:31Z

@cstamas does it mean we should fix the naming for mvn 4 and align it on what it is? Resolverpaths or alike since it is unrelated to any actual build and becomes even more ambiguous with jpms?

I also find the term build path somewhat misleading...

cstamas · 2024-01-25T09:54:37Z

I also find the term build path somewhat misleading...

Am completely fine to keep "resolution path" as well, but did not want to disturb existing classes...

cstamas · 2024-01-25T09:59:21Z

@gnodet and this is why I think that "step 1" should have been merged. This discussion will take some, and this PR will just potentially drift away from master (unless we freeze master, which I don't want).

gnodet · 2024-01-25T10:35:39Z

@gnodet and this is why I think that "step 1" should have been merged. This discussion will take some, and this PR will just potentially drift away from master (unless we freeze master, which I don't want).

Maybe, but there's no point in the first PR if we don't go further...

cstamas · 2024-01-25T10:44:54Z

Maybe, but there's no point in the first PR if we don't go further...

Um, the point of first commit is to move off from all the deprecated classes in resolver (see apache/maven-resolver#412 and related JIRA), as currently, boundary of "scope interpretation and transformation" is completely blurred and doubled between Resolver and Maven Core. Things like adding new scopes is not trivial at all, as it requires changes in both projects, whereis Resolver should be really oblivious about scopes.

The first commit is really not related to 2nd step, it just does the right thing (moves all scope related logic to Maven Core), that in turn also effectively clears grounds for step2.

rmannibucau · 2024-01-25T10:52:24Z

I'm more than fine doing it by "blocks" - in particular the last part Tamas explained - but how can we ensure we don't get this PR without next one in terms of release to avoid to expose - even in an alpha - something we know we'll not keep like that and we'll break?

cstamas · 2024-01-25T13:08:19Z

Just a general remark: Am fine if we "redefine" here everything, from ground up, but IMHO "safest" would be to go "resolver way" (adhere to its abstractions closest as possible) as it is proven and works. And then improve it from there (consider "resolver way" as starting point), instead to try to come up with something completely new and different, and risk the problem of something (some use case, edge case) being forgotten, and then go thru pain of "fixing it" in upcoming API versions...

As Guillaume is right here: I am somewhat pushing for resolver-like way of working (and interfaces/services).

cstamas · 2024-01-25T13:10:32Z

First "block" or "step" is here #1392

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

gnodet · 2024-01-31T22:41:50Z

@cstamas I've removed BuildPath which will be provided by #1378 (inside DependencyResolverResult) and renamed BuildPathScope to PathScope to align with the other PR which defines PathType.
Let me know what you think about the current state.

desruisseaux · 2024-02-03T12:48:35Z

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

+    NONE("none", false),
+
+    /**
+     * Empty scope.


Maybe explain the different between "empty" and "none"?

We saw that "empty" is present in old codebase (3.x), maybe will go away, but we left it here just to be on safe side until we clear up things.

The "none" on the other hand is clearly defined IMHO: merely added just to affect the reactor sorting.

desruisseaux · 2024-02-03T12:49:42Z

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

+    COMPILE_ONLY("compile-only", false),
+
+    /**
+     * Compile.


I presume that we mean "compile, test and runtime"? (for clarifying the difference with COMPILE_ONLY).

I think it would be worth to edit the javadoc accordingly (same for TEST).

desruisseaux · 2024-02-03T12:50:38Z

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

+    TEST_ONLY("test-only", false),
+
+    /**
+     * Test.


I presume that we mean "test compilation and execution"?

desruisseaux · 2024-02-03T12:55:50Z

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

+ * Implementation must have {@code equals()} and {@code hashCode()} implemented, so implementations of this interface
+ * can be used as keys.
+ */
+public interface ExtensibleEnum {


Just for information: the International Organization for Standardization (ISO) uses the CodeList term for extensible enumerations, at least in the ISO 19xxx series of international standards (geospatial information). But I did not verified if CodeList was consistently used in all other standards as well.

cstamas · 2024-02-05T10:11:43Z

Looks good! Sorry to be late to party, but we can improve all this "on the go" anyway...

Jircs1

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

cstamas requested a review from gnodet January 24, 2024 11:34

cstamas self-assigned this Jan 24, 2024

gnodet reviewed Jan 24, 2024

View reviewed changes

...rovider/src/main/java/org/apache/maven/repository/internal/scopes/MavenDependencyScopes.java Outdated Show resolved Hide resolved

gnodet reviewed Jan 24, 2024

View reviewed changes

...-provider/src/main/java/org/apache/maven/repository/internal/MavenRepositorySystemUtils.java Outdated Show resolved Hide resolved

cstamas requested a review from gnodet January 24, 2024 13:22

cstamas force-pushed the MNG-8026 branch from bdc3da8 to 420b1ae Compare January 24, 2024 13:51

gnodet approved these changes Jan 24, 2024

View reviewed changes

gnodet requested changes Jan 24, 2024

View reviewed changes

cstamas added 5 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

gnodet force-pushed the MNG-8026 branch from 610c172 to 22b31ad Compare January 30, 2024 23:43

gnodet added 2 commits January 31, 2024 11:40

Undo ArtifactProperties and Artifact#isSnapshot changes for now

dc4b1e7

Fix UT

3e3aa04

gnodet added 2 commits January 31, 2024 14:48

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

2536929

…tc...

Use the new scopes

cf08727

gnodet force-pushed the MNG-8026 branch from 2ddd29f to cf08727 Compare January 31, 2024 14:19

gnodet added 6 commits January 31, 2024 15:39

Fix UT

ff413ae

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

gnodet mentioned this pull request Jan 31, 2024

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

Closed

gnodet self-requested a review January 31, 2024 22:37

gnodet approved these changes Jan 31, 2024

View reviewed changes

desruisseaux reviewed Feb 3, 2024

View reviewed changes

Add some javadoc

4f5a879

gnodet marked this pull request as ready for review February 5, 2024 08:00

Fix bad links

0ee9939

gnodet merged commit 3f9fec2 into apache:master Feb 5, 2024
18 checks passed

cstamas deleted the MNG-8026 branch February 5, 2024 10:11

Jircs1 reviewed Feb 10, 2024

View reviewed changes

[MNG-8026] Maven drives regarding scopes #1391

[MNG-8026] Maven drives regarding scopes #1391

Conversation

cstamas commented Jan 24, 2024 • edited

cstamas commented Jan 24, 2024 • edited

Choose a reason for hiding this comment

cstamas Jan 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstamas Jan 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstamas Jan 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstamas commented Jan 24, 2024

rmannibucau commented Jan 24, 2024

cstamas commented Jan 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmannibucau commented Jan 25, 2024

gnodet commented Jan 25, 2024

cstamas commented Jan 25, 2024

cstamas commented Jan 25, 2024

gnodet commented Jan 25, 2024

cstamas commented Jan 25, 2024

rmannibucau commented Jan 25, 2024

cstamas commented Jan 25, 2024 • edited

cstamas commented Jan 25, 2024

gnodet commented Jan 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstamas commented Feb 5, 2024

Jircs1 left a comment

Choose a reason for hiding this comment

cstamas commented Jan 24, 2024 •

edited

cstamas commented Jan 24, 2024 •

edited

cstamas Jan 24, 2024 •

edited

cstamas Jan 25, 2024 •

edited

cstamas Jan 25, 2024 •

edited

cstamas commented Jan 24, 2024 •

edited

cstamas commented Jan 25, 2024 •

edited