ben-manes / gradle-versions-plugin

Gradle plugin to discover dependency updates
Apache License 2.0
3.89k stars 200 forks source link

Maven repository not used for resolution #66

Closed jochenberger closed 8 years ago

jochenberger commented 9 years ago

If the plugin is added via the plugins block, it uses the plugin repository to resolve the dependencies instead of the configured one. When using the following build file

plugins {
  id "com.github.ben-manes.versions" version "0.11.3"
}

apply plugin: 'java'

repositories {
    maven { url "https://jcenter.bintray.com/" }
}

dependencies {
    compile 'org.slf4j:slf4j-api:1.7.12'
}

calling gradle -i depU shows:

Resolving with repositories:
 - maven: http://plugins.gradle.org/m2

If I use jcenter() instead, it uses both repositories:

Resolving with repositories:
 - maven: http://plugins.gradle.org/m2
 - BintrayJCenter: https://jcenter.bintray.com/
jochenberger commented 9 years ago

Also, If I call 'gradle -i --refresh-dependencies depU', the output shows

Found locally available resource with matching checksum: [http://plugins.gradle.org/m2/org/slf4j/slf4j-api/1.7.12/slf4j-api-1.7.12.pom, /home/jochen/.gradle/caches/modules-2/files-2.1/org.slf4j/slf4j-api/1.7.12/1163b15d6680795fcc64a14e0c8426a3cc0825a3/slf4j-api-1.7.12.pom]

whereas if I call gradle -i --refresh-dependencies dependencies, it says

Cached resource Thu Mar 26 21:50:00 CET 2015 is up-to-date (lastModified: https://jcenter.bintray.com/org/slf4j/slf4j-api/1.7.12/slf4j-api-1.7.12.pom).

So the dependencyUpdates task uses the plugin repository to resolve compile dependencies.

jochenberger commented 9 years ago

0.10.1 and 0.11.1 use both the jcenter and the plugin repositories. 0.11.2 and 0.11.3 use only the plugins repository.

jochenberger commented 9 years ago

Ah, now I get what you asked in https://github.com/ben-manes/gradle-versions-plugin/commit/339294847ad27829ba0c61713362e1335780c763#commitcomment-11832576. I thought that by "all repositories" you meant "across all subprojects". Well, I think that we should not use buildscript repositories to resolve compile dependencies and vice versa.

jochenberger commented 9 years ago

The problem is that both repositories use the name name ("maven"), so only the first one is added in https://github.com/ben-manes/gradle-versions-plugin/blob/master/src/main/groovy/com/github/benmanes/gradle/versions/updates/Resolver.groovy#L67 (ArtifactRepositoryContainer#add returns false if a repository with the same name exists).

ben-manes commented 9 years ago

I did mean all repositories across all sub-projects =)

I was thinking that instead of aggregating, the configuration already knows where to look for its dependencies. This would avoid nasty surprises, like the naming conflict, but reduce the chance of detecting an update.

I'll separate out the repositories as causing issues.

jochenberger commented 9 years ago

This is making it hard to debug the other issues, so I'd like to find a solution soon. I'm hesitant to change the current behavior, but I'm not sure how to keep the old one without adding duplicate repositories. We could change the repositories and prefix the names with "buildscript" and "project" and remove the prefixes where we restore the repositories, but that feels slightly awkward. The commit in question is c0b83dfd2a1b162dea97832bf3bc37ba65b3a663. The only thing I can think of is switching back to the unique() approach, but with a closure that returns ... I don't know, maybe it.inspect()?

ben-manes commented 9 years ago

I'm not against releasing v0.12 that is simply a revert back to the old version and make this version a beta track. I can't tell yet whether the problems are going to be a big mess, or just a few quirks to iron out.

jochenberger commented 9 years ago

I don't think we need to do that. If something does not work, I (and others) can always revert to an earlier release. Maybe I'll just try to fix this in a local SNAPSHOT and tell you about my findings. If there only was a way to clone() a repository, or a whole project...

ben-manes commented 9 years ago

Thanks. I think I'm just frustrated at myself =)

jochenberger commented 9 years ago

Right, first finding is this:

org.gradle.api.tasks.TaskExecutionException: Execution failed for task ':dependencyUpdates'.
...
Caused by: java.lang.IllegalStateException: The name of an ArtifactRepository cannot be changed after it has been added to a repository container. You should set the name when creating the repository.
    at org.gradle.api.internal.artifacts.repositories.AbstractArtifactRepository.setName(AbstractArtifactRepository.java:37)

That won't work then.

jochenberger commented 9 years ago

Well, I don't think that we can properly determine whether two repositories are equal. I'd say we switch back to using unique() and talk the Gradle guys into implementing hashCode and equals.

ben-manes commented 9 years ago

Can you check to see if the current behavior the same as 0.9? I think it should be.

def repositories = project.allprojects.collectMany { Project proj ->
  (proj.repositories + proj.buildscript.repositories)
}.findAll { project.repositories.add(it) }

I think you're right that we can't do any equality tricks, since new repository types are being added (like S3).

I'm having a hard time justifying to myself that we should still collect all repositories instead of resolve with whatever is associated to a configuration. Can you explain why you prefer using all of them?

ben-manes commented 9 years ago

Also, can you put a sample build file that shows the wrong behavior? When I look at an existing project I see duplicates but unique names, e.g.

Resolving with repositories:
 - MavenLocal: file:/Users/ben/.m2/repository/
 - BintrayJCenter: https://jcenter.bintray.com/
 - maven: http://plugins.gradle.org/m2
 - MavenRepo: https://repo1.maven.org/maven2/
 - MavenLocal2: file:/Users/ben/.m2/repository/
 - MavenRepo2: https://repo1.maven.org/maven2/
 - maven2: http://repo.terracotta.org/maven2
ben-manes commented 9 years ago

I was able to reproduce this by using maven { name = ... url = ... } syntax. If the names conflict on the same repository {} block then a unique name is defined by appending a counter. If the names conflict across repository {} blocks then only one added in the plugin. The current behavior matches 0.9 and earlier, with 0.10.1 fixes this at the cost of duplicate queries.

It seems like we have 3 options,

jochenberger commented 9 years ago

0.9 uses only the project repository, not both of them. Funny, it always felt right but apparently it was never intended to work that way. I think that if we want to act here at all (instead of just restoring the pre-0.11.2 behavior), only the third option makes sense. The issues we'd fix with options 1 and 2 should be fixed within Gradle itself if you ask me.

ben-manes commented 9 years ago

I posted question on the Gradle forum. It actually gets really weird because add and addAll behave differently!

jochenberger commented 9 years ago

Hm, no response on the Gradle forum. Do you want to wait or should we try to find a solution ourselves?

ben-manes commented 9 years ago

I guess we'd better find a solution. Sorry that I haven't gotten back to this, as I've been focused on my other projects lately.

jochenberger commented 9 years ago

No worries, we had to give the Gradle guys time to respond anyway. So, what do you think we should do? I'd still say we should go for option 1 or 3 with a slight tendency toward option 1.

ben-manes commented 9 years ago

I have a slight preference for option 3 :)

If we went with (1) would you want us to try to dedup ourselves? That's obviously error prone, but probably doable for the common case.

jochenberger commented 9 years ago

That would be (2). ;-) I wouldn't try to do that but rather return to using unique() and report a Gradle issue that they should not use duplicate repos.

ben-manes commented 9 years ago

Oh yeah =)

Since the forum post wasn't acknowledge and Gradle devs are the only ones who can create issues in their tracker, the best I can do is ping @alkemist.

What would your recommendation be to users who are hit by the slow resolution? The only workaround, and my general preference anyways, is to use a virtual repository. Then resolution is performed by the Artifactory / Nexus proxy instead of Gradle.

jochenberger commented 9 years ago

Well, my suggestion would be to vote for the Gradle issue in their JIRA. ;-) I don't think we can expect users to setup a repository proxy just for the plugin to work fast, so we'd have to accept that it's slow if we go for option (1). We could start by providing a Gradle patch that implements equals and hashCode for DefaultMavenArtifactRepository, I guess that would solve >90% of the slow resolution issues.

ben-manes commented 9 years ago

Okay, we can send a pull request to patch Gradle. That's the ideal case anyways.

I had a very painful experience getting support for ScalaIDE in Gradle, even though it was a relatively simple change. There is a large backlog of pull requests and community contributions is one of the Gradle's team's few weaknesses. So I'm not optimistic that route will bare fruit or may take 3-6 months of prodding to get merged.

jochenberger commented 9 years ago

Well, we can also go for option 3, I'm not saying that option 1 is necessarily the better choice. Or we could make it configurable, but of course, that adds more complexity.

ben-manes commented 9 years ago

I like 3 because there are fewer surprises, but I'm willing to give sending a pull request a shot. I think making it configurable is unnecessary because a user could just add the repositories globally just as easily.

ben-manes commented 9 years ago

Actually I like the idea of 3 + pull request + doc example a lot. If a user wants all repositories evaluated then they should perform the global addition. Then there are no surprises. To make that friendlier, though, we'd want to submit a pull request to avoid the slowdown.

jochenberger commented 9 years ago

Well then, let's do that and get 0.12 out. :-)

tsegbert commented 9 years ago

Is there any progress on releasing 0.12?

ben-manes commented 9 years ago

Sorry, I've been spending my free time on Caffeine and haven't gotten back to this project.

ben-manes commented 9 years ago

I'm going to try to take a look at this over the weekend and remove the usage of RepositoryHandler. Its buggy behavior struck again, I forget about it when investigating issues, and embarrass myself by misidentify the cause. I'll try to tackle the backlog so we can have another release out soon.

tsegbert commented 8 years ago

Is this issue resolved? It prevents us from updating the plugin. Is release 0.12 still planned?

ben-manes commented 8 years ago

I'll try to go through the others in backlog to take care of the low hanging fruit over the next few days. Regardless of whether I make much of a dent, we should be able to push a release to resolve this glaring bug.