Skip to content

Commit

Permalink
Use RepositoryHandler to avoid duplicate repository listings
Browse files Browse the repository at this point in the history
  • Loading branch information
@ben-manes
ben-manes committed Jun 24, 2015
1 parent 0967855 commit c0b83df
Showing 1 changed file with 8 additions and 3 deletions.
11 changes: 8 additions & 3 deletions src/main/groovy/com/github/benmanes/gradle/versions/updates/Resolver.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,18 @@ class Resolver {
Resolver(Project project) {
this.project = project
this.repositoriesForBuildscript = project.allprojects.collectEntries { proj ->
[proj, proj.buildscript.repositories.asImmutable()]
[proj, new HashSet(proj.buildscript.repositories)]

This comment has been minimized.

Sign in to view

Copy link
@jochenberger

jochenberger Jun 24, 2015

Collaborator

This change isn't necessary, is it? Just curious. It shouldn't change anything AFAICT.

This comment has been minimized.

Sign in to view

Copy link
@ben-manes

ben-manes Jun 24, 2015

Author Owner

It may not be. I saw that DefaultGroovyMethods#asImmutable() is really just a call to Collections.unmodifiableSet(), so the underlying collection could be changed when adding/removing during evaluation.

This comment has been minimized.

Sign in to view

Copy link
@jochenberger

jochenberger Jun 24, 2015

Collaborator

Oops. ;-) Alright then. I thought maybe you changed it as part of the equals investigation.

This comment has been minimized.

Sign in to view

Copy link
@ben-manes

ben-manes Jun 24, 2015

Author Owner

I did, but it seemed safer to keep than revert =)
}
this.repositoriesForProject = project.allprojects.collectEntries { proj ->
[proj, proj.repositories.asImmutable()]
[proj, new HashSet(proj.repositories)]
}

project.repositories.clear()
this.allRepositories = project.allprojects.collectMany { Project proj ->
(proj.repositories + proj.buildscript.repositories)
proj.repositories + proj.buildscript.repositories
}.findAll {

This comment has been minimized.

Sign in to view

Copy link
@jochenberger

jochenberger Jun 24, 2015

Collaborator

Any specific reason for using findAll instead of each?

This comment has been minimized.

Sign in to view

Copy link
@ben-manes

ben-manes Jun 24, 2015

Author Owner

Its a filtering constraint and it looks like each returns the original collection.

> ​[1, 2, 3, 4].findAll { (it % 2) == 0 }​
[2, 4]

> ​[1, 2, 3, 4].each { (it % 2) == 0 }​
[1, 2, 3, 4]

This comment has been minimized.

Sign in to view

Copy link
@jochenberger

jochenberger Jun 24, 2015

Collaborator

I completely misread it, sorry, I didn't notice it was part of an assignment. I should probably get some sleep...
// Only RepositoryHandler knows how to determine equivalence
project.repositories.add(it)
} as Set
logRepositories()
}
Expand Down

8 comments on commit c0b83df

@jochenberger
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow, this doesn't seem right. Now, in a multi-module project, only the root project uses all repositories to resolve the latest versions, the subprojects don't use any repositories at all and all dependencies are listed as unresolved.

@jochenberger
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, the problem is related to the project.repositories.clear() in line 61.

@ben-manes
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided I was too tired and went to sleep. I guess we'll have to fix this and do another release?

@jochenberger
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sleep sounds great, too bad it's only 4pm here...
I found out that this happens only for a project where I apply the plugin to the root project only. If I apply it to the subprojects, everything's fine.

@ben-manes
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah.. its just past 7am here. I of course forgot to run tests before I pushed these changes so this is a mess. I'll have a fix shortly.

@ben-manes
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, pushed a version that actually passes the tests =)

I'll be on the train with slow wifi, but be available.

@jochenberger
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, anything left for 0.11.3?

@ben-manes
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, lets cross our fingers and release =)

Please sign in to comment.