You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@maven.apache.org by Martin Desruisseaux <ma...@geomatys.com> on 2024/08/05 12:07:16 UTC
Should the Dependency interface contains a getPath() method?
Maven 4 has two very similar public interfaces for dependencies:
* Dependency
* DependencyCoordinate
They both have groupId, artifactId, classifier, version, extension, type
and scope. Differences that I observed are:
* Spelling: `Dependency.isOptional()` versus
`DependencyCoordinate.getOptional()`.
* Return type of the `getVersion()` method: `Version` versus
`VersionConstraint`.
* Methods present only in `DependencyCoordinate`:
o getExclusions()
* Methods present only in `Dependency`:
o getBaseVersion()
o isSnapshot()
o toCoordinate()
The Javadoc does not explain those differences, neither why they exist.
I suspect that `DependencyCoordinate` is the dependency has declared in
the POM, while `Dependency` is the dependency effectively used after
resolution. For example, with the range of versions
(`VersionConstraint`) resolved to a single version (`Version`). Is that
right? If so, would it make sense to add a `getPath()` method in
`Dependency` since it has been resolved? I think it would make the API
easier to understand compared to the current approach of using
`Map<Dependency, Path>` returned by `DependencyResolverResult`.
The reason why I ask is because there is path-derived information that
need to be associated to a dependency, in particular `ModuleDescriptor`.
With the current design, those information are provided by
`DependencyResolverResult`. It would be more natural to have them in
`Dependency`. I.e., instead of:
* DependencyResolverResult
o Map<Dependency, Path> getDependencies()
o Optional<ModuleDescriptor> getModuleModuleDescriptor(Path)
o Optional<String> getModuleName(Path)
We could have:
* DependencyResolverResult
o Set<Dependency> getDependencies()
* Dependency
o Path getPath()
o Optional<ModuleDescriptor> getModuleModuleDescriptor()
o Optional<String> Dependency.getModuleName()
The choice would determine the form of pull request #1625, which is
needed for the new compiler plugin and will also be needed by at least
javadoc, surefire and exec plugins.
As a side note, do we really need those interfaces to be independent?
Couldn't we define Dependency as a subtype of DependencyCoordinate
together with Artifact as a subtype of ArtifactCoordinate? It would
remove the need for `toCoordinate()` methods and reduce a little bit the
amount of wrappings between different kinds of objects in Maven core,
which cost me hours of debugging (pull request #1621 is small, but was
hard to find).
Martin
Re: Should the Dependency interface contains a getPath() method?
Posted by Martin Desruisseaux <ma...@geomatys.com>.
Le 2024-08-07 à 18 h 25, Guillaume Nodet a écrit :
>
> Can it load non JPMS jars too ?
>
Yes, as an "automatic" or "unnamed" module.
Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: Should the Dependency interface contains a getPath() method?
Posted by Guillaume Nodet <gn...@apache.org>.
Can it load non JPMS jars too ?
Maybe this has changed, but my understanding is that everything had to be
JPMS modules, meaning that any plugin that has a dependency which is not a
JPMS module would not be loadable…
That’s a too big constraint imho.
------------------------
Guillaume Nodet
Le mer. 7 août 2024 à 18:16, Martin Desruisseaux <
martin.desruisseaux@geomatys.com> a écrit :
> Le 2024-08-07 à 17 h 38, Guillaume Nodet a écrit :
>
> > Did I miss something ? We need to load plugins at runtime, I thought
> > JPMS was static…
> JPMS can load plugins at runtime as well. However, the way to do that is
> very different. It is no longer with ClassLoader, we have to use
> ModuleLayer instead. It is maybe the biggest change when moving from a
> non-JPMS to a JPMS environment. I did exactly that one year ago in a
> Swing application running tests on a library specified at runtime by the
> "File Chooser" dialog box. The commit at [1] gives an idea of the
> changes when migrating from a "class loader" world to a "module layer"
> world.
>
> What is static is not JPMS, but jlink, which is only one tool among
> others that become available after a project has been modularized.
>
> Martin
>
> [1]
> https://github.com/IOGP-GIGS/GIGSGeoAPI/commit/ff6321dce23500ddd39aee21b6a90f911c2d1727
>
Re: Should the Dependency interface contains a getPath() method?
Posted by Martin Desruisseaux <ma...@geomatys.com>.
Le 2024-08-07 à 17 h 38, Guillaume Nodet a écrit :
> Did I miss something ? We need to load plugins at runtime, I thought
> JPMS was static…
JPMS can load plugins at runtime as well. However, the way to do that is
very different. It is no longer with ClassLoader, we have to use
ModuleLayer instead. It is maybe the biggest change when moving from a
non-JPMS to a JPMS environment. I did exactly that one year ago in a
Swing application running tests on a library specified at runtime by the
"File Chooser" dialog box. The commit at [1] gives an idea of the
changes when migrating from a "class loader" world to a "module layer"
world.
What is static is not JPMS, but jlink, which is only one tool among
others that become available after a project has been modularized.
Martin
[1]https://github.com/IOGP-GIGS/GIGSGeoAPI/commit/ff6321dce23500ddd39aee21b6a90f911c2d1727
Re: Should the Dependency interface contains a getPath() method?
Posted by Guillaume Nodet <gn...@apache.org>.
Le mer. 7 août 2024 à 15:30, Martin Desruisseaux <
martin.desruisseaux@geomatys.com> a écrit :
> Le 2024-08-07 à 14 h 21, Guillaume Nodet a écrit :
> >
> > Why do we care ? If we provide a well defined API (with a known set of
> > jars), people should be aware if they use maven-core jar directly….
> >
> It is a matter of reputation for the Maven project. A Murphy law is that
> given a sufficiently large amount of users, everything accessible will
> be depended upon by someone somewhere, no matter the warnings that we
> provide. Sun Microsystems and Oracle suffered from that Murphy law with
> libraries depending on Java implementation details, and developers
> blaming Sun or Oracle when those details changed. It forced Oracle to
> handle some of Java internal almost as if they were public API (e.g.,
> the Unsafe class). I think that it is one of the motivations for
> blocking accesses to internal packages in JPMS. Another reason is
> probably security.
>
> Maven does not have as many users as Java, so its exposition to above
> Murphy law may be lower, but I have the impression that Maven has a
> better compatibility record than Gradle (i.e., better chances that a
> plugin working with a Maven version continues to work with the next
> version). Reducing the visibility of implementation classes increases
> the chances to keep a plugin ecosystem that way.
>
>
> > Right, but I would think a follow-up PR would allow importing
> > additional packages from the maven realm (or even hide some). Hiding
> > may be needed in some very specific cases (I’ m thinking mainly about
> > hiding slf4j in case the plugin embeds a library which relies on a
> > specific logging provider).
> >
> Side note: those goals with Realm look very similar to JPMS… For
> example, the use case with SLF4J is a non-transitive dependency in JPMS
> ("transitive" here does not have the same meaning as in Maven). I was
> thinking, after completion of all the work for improving JPMS support in
> Maven, to try on a branch to modularize Maven itself just for checking
> if it is possible. If the latter case appears feasible, it is not clear
> to me why using Realm?
Did I miss something ? We need to load plugins at runtime, I thought JPMS
was static…
>
> Martin
>
>
Re: Should the Dependency interface contains a getPath() method?
Posted by Martin Desruisseaux <ma...@geomatys.com>.
Le 2024-08-07 à 14 h 21, Guillaume Nodet a écrit :
>
> Why do we care ? If we provide a well defined API (with a known set of
> jars), people should be aware if they use maven-core jar directly….
>
It is a matter of reputation for the Maven project. A Murphy law is that
given a sufficiently large amount of users, everything accessible will
be depended upon by someone somewhere, no matter the warnings that we
provide. Sun Microsystems and Oracle suffered from that Murphy law with
libraries depending on Java implementation details, and developers
blaming Sun or Oracle when those details changed. It forced Oracle to
handle some of Java internal almost as if they were public API (e.g.,
the Unsafe class). I think that it is one of the motivations for
blocking accesses to internal packages in JPMS. Another reason is
probably security.
Maven does not have as many users as Java, so its exposition to above
Murphy law may be lower, but I have the impression that Maven has a
better compatibility record than Gradle (i.e., better chances that a
plugin working with a Maven version continues to work with the next
version). Reducing the visibility of implementation classes increases
the chances to keep a plugin ecosystem that way.
> Right, but I would think a follow-up PR would allow importing
> additional packages from the maven realm (or even hide some). Hiding
> may be needed in some very specific cases (I’ m thinking mainly about
> hiding slf4j in case the plugin embeds a library which relies on a
> specific logging provider).
>
Side note: those goals with Realm look very similar to JPMS… For
example, the use case with SLF4J is a non-transitive dependency in JPMS
("transitive" here does not have the same meaning as in Maven). I was
thinking, after completion of all the work for improving JPMS support in
Maven, to try on a branch to modularize Maven itself just for checking
if it is possible. If the latter case appears feasible, it is not clear
to me why using Realm?
Martin
Re: Should the Dependency interface contains a getPath() method?
Posted by Guillaume Nodet <gn...@apache.org>.
------------------------
Guillaume Nodet
Le mer. 7 août 2024 à 13:24, Martin Desruisseaux <
martin.desruisseaux@geomatys.com> a écrit :
> Le 2024-08-07 à 12 h 55, Guillaume Nodet a écrit :
>
> > I'm not a big fan of reducing the visibility of implementation
> > classes. The API is well defined, i.e. all org.apache.maven.api.*
> > packages.
> >
> It is because while the API does not expose Eclipse Aether, the
> implementation does. If, in the future, we want to reduce the amount of
> wrapping, we will need to evaluate the consequence of that change, and
> having the implementation classes publicly accessible makes that
> evaluation more difficult.
Why do we care ? If we provide a well defined API (with a known set of
jars), people should be aware if they use maven-core jar directly….
> > Anyway, I don't really see where the problem can be, as for plugins,
> > the default setup should only expose the org.apache.maven.api
> > packages, so they won't even have access to implementation classes. We
> > just need to merge https://github.com/apache/maven/pull/1336
> >
> If PR #1336 has the effect of making the implementation classes
> inaccessible outside Maven core, then removing some "public" keywords
> would not change the situation for plugins and would have the advantage
> of clarity, isn't it?
Right, but I would think a follow-up PR would allow importing additional
packages from the maven realm (or even hide some). Hiding may be needed in
some very specific cases (I’ m thinking mainly about hiding slf4j in case
the plugin embeds a library which relies on a specific logging provider).
Anyway, that was just an opinion, as I’ve needed more than once to
workaround such good designs patterns, and that’s really painful. I don’t
really oppose, I’m just not really seeing the benefits.
>
> Martin
>
>
Re: Should the Dependency interface contains a getPath() method?
Posted by Martin Desruisseaux <ma...@geomatys.com>.
Le 2024-08-07 à 12 h 55, Guillaume Nodet a écrit :
> I'm not a big fan of reducing the visibility of implementation
> classes. The API is well defined, i.e. all org.apache.maven.api.*
> packages.
>
It is because while the API does not expose Eclipse Aether, the
implementation does. If, in the future, we want to reduce the amount of
wrapping, we will need to evaluate the consequence of that change, and
having the implementation classes publicly accessible makes that
evaluation more difficult.
> Anyway, I don't really see where the problem can be, as for plugins,
> the default setup should only expose the org.apache.maven.api
> packages, so they won't even have access to implementation classes. We
> just need to merge https://github.com/apache/maven/pull/1336
>
If PR #1336 has the effect of making the implementation classes
inaccessible outside Maven core, then removing some "public" keywords
would not change the situation for plugins and would have the advantage
of clarity, isn't it?
Martin
Re: Should the Dependency interface contains a getPath() method?
Posted by Guillaume Nodet <gn...@apache.org>.
Le mar. 6 août 2024 à 11:08, Martin Desruisseaux
<ma...@geomatys.com> a écrit :
>
> Le 2024-08-05 à 14 h 19, Tamás Cservenák a écrit :
>
> > You are on the right track and Guillame and I had a lot of discussions
> > about this. IMO, best would be to wait for him (is on PTO I think) and
> > have a chat face to face?
> >
> Yes I'm fine with a talk. I'm available most time of the day (except
> today). Maybe it should be before merging pull request #1625 because
> that PR may change depending on the outcome.
>
> I have another wish that we may discuss: in order to reduce debugging
> time, I suggest to reduce the amount of convenience methods in the API.
> Especially the ones that are shortcuts doing nothing more than invoking
> a method of the same name with the same parameters plus some more
> parameters set to null. Those shortcuts only save the typing of a few
> "null" keywords, but come with a cost: when reading the code, it is not
> obvious that there are hidden null parameter values and that maybe we
> should paid attention to those parameters. This is what happened when I
> tried to understand why the dependency type ("test-jar", "modular-jar",
> etc.) was lost somewhere. When reading the code, it seems that all
> parameters are supplied. It took me step-by-step debugging to realize
> that they were an implicit null parameter value hidden behind a
> convenience method.
No real problem from my side.
> Another topic I would like to discuss is reducing the visibility of
> implementation classes (i.e. turning public classes to package-private
> ones). I started an experiment at [1], just to verify that Maven can
> still run. The goal is to have more freedom for refactoring the
> implementation later, with less fear of breaking some plugins outside
> Maven core. I'm thinking in particular in reducing the amount of
> wrappings or copyings, for reducing the risk of codes that forget to
> copy an information.
I'm not a big fan of reducing the visibility of implementation
classes. The API is well defined, i.e. all org.apache.maven.api.*
packages.
If people go outside, their code may be broken. But from a usability
POV, if they do need to access it (maybe as a workaround for a missing
feature in the API or non foreseen use case), having to deal with
private methods or classes is just a nightmare. As an API user, I
prefer my code to be broken from time to time, rather than having to
copy a bunch of classes and having to deal with merging changes.
Anyway, I don't really see where the problem can be, as for plugins,
the default setup should only expose the org.apache.maven.api
packages, so they won't even have access to implementation classes.
We just need to merge https://github.com/apache/maven/pull/1336
> Martin
>
> [1]https://github.com/Geomatys/maven/commit/8964892f8fd07a9fb2d4ffce833718d45f9aaf58
--
------------------------
Guillaume Nodet
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: Should the Dependency interface contains a getPath() method?
Posted by Martin Desruisseaux <ma...@geomatys.com>.
Le 2024-08-05 à 14 h 19, Tamás Cservenák a écrit :
> You are on the right track and Guillame and I had a lot of discussions
> about this. IMO, best would be to wait for him (is on PTO I think) and
> have a chat face to face?
>
Yes I'm fine with a talk. I'm available most time of the day (except
today). Maybe it should be before merging pull request #1625 because
that PR may change depending on the outcome.
I have another wish that we may discuss: in order to reduce debugging
time, I suggest to reduce the amount of convenience methods in the API.
Especially the ones that are shortcuts doing nothing more than invoking
a method of the same name with the same parameters plus some more
parameters set to null. Those shortcuts only save the typing of a few
"null" keywords, but come with a cost: when reading the code, it is not
obvious that there are hidden null parameter values and that maybe we
should paid attention to those parameters. This is what happened when I
tried to understand why the dependency type ("test-jar", "modular-jar",
etc.) was lost somewhere. When reading the code, it seems that all
parameters are supplied. It took me step-by-step debugging to realize
that they were an implicit null parameter value hidden behind a
convenience method.
Another topic I would like to discuss is reducing the visibility of
implementation classes (i.e. turning public classes to package-private
ones). I started an experiment at [1], just to verify that Maven can
still run. The goal is to have more freedom for refactoring the
implementation later, with less fear of breaking some plugins outside
Maven core. I'm thinking in particular in reducing the amount of
wrappings or copyings, for reducing the risk of codes that forget to
copy an information.
Martin
[1]https://github.com/Geomatys/maven/commit/8964892f8fd07a9fb2d4ffce833718d45f9aaf58
Re: Should the Dependency interface contains a getPath() method?
Posted by Tamás Cservenák <ta...@cservenak.net>.
Howdy,
You are on the right track and Guillame and I had a lot of discussions
about this.
IMO, best would be to wait for him (is on PTO I think) and have a chat face
to face?
Thanks
T
On Mon, Aug 5, 2024 at 2:07 PM Martin Desruisseaux <
martin.desruisseaux@geomatys.com> wrote:
> Maven 4 has two very similar public interfaces for dependencies:
>
> * Dependency
> * DependencyCoordinate
>
> They both have groupId, artifactId, classifier, version, extension, type
> and scope. Differences that I observed are:
>
> * Spelling: `Dependency.isOptional()` versus
> `DependencyCoordinate.getOptional()`.
> * Return type of the `getVersion()` method: `Version` versus
> `VersionConstraint`.
> * Methods present only in `DependencyCoordinate`:
> o getExclusions()
> * Methods present only in `Dependency`:
> o getBaseVersion()
> o isSnapshot()
> o toCoordinate()
>
> The Javadoc does not explain those differences, neither why they exist.
> I suspect that `DependencyCoordinate` is the dependency has declared in
> the POM, while `Dependency` is the dependency effectively used after
> resolution. For example, with the range of versions
> (`VersionConstraint`) resolved to a single version (`Version`). Is that
> right? If so, would it make sense to add a `getPath()` method in
> `Dependency` since it has been resolved? I think it would make the API
> easier to understand compared to the current approach of using
> `Map<Dependency, Path>` returned by `DependencyResolverResult`.
>
> The reason why I ask is because there is path-derived information that
> need to be associated to a dependency, in particular `ModuleDescriptor`.
> With the current design, those information are provided by
> `DependencyResolverResult`. It would be more natural to have them in
> `Dependency`. I.e., instead of:
>
> * DependencyResolverResult
> o Map<Dependency, Path> getDependencies()
> o Optional<ModuleDescriptor> getModuleModuleDescriptor(Path)
> o Optional<String> getModuleName(Path)
>
> We could have:
>
> * DependencyResolverResult
> o Set<Dependency> getDependencies()
> * Dependency
> o Path getPath()
> o Optional<ModuleDescriptor> getModuleModuleDescriptor()
> o Optional<String> Dependency.getModuleName()
>
> The choice would determine the form of pull request #1625, which is
> needed for the new compiler plugin and will also be needed by at least
> javadoc, surefire and exec plugins.
>
> As a side note, do we really need those interfaces to be independent?
> Couldn't we define Dependency as a subtype of DependencyCoordinate
> together with Artifact as a subtype of ArtifactCoordinate? It would
> remove the need for `toCoordinate()` methods and reduce a little bit the
> amount of wrappings between different kinds of objects in Maven core,
> which cost me hours of debugging (pull request #1621 is small, but was
> hard to find).
>
> Martin
>
>
Re: Should the Dependency interface contains a getPath() method?
Posted by Guillaume Nodet <gn...@apache.org>.
I’ll reply to that one later…
------------------------
Guillaume Nodet
Le lun. 5 août 2024 à 14:07, Martin Desruisseaux <
martin.desruisseaux@geomatys.com> a écrit :
> Maven 4 has two very similar public interfaces for dependencies:
>
> * Dependency
> * DependencyCoordinate
>
> They both have groupId, artifactId, classifier, version, extension, type
> and scope. Differences that I observed are:
>
> * Spelling: `Dependency.isOptional()` versus
> `DependencyCoordinate.getOptional()`.
> * Return type of the `getVersion()` method: `Version` versus
> `VersionConstraint`.
> * Methods present only in `DependencyCoordinate`:
> o getExclusions()
> * Methods present only in `Dependency`:
> o getBaseVersion()
> o isSnapshot()
> o toCoordinate()
>
> The Javadoc does not explain those differences, neither why they exist.
> I suspect that `DependencyCoordinate` is the dependency has declared in
> the POM, while `Dependency` is the dependency effectively used after
> resolution. For example, with the range of versions
> (`VersionConstraint`) resolved to a single version (`Version`). Is that
> right? If so, would it make sense to add a `getPath()` method in
> `Dependency` since it has been resolved? I think it would make the API
> easier to understand compared to the current approach of using
> `Map<Dependency, Path>` returned by `DependencyResolverResult`.
>
> The reason why I ask is because there is path-derived information that
> need to be associated to a dependency, in particular `ModuleDescriptor`.
> With the current design, those information are provided by
> `DependencyResolverResult`. It would be more natural to have them in
> `Dependency`. I.e., instead of:
>
> * DependencyResolverResult
> o Map<Dependency, Path> getDependencies()
> o Optional<ModuleDescriptor> getModuleModuleDescriptor(Path)
> o Optional<String> getModuleName(Path)
>
> We could have:
>
> * DependencyResolverResult
> o Set<Dependency> getDependencies()
> * Dependency
> o Path getPath()
> o Optional<ModuleDescriptor> getModuleModuleDescriptor()
> o Optional<String> Dependency.getModuleName()
>
> The choice would determine the form of pull request #1625, which is
> needed for the new compiler plugin and will also be needed by at least
> javadoc, surefire and exec plugins.
>
> As a side note, do we really need those interfaces to be independent?
> Couldn't we define Dependency as a subtype of DependencyCoordinate
> together with Artifact as a subtype of ArtifactCoordinate? It would
> remove the need for `toCoordinate()` methods and reduce a little bit the
> amount of wrappings between different kinds of objects in Maven core,
> which cost me hours of debugging (pull request #1621 is small, but was
> hard to find).
>
> Martin
>
>
Re: Should the Dependency interface contains a getPath() method?
Posted by Guillaume Nodet <gn...@apache.org>.
Le jeu. 8 août 2024 à 11:05, Martin Desruisseaux <
martin.desruisseaux@geomatys.com> a écrit :
> Hello Guillaume
>
> Le 2024-08-07 à 22 h 23, Guillaume Nodet a écrit :
>
> > * ArtifactCoordinate are used to locate artifacts in a repository,
> > it's basically a pointer to a file in maven repository
> > * Artifact is the pointed artifact in the repository. They are
> > created when *resolving* an ArtifactCoordinate which basically
> > download the artifact in the local repository
> > * DependencyCoordinate is used to express a dependency
> > * Node is the main output of the *dependency collection* process,
> > it's the graph of dependencies
> > * Dependency is the output of the *collection* process, part of the
> > graph computed from one or more DependencyCoordinate, it's an
> > artifact + type + scope
> >
> It would be great to have this summary in the package-info Javadoc, and
> also the relevant parts in each interface Javadoc. I will try to do a PR
> next weekend for that.
>
>
> > when building the collected graph, the artifacts are not downloaded,
> > only the POMs are needed to build the graph. When will those
> > getModuleDescriptor() and getModuleName() methods be made available ?
> > I'd like to keep the objects immutable though.
> >
> I agree about immutability, but "immutable" does not necessarily mean
> "no deferred computation". The JDK has some examples of immutable
> objects that compute and cache their values when first requested (e.g.,
> `Path` itself, or `BigDecimal`, `Class`, etc.). Users still have the
> benefit of immutable objects, in the sense that they can share the same
> instance without the need for defensive copies and without the need to
> do synchronization themselves. So maybe a
> `Dependency.getModuleDescription()` method could load the module
> description when first needed, and `Dependency` still be considered
> immutable as long as no visible state changed. That method would
> potentially throw IOException, but again there is examples in the JDK of
> immutable objects throwing exceptions (e.g. File.getCanonicalPath()).
>
>
> > We could have a `ResolvedDependency` which would inherit from
> > `Dependency` and provide a `getPath()` method. Same for `Artifact`,
> > where we could define a `ResolvedArtifact`. Or maybe add a `Resolved`
> > interface with the`getPath` method, which could be implemented by both
> > ResolvedArtifact and ResolvedDependency.
> >
> That would work. But isn't `ResolvedDependency` the same as `Dependency`
> if we accept deferred computation, including for downloading the JAR file?
That would completely hide the download and means the object is not really
a data object anymore. We’d have to keep pointers to the session, remote
repositories.
Deferring the module descriptor loading sounds reasonable, but I would not
defer the download which may fail for various reasons.
Also, we want downloads to happen in parallel, so deferring them would
break that.
>
>
>
> About `Dependency` as a specialization of `DependencyCoordinate`:
>
> > I find the outcome much less clear. The same kind of objects are used
> > for multiple things, and it's very confusing.
> >
> Alternatively we can keep the interfaces independent but nevertheless
> resolve the method collisions as below:
>
> * In DependencyCoordinate, rename getVersion() as getVersionConstraint().
Yes, that one would be better.
>
> * Choose a common name and return type for `isOptional()`.
That one is not doable imho. DepedencyCoordinate and Dependency are not at
the same level. One is a request and the other the result. That’s why one
is a boolean primitive and the other a Boolean object which can carry 3
states. That’s the exact same problem with version and version constraint.
They are. Inflated in the resolver because they are stored as strings, but
this comes with confusion.
>
> It would allow an implementation class, if desired, to implement the two
> interfaces even if from user's eyes they are still two separated types.
> This strategy allows to implement the `toCoordinate()` method simply as
> `return this`, thus avoiding a copy operation and the possible
> information lost that come with it. Implementations are not forced to do
> that, it just become possible when convenient.
That’s still doable without the booleans.
>
> Martin
>
>
Re: Should the Dependency interface contains a getPath() method?
Posted by Martin Desruisseaux <ma...@geomatys.com>.
Le 2024-08-08 à 11 h 04, Martin Desruisseaux a écrit :
> It would be great to have this summary in the package-info Javadoc,
> and also the relevant parts in each interface Javadoc. I will try to
> do a PR next weekend for that.
>
Done as pull request #1640. Relevant commit is:
https://github.com/apache/maven/pull/1640/commits/74cf8257c4f16e7a4dba3e46d3c5898e45fee931
A side-effect of renaming ArtifactCoordinate.getVersion() as
getVersionConstraint() is to make more apparent a dubious call to an
org.eclipse.aether.artifact.DefaultArtifact constructor in
AbstractSession (I didn't investigated if it was correct or not).
Martin
Re: Should the Dependency interface contains a getPath() method?
Posted by Martin Desruisseaux <ma...@geomatys.com>.
Hello Guillaume
Le 2024-08-07 à 22 h 23, Guillaume Nodet a écrit :
> * ArtifactCoordinate are used to locate artifacts in a repository,
> it's basically a pointer to a file in maven repository
> * Artifact is the pointed artifact in the repository. They are
> created when *resolving* an ArtifactCoordinate which basically
> download the artifact in the local repository
> * DependencyCoordinate is used to express a dependency
> * Node is the main output of the *dependency collection* process,
> it's the graph of dependencies
> * Dependency is the output of the *collection* process, part of the
> graph computed from one or more DependencyCoordinate, it's an
> artifact + type + scope
>
It would be great to have this summary in the package-info Javadoc, and
also the relevant parts in each interface Javadoc. I will try to do a PR
next weekend for that.
> when building the collected graph, the artifacts are not downloaded,
> only the POMs are needed to build the graph. When will those
> getModuleDescriptor() and getModuleName() methods be made available ?
> I'd like to keep the objects immutable though.
>
I agree about immutability, but "immutable" does not necessarily mean
"no deferred computation". The JDK has some examples of immutable
objects that compute and cache their values when first requested (e.g.,
`Path` itself, or `BigDecimal`, `Class`, etc.). Users still have the
benefit of immutable objects, in the sense that they can share the same
instance without the need for defensive copies and without the need to
do synchronization themselves. So maybe a
`Dependency.getModuleDescription()` method could load the module
description when first needed, and `Dependency` still be considered
immutable as long as no visible state changed. That method would
potentially throw IOException, but again there is examples in the JDK of
immutable objects throwing exceptions (e.g. File.getCanonicalPath()).
> We could have a `ResolvedDependency` which would inherit from
> `Dependency` and provide a `getPath()` method. Same for `Artifact`,
> where we could define a `ResolvedArtifact`. Or maybe add a `Resolved`
> interface with the`getPath` method, which could be implemented by both
> ResolvedArtifact and ResolvedDependency.
>
That would work. But isn't `ResolvedDependency` the same as `Dependency`
if we accept deferred computation, including for downloading the JAR file?
About `Dependency` as a specialization of `DependencyCoordinate`:
> I find the outcome much less clear. The same kind of objects are used
> for multiple things, and it's very confusing.
>
Alternatively we can keep the interfaces independent but nevertheless
resolve the method collisions as below:
* In DependencyCoordinate, rename getVersion() as getVersionConstraint().
* Choose a common name and return type for `isOptional()`.
It would allow an implementation class, if desired, to implement the two
interfaces even if from user's eyes they are still two separated types.
This strategy allows to implement the `toCoordinate()` method simply as
`return this`, thus avoiding a copy operation and the possible
information lost that come with it. Implementations are not forced to do
that, it just become possible when convenient.
Martin
Re: Should the Dependency interface contains a getPath() method?
Posted by Guillaume Nodet <gn...@apache.org>.
Le lun. 5 août 2024 à 14:07, Martin Desruisseaux
<ma...@geomatys.com> a écrit :
>
> Maven 4 has two very similar public interfaces for dependencies:
>
> * Dependency
> * DependencyCoordinate
>
> They both have groupId, artifactId, classifier, version, extension, type
> and scope. Differences that I observed are:
>
> * Spelling: `Dependency.isOptional()` versus
> `DependencyCoordinate.getOptional()`.
Right, though `Dependency.isOptional()` is of type `boolean`, while
`DependencyCoordinate.getOptional()` is of type `Boolean` (it is
nullable).
> * Return type of the `getVersion()` method: `Version` versus
> `VersionConstraint`.
Yes, that's expected.
> * Methods present only in `DependencyCoordinate`:
> o getExclusions()
> * Methods present only in `Dependency`:
> o getBaseVersion()
> o isSnapshot()
> o toCoordinate()
>
> The Javadoc does not explain those differences, neither why they exist.
> I suspect that `DependencyCoordinate` is the dependency has declared in
> the POM, while `Dependency` is the dependency effectively used after
> resolution. For example, with the range of versions
> (`VersionConstraint`) resolved to a single version (`Version`). Is that
> right? If so, would it make sense to add a `getPath()` method in
> `Dependency` since it has been resolved? I think it would make the API
> easier to understand compared to the current approach of using
> `Map<Dependency, Path>` returned by `DependencyResolverResult`.
We have ArtifactCoordinate, Artifact, DependencyCoordinate and
Dependency in the main API. We also have the
org.apache.maven.api.model.Dependency which is the object deserialised
from the POM. That one is used to create DependencyCoordinate so that
they can be resolved.
Back to the 4 objects:
- ArtifactCoordinate are used to locate artifacts in a repository,
it's basically a pointer to a file in maven repository
- Artifact is the pointed artifact in the repository. They are
created when *resolving* an ArtifactCoordinate which basically
download the artifact in the local repository
- DependencyCoordinate is used to express a dependency
- Node is the main output of the *dependency collection* process,
it's the graph of dependencies
- Dependency is the output of the *collection* process, part of the
graph computed from one or more DependencyCoordinate, it's an artifact
+ type + scope
The main reason was to remove some ambiguities between the various
states that we have in the resolver API for Artifact and Dependency.
They may or may not have a resolved version (i.e. be an actual
pointer), may or may not be present in the local repository. You
can't really use a non resolved Artifact for a resolved one, but the
resolver api does not provide any difference between those.
So, ArtifactCoordinate and DependencyCoordinate have a VersionRange,
while Artifact and Dependency have their version resolved.
> The reason why I ask is because there is path-derived information that
> need to be associated to a dependency, in particular `ModuleDescriptor`.
> With the current design, those information are provided by
> `DependencyResolverResult`. It would be more natural to have them in
> `Dependency`. I.e., instead of:
>
> * DependencyResolverResult
> o Map<Dependency, Path> getDependencies()
> o Optional<ModuleDescriptor> getModuleModuleDescriptor(Path)
> o Optional<String> getModuleName(Path)
>
> We could have:
>
> * DependencyResolverResult
> o Set<Dependency> getDependencies()
> * Dependency
> o Path getPath()
> o Optional<ModuleDescriptor> getModuleModuleDescriptor()
> o Optional<String> Dependency.getModuleName()
>
> The choice would determine the form of pull request #1625, which is
> needed for the new compiler plugin and will also be needed by at least
> javadoc, surefire and exec plugins.
The Path for a given Artifact can be retrieved using the
`ArtifactManager`, that was done to be similar to the
`ProjectManager`.
So the main problem is that one of the core design decision of the API
was to have immutable objects. We could have a `ResolvedDependency`
which would inherit from `Dependency` and provide a `getPath()`
method. Same for `Artifact`, where we could define a
`ResolvedArtifact`. Or maybe add a `Resolved` interface with the
`getPath` method, which could be implemented by both ResolvedArtifact
and ResolvedDependency. The `collect`
The collected graph contains Node and Dependency, while the resolved
dependencies are... resolved. Which means that when building the
collected graph, the artifacts are not downloaded, only the POMs are
needed to build the graph. When will those getModuleDescriptor() and
getModuleName() methods be made available ?
I'd like to keep the objects immutable though.
> As a side note, do we really need those interfaces to be independent?
> Couldn't we define Dependency as a subtype of DependencyCoordinate
> together with Artifact as a subtype of ArtifactCoordinate? It would
> remove the need for `toCoordinate()` methods and reduce a little bit the
> amount of wrappings between different kinds of objects in Maven core,
> which cost me hours of debugging (pull request #1621 is small, but was
> hard to find).
I find the outcome much less clear. The same kind of objects are used
for multiple things, and it's very confusing.
>
> Martin
>
--
------------------------
Guillaume Nodet
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: Should the Dependency interface contains a getPath() method?
Posted by Martin Desruisseaux <ma...@geomatys.com>.
Le 2024-08-05 à 14 h 07, Martin Desruisseaux a écrit :
>
> * Optional<ModuleDescriptor> getModuleModuleDescriptor()
>
Typo: should be getModuleDescription(). It was a copy-and-paste error.
Martin