[MRESOLVER-494] The LOCAL_PATH system property is client app property #428

cstamas · 2024-02-10T21:50:16Z

This ArtifactProperty is specific to "system" scope, tied to it. Introduce a "system scope handler" that does all, and let client app control all. Also, this component belongs to session, so expose it via session.

https://issues.apache.org/jira/browse/MRESOLVER-494

This ArtifactProperty is specific to "system" scope, tied to it. Introduce a "system scope handler" that does all, and let client app control all. Also, this component belongs to session, so expose it via session. --- https://issues.apache.org/jira/browse/MRESOLVER-494

cstamas · 2024-02-10T22:02:04Z

...impl/src/main/java/org/eclipse/aether/internal/impl/collect/DependencyCollectorDelegate.java

-    protected static boolean isLackingDescriptor(Artifact artifact) {
-        return artifact.getProperty(ArtifactProperties.LOCAL_PATH, null) != null;
+    protected static boolean isLackingDescriptor(RepositorySystemSession session, Artifact artifact) {
+        return session.getSystemScopeHandler().getSystemPath(artifact.getProperties()) != null;


This makes me think that "system" scope and an "artifact with LOCAL_PATH having set" are one and same (are same thing, synonyms so to say). We may want to handle that as one, instead to have them separated?
Still, especially in depMgt, there are cases where path is unset, but scope remains... so maybe this "handler" is still ok?

I'm not really sure to understand what you're suggesting here...

If you look at "system" scope definition, there is this sentence "Dependency in this scope does not have artifact descriptor either.". And this here is the check "does it have descriptor". And since Resolver 1.0 the actual check is done like this: artifact.getProperty( ArtifactProperties.LOCAL_PATH, null ) != null (presence of property and not actual scope).

slawekjaranowski · 2024-02-12T13:48:05Z

...lver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java

@@ -43,7 +42,7 @@ public abstract class AbstractDependencyManager implements DependencyManager {
     * @deprecated To be removed when deprecated constructors are removed.
     */
    @Deprecated
-    protected static final SystemScopePredicate SYSTEM_PREDICATE = "system"::equals;
+    protected static final SystemScopeHandler SYSTEM_PREDICATE = SystemScopeHandler.LEGACY;


SYSTEM_PREDICATE -> SYSTEM_SCOPE_HANDLER

gnodet · 2024-02-12T14:59:39Z

maven-resolver-api/src/main/java/org/eclipse/aether/SystemScopeHandler.java

+     *
+     * @return the system path from passed in properties, or {@code null} if not present.
+     */
+    String getSystemPath(Map<String, String> properties);


I wonder if we should abstract slightly more and have:

String getSystemPath(Artifact artifact);

gnodet · 2024-02-12T15:00:05Z

maven-resolver-api/src/main/java/org/eclipse/aether/SystemScopeHandler.java

+     * Sets system path in properties. The passed in {@code systemPath} can be {@code null}, in which case this is
+     * "remove" operation (or "unset").
+     */
+    void setSystemPath(Map<String, String> properties, String systemPath);


This method does not make much sense to me. It should not be needed, as the code calling the resolver is now responsible for the system scope. The resolver should not alter what has been given.

Problem is how Artifact is immutable, so it does not have setProperty method. Also, due this, Artifact#setProperties returns new Artifact instance (as almost every mutator method in Resolver).

I don't think we need this method. We can just delay until the very end of the resolution, then call SystemScopeManager#getSystemPath(Artifact) and put it as the resolved path of the artifact.
We don't need to carry the path for the duration of the resolution imho.

This method is used in two cases:

when dependency scope is managed, and is not "system", unset the LOCAL_PATH if set

when dependency scope is managed, and is "system", make sure LOCAL_PATH is set

Given this may happen on any (n-th level of depth), am again unsure from where would you get (and which) artifact to call with getSystemPath()? As now, especially with introduction of "transitive dep mgr", we should track introduced depMgr sections during construction of whole graph (unlike it was in Maven3, only 2 depths).

gnodet · 2024-02-12T15:07:07Z

maven-resolver-api/src/main/java/org/eclipse/aether/SystemScopeHandler.java

+    /**
+     * Returns {@code true} if given scope label is "system" scope.
+     */
+    boolean isSystemScope(String scope);


Is scope supposed to be nullable ? It seems the code is given null. If that's really expected we should indicate that on the javadoc.

I think javadoc is quite clear on this matter. If you consider null "system" scope, then yet, return true here.

gnodet · 2024-02-12T15:08:09Z

maven-resolver-api/src/main/java/org/eclipse/aether/SystemScopeHandler.java

+     * Returns {@code true} if given dependency is in "system" scope.
+     */
+    default boolean isSystemScope(Dependency dependency) {
+        return isSystemScope(dependency.getScope());


Is dependency nullable or not ?
If yes, we should test for a null dependency and have:

return dependency != null && isSystemScope(dependency.getScope());

gnodet · 2024-02-12T15:08:25Z

maven-resolver-api/src/main/java/org/eclipse/aether/SystemScopeHandler.java

+     * Returns {@code true} if given dependency node dependency is in "system" scope.
+     */
+    default boolean isSystemScope(DependencyNode dependencyNode) {
+        return dependencyNode.getDependency() != null && isSystemScope(dependencyNode.getDependency());


dependencyNode may be null here too

gnodet · 2024-02-12T15:09:30Z

...lver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java

                    Map<String, String> properties =
                            new HashMap<>(dependency.getArtifact().getProperties());
-                    properties.remove(ArtifactProperties.LOCAL_PATH);
+                    systemScopeHandler.setSystemPath(properties, null);


I fail to see how this code is relevant.
If this is really needed ? that would mean that the scope isn't solely determined by the dependency.getScope().

Obviously is not, that was my concern here.

Not needed or not only determined by the scope ?

gnodet · 2024-02-12T15:11:56Z

...lver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java

-            if ((systemScopePredicate.test(scope))
-                    || (scope == null && systemScopePredicate.test(dependency.getScope()))) {
+            if ((systemScopeHandler.isSystemScope(scope))
+                    || (scope == null && systemScopeHandler.isSystemScope(dependency.getScope()))) {
                String localPath = managedLocalPaths.get(key);


The managedLocalPaths should be integrated into the systemScopeHandler imho.

systemScopeHandler is session scoped, while managedLocalPaths is per mgr, and mgr is created as needed (see "deriveChildManager") per collection invocation, so unsure how could we do that.

I don't really see a use case in the code where scope == "system" and localPath != null are not interchangeable. So I think we don't need to manage the local paths, they'll just be retrieved from the SystemScopeHandler when needed using getSystemPath(Artifact).

gnodet · 2024-02-13T09:36:27Z

@cstamas Here's what I have in mind: gnodet@791c07e
The local path is only managed by the scope handler. The assumption is that for a given artifact, if it has a system scope, it also has a local path. Maybe I'm missing something though ...

cstamas · 2024-02-13T09:41:05Z

@cstamas Here's what I have in mind: gnodet@791c07e The local path is only managed by the scope handler. The assumption is that for a given artifact, if it has a system scope, it also has a local path. Maybe I'm missing something though ...

You do: i.e. you have a dep w/o scope, but you define a depMgmt w/ system scope (and system path), who will apply then that mgmt onto dependency? Also, this may come from n-th level now, as we introduced "transitive depmgr" etc.

Your change merely removed management of "system" scope from depManager... that is wrong, and will probably explode in Maven ITs (but we should see).

cstamas self-assigned this Feb 10, 2024

cstamas requested review from gnodet and slawekjaranowski February 10, 2024 21:50

Javadoc

a0a5076

cstamas commented Feb 10, 2024

View reviewed changes

slawekjaranowski reviewed Feb 12, 2024

View reviewed changes

Proper rename

b8c9c2c

gnodet requested changes Feb 12, 2024

View reviewed changes

cstamas added 3 commits February 13, 2024 10:02

Make nullchecks

a049aed

Reformat

e36ad23

Pass in artifact

cc8d47e

Javadoc

9730f62

gnodet approved these changes Feb 13, 2024

View reviewed changes

slawekjaranowski approved these changes Feb 13, 2024

View reviewed changes

cstamas merged commit 7280c24 into apache:master Feb 13, 2024
4 checks passed

cstamas deleted the MRESOLVER-494 branch February 13, 2024 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRESOLVER-494] The LOCAL_PATH system property is client app property #428

[MRESOLVER-494] The LOCAL_PATH system property is client app property #428

cstamas commented Feb 10, 2024

cstamas Feb 10, 2024 •

edited

Loading

gnodet Feb 11, 2024

cstamas Feb 12, 2024 •

edited

Loading

slawekjaranowski Feb 12, 2024

cstamas Feb 12, 2024

gnodet Feb 12, 2024

gnodet Feb 12, 2024

cstamas Feb 13, 2024

gnodet Feb 13, 2024 •

edited

Loading

cstamas Feb 13, 2024

gnodet Feb 12, 2024

cstamas Feb 13, 2024

gnodet Feb 12, 2024

gnodet Feb 12, 2024

gnodet Feb 12, 2024

cstamas Feb 13, 2024

gnodet Feb 13, 2024

gnodet Feb 12, 2024

cstamas Feb 13, 2024 •

edited

Loading

gnodet Feb 13, 2024

gnodet commented Feb 13, 2024 •

edited

Loading

cstamas commented Feb 13, 2024 •

edited

Loading

[MRESOLVER-494] The LOCAL_PATH system property is client app property #428

[MRESOLVER-494] The LOCAL_PATH system property is client app property #428

Conversation

cstamas commented Feb 10, 2024

cstamas Feb 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstamas Feb 12, 2024 • edited Loading

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

gnodet Feb 13, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstamas Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnodet commented Feb 13, 2024 • edited Loading

cstamas commented Feb 13, 2024 • edited Loading

cstamas Feb 10, 2024 •

edited

Loading

cstamas Feb 12, 2024 •

edited

Loading

gnodet Feb 13, 2024 •

edited

Loading

cstamas Feb 13, 2024 •

edited

Loading

gnodet commented Feb 13, 2024 •

edited

Loading

cstamas commented Feb 13, 2024 •

edited

Loading