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.

Sign up for GitHub

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

Jump to bottom

Fixed StackOverflow when adding exclusions #69

Merged
merged 3 commits into from
Jul 16, 2014
Merged

Fixed StackOverflow when adding exclusions #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be743e0
Fixed StackOverflow when adding exclusions
brandonkearby Jul 5, 2014
6030c21
Added @Slf4j, changed log level to debug. Added unit test for circula…
brandonkearby Jul 7, 2014
1256c68
Added @Issue(SHADOW-69) to unit test
brandonkearby Jul 7, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions gradle/wrapper/gradle-wrapper.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#Tue Apr 29 12:45:22 CDT 2014
#Sat Jul 05 17:55:58 CDT 2014
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-1.12-bin.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-1.12-all.zip
28 changes: 24 additions & 4 deletions src/main/groovy/com/github/jengelman/gradle/plugins/shadow/internal/DependencyFilter.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.github.jengelman.gradle.plugins.shadow.internal

import groovy.util.logging.Slf4j
import org.apache.commons.io.FilenameUtils
import org.gradle.api.Project
import org.gradle.api.artifacts.Dependency
Expand All @@ -8,6 +9,7 @@ import org.gradle.api.specs.Spec
import org.gradle.api.specs.Specs
import org.gradle.api.tasks.util.PatternSet

@Slf4j
class DependencyFilter {

private final Project project
Expand All @@ -31,6 +33,7 @@ class DependencyFilter {
dependencies.collect { it.moduleArtifacts.file }.flatten().each { File file ->
this.patternSet.exclude(FilenameUtils.getName(file.path))
}
dependencies.each { log.debug("Excluding: ${it}")}
return this
}

Expand Down Expand Up @@ -115,21 +118,38 @@ class DependencyFilter {
Specs.<? super ResolvedDependency>convertClosureToSpec(spec), dependencies)
}


/**
* Support method for querying the resolved dependency graph using maven/project coordinates
* @param spec
* @param dependencies
* @return
*/
protected Set<ResolvedDependency> findMatchingDependencies(Spec<? super ResolvedDependency> spec,
Set<ResolvedDependency> dependencies) {
Set<ResolvedDependency> dependencies) {
Set<ResolvedDependency> visitedDependencies = new HashSet<ResolvedDependency>()
return findMatchingDependenciesImpl(visitedDependencies, spec, dependencies)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need to split this out into a separate method. You could rely on the behavior of Set that it returns a boolean only if the item is added to the Set. So something like,

if (spec.isSatisifiedBy(it) && matched.add(it)) {
  matched.addAll(....)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this would work, but you still need to recurse into the children, even if it doesn't match


/**
* Impl method for querying the resolved dependency graph using maven/project coordinates
* @param spec
* @param dependencies
* @return
*/
private Set<ResolvedDependency> findMatchingDependenciesImpl(Set<ResolvedDependency> visitedDependencies,
Spec<? super ResolvedDependency> spec,
Set<ResolvedDependency> dependencies) {

Set<ResolvedDependency> matched = []
dependencies.each {
if (spec.isSatisfiedBy(it)) {
matched.add(it)
if (!visitedDependencies.contains(it)) {
visitedDependencies.add(it)
if (spec.isSatisfiedBy(it)) {
matched.add(it)
}
matched.addAll(findMatchingDependenciesImpl(visitedDependencies, spec, it.children))
}
matched.addAll(findMatchingDependencies(spec, it.children))
}
return matched
}
Expand Down
39 changes: 39 additions & 0 deletions src/test/groovy/com/github/jengelman/gradle/plugins/shadow/FilteringSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,4 +406,43 @@ class FilteringSpec extends PluginSpecification {
and:
doesNotContain(output, ['a2.properties'])
}

@Issue("SHADOW-69")
def "handle exclude with circular dependency"() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there actual example of this out in the wild (i.e. circular dependencies in the POM files)?
Do you have an example of one. I'd like to look at it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm assuming you ran into one and that's what revealed this problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the original bug had my stacktrace from real code in the wild. The dependencies where pretty messy, so it wasn't as clean as the simple unit test.

given:
repo.module('shadow', 'c', '1.0')
.insertFile('c.properties', 'c')
.dependsOn('d')
.publish()
repo.module('shadow', 'd', '1.0')
.insertFile('d.properties', 'd')
.dependsOn('c')
.publish()

buildFile << '''
|dependencies {
| compile 'shadow:d:1.0'
|}
|
|shadowJar {
| dependencies {
| exclude(dependency('shadow:d:1.0'))
| }
|}
'''.stripMargin()

when:
runner.arguments << 'shadowJar'
ExecutionResult result = runner.run()

then:
success(result)

and:
contains(output, ['a.properties', 'a2.properties', 'b.properties', 'c.properties'])

and:
doesNotContain(output, ['d.properties'])
}

}