Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Candidate set provided to AttributeDisambiguationRule contains null entry #6747

Closed
jdochez opened this issue Sep 13, 2018 · 9 comments
Closed
Labels
Milestone

Comments

@jdochez
Copy link
Contributor

jdochez commented Sep 13, 2018

This is a mirror bug for :

https://issuetracker.google.com/issues/112231683

if you check out the small example project, you will be able to reproduce the NPE by changing
libx/build.gradle

// releaseDebug {
// }

basically removing the build type on the library forces the resolution strategy to kick in in the app module.

when android-kotlin plugin is disabled, it works. When it is enabled, NPE during configuration.

I have checked that we set up the AttributeMatchingStrategy correctly independently of the kotlin plugin presence so it does not seem to be an android gradle plugin issue.

@lptr lptr added the in:dependency-management DO NOT USE label Sep 13, 2018
@mtrakal
Copy link

mtrakal commented Sep 13, 2018

Adding info for users which are not from Google corp: https://issuetracker.google.com/issues/112231683 because they can't use link on #6747 (comment)

Link to repo: https://github.com/mtrakal/issue112231683 which allow reproduce this bug

@bigdaz bigdaz added this to the 4.10.2 milestone Sep 14, 2018
@bigdaz
Copy link
Member

bigdaz commented Sep 14, 2018

Thanks for reporting this issue. I've done some debugging, and the fundamental issue is caused by the fact that MultipleCandidateDetails.getCandidateValues() can currently contain a null value, if any of the variants are missing this attribute.

Analysis

Applying the kotlin-android plugin adds a bunch of additional variants, none of which have the BuildTypeAttr attribute set, and this results in a null being added to the candidate values. In order to choose the correct variant, AlternateDisambiguationRule is invoked and attempts to call getName() on this null value, resulting in a NPE. This doesn't happen when the candidate values contain an exact match for the requested attribute, since AlternateDisambiguationRule has a simpler code path in this case (ie when there is a releaseDebug build type variant).

Note that this is further complicated by the fact that our exception reporting also barfs on the null candidate value, resulting in a different NPE that masks the NPE emanating from AlternateDisambiguationRule.

Solution

I'm attempting to find a fix that eliminates the null value from the candidate set, but this is proving elusive so far. If I manage to find a relatively simple fix I'll try to get it into the upcoming 4.10.2, but otherwise it's likely to slip into 5.0.

In the meantime, you should be able to update AlternateDisambiguationRule to check for null values when iterating over the candidates. I haven't been able to test this.

@bigdaz bigdaz changed the title Matching strategies conflict with android-kotlin plugin. Candidate set provided to AttributeDisambiguationRule contains null entry Sep 16, 2018
@bigdaz bigdaz closed this as completed in 9851ca5 Sep 17, 2018
@bigdaz
Copy link
Member

bigdaz commented Sep 17, 2018

@jdochez We've implemented a fix which will be in the upcoming 4.10.2 release. It would be great if you could try it out before the release so we know for sure that the issue is fixed (and that the change didn't introduce any other problems).

You can grab a pre-release distro from http://services.gradle.org/distributions-snapshots/gradle-4.10.2-20180917175211+0000-all.zip

@bigdaz
Copy link
Member

bigdaz commented Sep 17, 2018

@mtrakal Please try out the above nightly that contains a fix for this issue. ☝️

@mikescamell
Copy link

@bigdaz We had this issue in our project as well and I can confirm that the pre-release distro works for us 👍

@mtrakal
Copy link

mtrakal commented Sep 18, 2018

@bigdaz I can confirm that's working like a charm too :). Thanks a lot for the fix 👍 🥇

@eric-labelle
Copy link

Awesome, works for me too with the 4.10.2 nightly :) I can finally remove the useless buildType in all my libraries modules which was a temporary hack in order to have all the same buildType in all modules!
Thanks

@h0tk3y
Copy link
Member

h0tk3y commented Dec 10, 2018

IMO the fix for this issue somewhat introduces a conceptual flaw into attributes disambiguation process: there is no more a way to disambiguate an absent attribute (null) from a single value in case that value doesn't match the requested attribute. So if there are candidates with a certain value and no value (i.e. a value foo and null, and the requested value is bar or null) then no rule can disambiguate them since no rule even gets called.

In the code of the kotlin-gradle-plugin, we have a rule that makes sure a project(...) dependency gets resolved into one of the *Elements configurations, not one of the deprecated ones which canBeConsumed = true and still have no proper Usage attributes (unless specified otherwise with e.g. configuration: 'compile').

We add a special attribute <...>.localToProject to those configurations which we don't want to be chosen, and the rule prefers the null value over any other value, so none of the configurations marked with the attribute could be chosen over the configurations not marked with the attribute.

Prior to 4.10.2, this worked fine. In 4.10.2+, ambiguity arises due to the null value not disambiguated properly from a single alternative value:

Cannot choose between the following variants of project ::
  - metadataApiElements
  - metadataCompile
  - metadataCompileOnly
  - metadataDefault
All of them match the consumer attributes:
  - Variant 'metadataApiElements':
      - Found org.gradle.usage 'kotlin-api' but wasn't required.
      - Found org.jetbrains.kotlin.platform.type 'common' but wasn't required.
  - Variant 'metadataCompile':
      - Found org.gradle.usage 'kotlin-api' but wasn't required.
      - Found org.jetbrains.kotlin.localToProject ':' but wasn't required.
      - Found org.jetbrains.kotlin.platform.type 'common' but wasn't required.
  - Variant 'metadataCompileOnly':
      - Found org.gradle.usage 'kotlin-api' but wasn't required.
      - Found org.jetbrains.kotlin.localToProject ':' but wasn't required.
      - Found org.jetbrains.kotlin.platform.type 'common' but wasn't required.
  - Variant 'metadataDefault':
      - Found org.gradle.usage 'kotlin-api' but wasn't required.
      - Found org.jetbrains.kotlin.localToProject ':' but wasn't required.
      - Found org.jetbrains.kotlin.platform.type 'common' but wasn't required.

@melix
Copy link
Contributor

melix commented Dec 11, 2018

Thanks for the insight @h0tk3y .

We add a special attribute <...>.localToProject to those configurations which we don't want to be chosen, and the rule prefers the null value over any other value, so none of the configurations marked with the attribute could be chosen over the configurations not marked with the attribute.

I think this is the problem, it should have been setup the other way around: attributes are used to select a configuration, not to exclude them. So preferring null should have been an error in the first place. The selection algorithm makes use of configurations with attributes, and should select a configuration that has the appropriate value.

So one option is just to add the attribute to every configuration, and set a value like internal and have a compatibility rule that says that this value is not compatible with any queried value.

h0tk3y added a commit to JetBrains/kotlin that referenced this issue Dec 12, 2018
In Gradle 4.10.2, a change was made in the attributes disambiguation
process: some if the candidates have a single attribute value and others
don't have that attribute (i.e. have a null value), the disambiguation
rule for that attribute doesn't get called. Effectively, null values are
excluded from disambiguation, but still may cause ambiguity (sic!)

See: gradle/gradle#6747 (comment)

This change affected our attribute `localToProject` that we use to
disambiguate the deprecated but still consumable configurations like
`compile`, `runtime`, `testCompile`, `testRuntime` from those
configurations which should have priority during project dependency
resolution: the `*Element` ones. Our scheme marked the former
configurations with the attribute and the latter were not marked, with
a disambiguation rule that preferred null values.

To fix this logic with Gradle 4.10.2, we instead mark both kinds of
configurations with the attribute, which now has a value 'public'
that is preferred by the rule over the other values. We also make sure
that the attribute doesn't leak into the published Gradle metadata, as
it is only needed for project-to-project dependencies resolution.
h0tk3y added a commit to JetBrains/kotlin that referenced this issue Dec 12, 2018
In Gradle 4.10.2, a change was made in the attributes disambiguation
process: some if the candidates have a single attribute value and others
don't have that attribute (i.e. have a null value), the disambiguation
rule for that attribute doesn't get called. Effectively, null values are
excluded from disambiguation, but still may cause ambiguity (sic!)

See: gradle/gradle#6747 (comment)

This change affected our attribute `localToProject` that we use to
disambiguate the deprecated but still consumable configurations like
`compile`, `runtime`, `testCompile`, `testRuntime` from those
configurations which should have priority during project dependency
resolution: the `*Element` ones. Our scheme marked the former
configurations with the attribute and the latter were not marked, with
a disambiguation rule that preferred null values.

To fix this logic with Gradle 4.10.2, we instead mark both kinds of
configurations with the attribute, which now has a value 'public'
that is preferred by the rule over the other values. We also make sure
that the attribute doesn't leak into the published Gradle metadata, as
it is only needed for project-to-project dependencies resolution.
h0tk3y added a commit to JetBrains/kotlin that referenced this issue Dec 12, 2018
In Gradle 4.10.2, a change was made in the attributes disambiguation
process: if some of the candidates have a single attribute value and
others don't have that attribute (i.e. have a null value), the
disambiguation rule for that attribute doesn't get called.
Effectively, null values are excluded from disambiguation, but still may
cause ambiguity (sic!)

See: gradle/gradle#6747 (comment)

This change affected our attribute `localToProject` that we use to
disambiguate the deprecated but still consumable configurations like
`compile`, `runtime`, `testCompile`, `testRuntime` from those
configurations which should have priority during project dependency
resolution: the `*Element` ones. Our scheme marked the former
configurations with the attribute and the latter were not marked, with
a disambiguation rule that preferred null values.

To fix this logic with Gradle 4.10.2, we instead mark both kinds of
configurations with the attribute, which now has a value 'public'
that is preferred by the rule over the other values. We also make sure
that the attribute doesn't leak into the published Gradle metadata, as
it is only needed for project-to-project dependencies resolution.

Issue #KT-28795 Fixed
h0tk3y added a commit to JetBrains/kotlin that referenced this issue Dec 12, 2018
In Gradle 4.10.2, a change was made in the attributes disambiguation
process: if some of the candidates have a single attribute value and
others don't have that attribute (i.e. have a null value), the
disambiguation rule for that attribute doesn't get called.
Effectively, null values are excluded from disambiguation, but still may
cause ambiguity (sic!)

See: gradle/gradle#6747 (comment)

This change affected our attribute `localToProject` that we use to
disambiguate the deprecated but still consumable configurations like
`compile`, `runtime`, `testCompile`, `testRuntime` from those
configurations which should have priority during project dependency
resolution: the `*Element` ones. Our scheme marked the former
configurations with the attribute and the latter were not marked, with
a disambiguation rule that preferred null values.

To fix this logic with Gradle 4.10.2, we instead mark both kinds of
configurations with the attribute, which now has a value 'public'
that is preferred by the rule over the other values. We also make sure
that the attribute doesn't leak into the published Gradle metadata, as
it is only needed for project-to-project dependencies resolution.

Issue #KT-28795 Fixed
h0tk3y added a commit to JetBrains/kotlin that referenced this issue Dec 17, 2018
In Gradle 4.10.2, a change was made in the attributes disambiguation
process: if some of the candidates have a single attribute value and
others don't have that attribute (i.e. have a null value), the
disambiguation rule for that attribute doesn't get called.
Effectively, null values are excluded from disambiguation, but still may
cause ambiguity (sic!)

See: gradle/gradle#6747 (comment)

This change affected our attribute `localToProject` that we use to
disambiguate the deprecated but still consumable configurations like
`compile`, `runtime`, `testCompile`, `testRuntime` from those
configurations which should have priority during project dependency
resolution: the `*Element` ones. Our scheme marked the former
configurations with the attribute and the latter were not marked, with
a disambiguation rule that preferred null values.

To fix this logic with Gradle 4.10.2, we instead mark both kinds of
configurations with the attribute, which now has a value 'public'
that is preferred by the rule over the other values. We also make sure
that the attribute doesn't leak into the published Gradle metadata, as
it is only needed for project-to-project dependencies resolution.

Issue #KT-28795 Fixed
h0tk3y added a commit to JetBrains/kotlin that referenced this issue Dec 17, 2018
In Gradle 4.10.2, a change was made in the attributes disambiguation
process: if some of the candidates have a single attribute value and
others don't have that attribute (i.e. have a null value), the
disambiguation rule for that attribute doesn't get called.
Effectively, null values are excluded from disambiguation, but still may
cause ambiguity (sic!)

See: gradle/gradle#6747 (comment)

This change affected our attribute `localToProject` that we use to
disambiguate the deprecated but still consumable configurations like
`compile`, `runtime`, `testCompile`, `testRuntime` from those
configurations which should have priority during project dependency
resolution: the `*Element` ones. Our scheme marked the former
configurations with the attribute and the latter were not marked, with
a disambiguation rule that preferred null values.

To fix this logic with Gradle 4.10.2, we instead mark both kinds of
configurations with the attribute, which now has a value 'public'
that is preferred by the rule over the other values. We also make sure
that the attribute doesn't leak into the published Gradle metadata, as
it is only needed for project-to-project dependencies resolution.

Issue #KT-28795 Fixed

(cherry picked from commit 557fb07)
DeFuex pushed a commit to DeFuex/kotlin that referenced this issue Jan 27, 2019
In Gradle 4.10.2, a change was made in the attributes disambiguation
process: if some of the candidates have a single attribute value and
others don't have that attribute (i.e. have a null value), the
disambiguation rule for that attribute doesn't get called.
Effectively, null values are excluded from disambiguation, but still may
cause ambiguity (sic!)

See: gradle/gradle#6747 (comment)

This change affected our attribute `localToProject` that we use to
disambiguate the deprecated but still consumable configurations like
`compile`, `runtime`, `testCompile`, `testRuntime` from those
configurations which should have priority during project dependency
resolution: the `*Element` ones. Our scheme marked the former
configurations with the attribute and the latter were not marked, with
a disambiguation rule that preferred null values.

To fix this logic with Gradle 4.10.2, we instead mark both kinds of
configurations with the attribute, which now has a value 'public'
that is preferred by the rule over the other values. We also make sure
that the attribute doesn't leak into the published Gradle metadata, as
it is only needed for project-to-project dependencies resolution.

Issue #KT-28795 Fixed
melix added a commit that referenced this issue Feb 14, 2019
This commit implements a smarter strategy for attribute disambiguation,
whenever _some_ variants have missing information. By missing information,
we mean that some variants carry at least one extra attribute, and others
do not. Because we consider the absence of an attribute as a compatible
match, BUT that when we disambiguate we only consider the variants which
actually provided a value, there was no way to prefer a match which
is "closer" to the request.

See #6747
melix added a commit that referenced this issue Feb 18, 2019
This commit implements a smarter strategy for attribute disambiguation,
whenever _some_ variants have missing information. By missing information,
we mean that some variants carry at least one extra attribute, and others
do not. Because we consider the absence of an attribute as a compatible
match, BUT that when we disambiguate we only consider the variants which
actually provided a value, there was no way to prefer a match which
is "closer" to the request.

See #6747
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants