Skip to content

Commit

Permalink
Only resolve useful configurations
Browse files Browse the repository at this point in the history 
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
  • Loading branch information
@SUPERCILEX
SUPERCILEX committed Feb 22, 2020
1 parent 8cfd796 commit 1c6627e
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 2 deletions.
9 changes: 8 additions & 1 deletion src/main/groovy/com/github/benmanes/gradle/versions/updates/DependencyUpdates.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,14 @@ class DependencyUpdates {
Set<Configuration> configurations = projectConfigs.get(proj)
Resolver resolver = new Resolver(proj, resolutionStrategy, checkConstraints)
configurations.collect { Configuration config ->
resolve(resolver, proj, config)
def isUsefulConfiguration = !config.canBeResolved || config.canBeConsumed ||

This comment has been minimized.

Sign in to view

Copy link
@chrimaeon

chrimaeon Mar 4, 2020

these changes brake some configurations resolution added with the android gradle plugin - i.e. lintCheck and lintPublish which are canBeResolved=true and canBeConsumed=false

This comment has been minimized.

Sign in to view

Copy link
@anuraaga

anuraaga Mar 4, 2020 •
edited

Collaborator

@jjohannes Would you be able to advise on what is a good way to recognize a configuration where users define versions, and hence are applicable for this plugin which suggests version upgrades? If I understand correctly, anything other than "Resolve for certain usage" allow user dependency versions and this check is correct. Meaning the Android plugin maybe incorrect.

https://docs.gradle.org/current/userguide/declaring_dependencies.html#sec:resolvable-consumable-configs

Either way we could probably add android plugin specific workarounds here but want to confirm out standard behavior is ok.

This comment has been minimized.

Sign in to view

Copy link
@ben-manes

ben-manes Mar 4, 2020

Owner

@chrimaeon please open an issue if you want changes, as commit comments isn’t really a place for these discussions.

This comment has been minimized.

Sign in to view

Copy link
@chrimaeon

chrimaeon Mar 4, 2020

Issue is already created (#382) just wanted to point what these changes do.

This comment has been minimized.

Sign in to view

Copy link
@ben-manes

ben-manes Mar 4, 2020

Owner

Thanks!

This comment has been minimized.

Sign in to view

Copy link
@chrimaeon

chrimaeon Mar 4, 2020

@anuraaga as far as I understand, I can define a dependency on any kind of configuration. Either internally uses by my own plugins or needed for the "application" itself. lintCheck for example is used to define linters for Android development and so it is internally used by the plugin and that's why the configuration is not canBeConsumed. https://docs.gradle.org/current/userguide/declaring_dependencies.html#sec:choosing-configuration

This comment has been minimized.

Sign in to view

Copy link
@chrimaeon

chrimaeon Mar 4, 2020

@anuraaga maybe lets discuss in the Issue Ticket #382

This comment has been minimized.

Sign in to view

Copy link
@jjohannes

jjohannes Mar 5, 2020

@anuraaga can you elaborate why you added that filtering in the first place?

If I understand you approach correctly, you look at all the dependencies/constraints directly declared on a configuration.

Although we do not enforce it, currently the encouraged way is to use the following configurations for that:

  • canBeConsumed=false && canBeResolved=false - this is the ideal case - a configuration only for declaring dependencies
  • canBeConsumed=true && canBeResolved=false - this is sometimes used as shortcut to avoid an additional configuration (even in Gradle itself). Although we are discussing to get rid of it in the future.
  • canBeConsumed=true && canBeResolved=true - this is still the default when you create a configuration. So you see this often only because no one bothers to set the flags. We will probably move away from this at some point in the future.

That being said, you can technically declare dependencies on any configuration (even if some cases are discouraged). So why don't you just look at all of them?

This comment has been minimized.

Sign in to view

Copy link
@SUPERCILEX

SUPERCILEX Mar 5, 2020

Author Collaborator

@jjohannes I made this change with the reasoning being to limit unresolvable configurations. Previously, the plugin would spew out various errors about failing to resolve a bunch of different configurations. I thought I had caught just the right configurations with this if condition, but I guess not.

Is there a way to get declared dependencies without resolving the configuration?

This comment has been minimized.

Sign in to view

Copy link
@jjohannes

jjohannes Mar 5, 2020

Configuration.getDependencies() won't trigger the resolving. I think that should be all you need. What in your code is triggering the resolving?

This comment has been minimized.

Sign in to view

Copy link
@SUPERCILEX

SUPERCILEX Mar 5, 2020

Author Collaborator

@jjohannes Oh right, I remember what we're doing now. The idea is to create a copy of the configuration and change its dependencies to a + as the version. Then, we resolve the configuration to let Gradle figure what the latest version is for us.

Since I noticed you work at Gradle, how should we be doing this the right way?

This comment has been minimized.

Sign in to view

Copy link
@jjohannes

jjohannes Mar 5, 2020

@SUPERCILEX without knowing all the details, I think it is fine if you just do the copy (a detached configuration I assume?) and resolve that. Even if the original configuration is "canBeResolved=false".

This comment has been minimized.

Sign in to view

Copy link
@ben-manes

ben-manes Mar 5, 2020

Owner

We use copyRecursive to capture all of the original configuration. We used to use detached but kept hitting Gradle bugs, e.g. adding repositories had broken dedup’ing logic.
config.name == 'annotationProcessor' || config.name == 'kapt'

This comment has been minimized.

Sign in to view

Copy link
@chrimaeon

chrimaeon Mar 4, 2020

Also with AGP (android gradle plugin) one can have build types like "release" and "debug" and so you can have a configuration named "debugAnnotationProcessor" which will not work with those changes as well.

This comment has been minimized.

Sign in to view

Copy link
@anuraaga

anuraaga Mar 4, 2020

Collaborator

Again we can add android plugin specific code here. But I would really only be happy with it knowing we have filed issues upstream to fix the plugin itself as needed. AFAIU, these hard coded workarounds are for legacy gradle-apt-plugin type of configurations and not Gradle's native ones which should pass our current checks.

This comment has been minimized.

Sign in to view

Copy link
@chrimaeon

chrimaeon Mar 4, 2020

Yes, just checked, the "native" annotationProcessor has a configuration of !canBeResolved and canBeConsumed and kapt as well.

if (isUsefulConfiguration) {
resolve(resolver, proj, config)
} else {
[]
}
}.flatten() as Set<DependencyStatus>
}.flatten() as Set<DependencyStatus>
}
Expand Down
25 changes: 24 additions & 1 deletion src/test/groovy/com/github/benmanes/gradle/versions/DependencyUpdatesSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ import spock.lang.Issue
import spock.lang.Specification
import spock.lang.Unroll

import static com.github.benmanes.gradle.versions.updates.gradle.GradleReleaseChannel.*
import static com.github.benmanes.gradle.versions.updates.gradle.GradleReleaseChannel.CURRENT
import static com.github.benmanes.gradle.versions.updates.gradle.GradleReleaseChannel.RELEASE_CANDIDATE

/**
* A specification for the dependency updates task.
Expand Down Expand Up @@ -401,6 +402,28 @@ final class DependencyUpdatesSpec extends Specification {
}
}

def 'Single project with annotation processor'() {
given:
def project = singleProject()
project.plugins.apply('java')
addRepositoryTo(project)
project.dependencies {
annotationProcessor 'com.google.guava:guava:99.0-SNAPSHOT'
}

when:
def reporter = evaluate(project)
reporter.write()

then:
with(reporter) {
unresolved.isEmpty()
upgradeVersions.isEmpty()
upToDateVersions.isEmpty()
downgradeVersions == [['group': 'com.google.guava', 'name': 'guava']: '99.0-SNAPSHOT']
}
}

def 'Single project with component selection rule'() {
given:
def project = new ProjectBuilder().withName('single').build()
Expand Down

0 comments on commit 1c6627e

Please sign in to comment.