Fix discovery of components without metadata in repositories #10996

jjohannes · 2019-10-09T21:53:15Z

For components without metadata (metadataSource = artifact), we need to HEAD an artifact to check if a component exists in a repository. This is done in the metadata download phase in resolution (instead of actually downloading a metadata file).

For this to work consistently, information about dependency artifacts needs to be preserved when we construct the so called ComponentOverrideMetadata. However, there can be multiple dependencies (or dependency constraints) declarations with different dependency artifact information. We basically always took the first definition we found. However, this does not work, if the first definition does not define an artifact at all (which is always true for dependency constraints). So it never worked for all cases. Recent changes in selector sorting (8eb2d60 of #9307 and probably others) have lead to changes in the ordering and now some cases that worked before do not work anymore (#10948).

This PR fixes the issue in general by tracking the first found artifact across all sectors for a module instead of just tracking the first selector.

Note: these changes are only necessary to fix the discovery of components in repositories. The actual retrieving of artifact is done later after resolution. This works (and worked before): All artifacts are retrieved or, if one is missing, an error is thrown.

In addition to the changes required for artifact handling, this PR also improves the handling of 'client modules' and 'changing' status (the other details of ComponentOverrideMetadata).

This is just a shortcut to a final filed in 'dependencyState'. Sometimes it was used and sometimes 'dependencyState.getDepenedency()' was used which is always the same value. This makes this complex code a bit easier to comprehend.

This information needs to be preserved to make sure that an artifact that acts as metadata for itself (metadataSources = artifact()) is found during the early resolution phase where we search for modules in repositories.

Later in the resolution, we already combine all artifacts defined as 'dependency artifacts' on incoming edges. If a component does not have metadata, we need at least information about one artifact early when we look for an artifact (instead of a metadata file).

Such artifacts can actually be addressed by 'dependency artifacts' through the 'artifact { }' notation inside a dependency declaration; or in Gradle Module Metadata.

The order can make a difference in repository selection, when checking if an artifact-only-component containing the artifacts exists in a repository. In absence of a metadata file, we initially test for the existence of one artifact to decide if a repository contains the corresponding artifact-only-component. This is always the first artifact in the set (which is internally converted into a list). This change makes sure that the first artifact is always the same for a build that does not change.

This test reproduces #10948 and other variations of it.

As stated in the here implemented 'ServerWithExpectations' fixture: "handlers, as well as failures, need to be thread-safe" This concrete case was working most of the time since usually there are not more than one expectations for the same request. But if the same request is expected several times, and the requests are received in parallel (as it is the case for metadata download), the handle() method behaved flaky - by not doing reading and updating of the 'run' flag as an atomic operation.

This seems to be dependent on timing. If one request starts after another did already finish, it can take the result from cache.

jjohannes · 2019-10-10T08:42:15Z

...org/gradle/api/internal/artifacts/ivyservice/resolveengine/graph/builder/ComponentState.java

-    public void selectedBy(ResolvableSelectorState selectorState) {
-        if (firstSelectedBy == null) {
-            firstSelectedBy = (SelectorState) selectorState;
-        }


The behavior of this changed with the selector sorting. So a gaol of this PR is to get rid of this additional (weird) state and instead rely on the order of the selector list.

Although these are edge cases, it leads to more consistency and makes the behavior less dependent on order which may change unexpectedly through internal optimisations.

We now rely on the selector sorting when choosing a selector to compute override metadata. The sorting puts the most likely used selector first.

ljacomet

Good stuff in there! 👍

Two minor remarks

ljacomet · 2019-10-10T09:21:34Z

...ment/src/main/java/org/gradle/internal/component/model/LocalComponentDependencyMetadata.java

@@ -96,7 +96,7 @@ public LocalComponentDependencyMetadata(ComponentIdentifier componentId,
    }

    private static List<IvyArtifactName> asImmutable(List<IvyArtifactName> artifactNames) {
-        return artifactNames.isEmpty() ? Collections.emptyList() : ImmutableList.copyOf(artifactNames);
+        return artifactNames.isEmpty() ? Collections.emptyList() : artifactNames instanceof ImmutableList ? artifactNames : ImmutableList.copyOf(artifactNames);


This is already the case inside ImmutableList.copyOf AFAICT

Maybe I missed something, but it is called copyOf and I think it always creates a copy.

From the implementation:
ImmutableList<E> list = ((ImmutableCollection<E>) elements).asList(); (which in turn does toArray())
And there is no type check for ImmutableList. Maybe the underlying array is not copied (indicated in the Javadoc, but I can't really find that in the code).

((ImmutableCollection<E>) elements).asList(); on a ImmutableList does return this

.../org/gradle/api/internal/artifacts/ivyservice/resolveengine/graph/builder/SelectorState.java

jjohannes added 10 commits October 9, 2019 20:18

Remove private 'firstSeenDependency' field

03903bb

This is just a shortcut to a final filed in 'dependencyState'. Sometimes it was used and sometimes 'dependencyState.getDepenedency()' was used which is always the same value. This makes this complex code a bit easier to comprehend.

Reuse one object for empty ComponentOverrideMetadata

adf28d2

Support artifacts with different names in maven module fixture

0395f2e

Such artifacts can actually be addressed by 'dependency artifacts' through the 'artifact { }' notation inside a dependency declaration; or in Gradle Module Metadata.

Avoid copying an already immutable list

146c6f9

Add integration test for dependency artifacts in multiple declarations

2738256

This test reproduces #10948 and other variations of it.

Remove unused imports

b279171

jjohannes added in:dependency-management DO NOT USE from:member labels Oct 9, 2019

jjohannes added this to the 6.0 RC1 milestone Oct 9, 2019

jjohannes self-assigned this Oct 9, 2019

jjohannes requested a review from ljacomet October 9, 2019 21:59

jjohannes added 2 commits October 10, 2019 00:13

Do not expect an exact number of HEAD requests

32c2d7d

This seems to be dependent on timing. If one request starts after another did already finish, it can take the result from cache.

Remove duplicated 'isKeyEquals' check

d4fe862

jjohannes force-pushed the jjohannes/artifacts-metadata-source-discovery-fix branch from d9f51e7 to 67b02ff Compare October 10, 2019 08:32

jjohannes commented Oct 10, 2019

View reviewed changes

jjohannes force-pushed the jjohannes/artifacts-metadata-source-discovery-fix branch from 67b02ff to e43500a Compare October 10, 2019 08:45

jjohannes added 3 commits October 10, 2019 10:54

Fix equals() of client module

de1fa3f

Track 'changing' and 'client module' information for override metadata

17bba05

Although these are edge cases, it leads to more consistency and makes the behavior less dependent on order which may change unexpectedly through internal optimisations.

Add test coverage to pin down selector sorting behavior

181205e

We now rely on the selector sorting when choosing a selector to compute override metadata. The sorting puts the most likely used selector first.

jjohannes force-pushed the jjohannes/artifacts-metadata-source-discovery-fix branch from e43500a to 181205e Compare October 10, 2019 08:55

Add missing hashCode() method

f4a6f63

ljacomet approved these changes Oct 10, 2019

View reviewed changes

jjohannes added 3 commits October 10, 2019 12:02

Fix code comment

91c47cb

Add equality check in case update() is called twice for the same input

9ce0df0

Relax flaky test expectation

75f57bf

jjohannes merged commit 0511e1f into release Oct 10, 2019

This was referenced Oct 10, 2019

Regression in 5.5 when using dependency constraints for non-jar dependencies without a POM #10948

Closed

Fix discovery of components without metadata in repositories (5.6.3) #11011

Merged

jjohannes deleted the jjohannes/artifacts-metadata-source-discovery-fix branch November 11, 2019 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix discovery of components without metadata in repositories #10996

Fix discovery of components without metadata in repositories #10996

jjohannes commented Oct 9, 2019 •

edited

jjohannes Oct 10, 2019

ljacomet left a comment

ljacomet Oct 10, 2019

jjohannes Oct 10, 2019

ljacomet Oct 10, 2019

Fix discovery of components without metadata in repositories #10996

Fix discovery of components without metadata in repositories #10996

Conversation

jjohannes commented Oct 9, 2019 • edited

jjohannes Oct 10, 2019

Choose a reason for hiding this comment

ljacomet left a comment

Choose a reason for hiding this comment

ljacomet Oct 10, 2019

Choose a reason for hiding this comment

jjohannes Oct 10, 2019

Choose a reason for hiding this comment

ljacomet Oct 10, 2019

Choose a reason for hiding this comment

jjohannes commented Oct 9, 2019 •

edited