Fixed StackOverflow when adding exclusions #69

Merged
merged 3 commits into from Jul 16, 2014

Conversation

Projects
None yet
2 participants
@brandonkearby
Contributor

brandonkearby commented Jul 5, 2014

Hi John,

I opened a bug on this, then decided to take a whack at it...

Cheers,

-Brandon

@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Jul 7, 2014

Owner

Please add a test to the FilteringSpec class to verify this behavior.

Owner

johnrengelman commented Jul 7, 2014

Please add a test to the FilteringSpec class to verify this behavior.

@@ -9,6 +10,7 @@ import org.gradle.api.specs.Specs
import org.gradle.api.tasks.util.PatternSet
class DependencyFilter {
+ static Logger log = Logger.getLogger(DependencyFilter.class)

This comment has been minimized.

@johnrengelman

johnrengelman Jul 7, 2014

Owner

I'd prefer using the @Slf4j annotation on the class instead of declaring the logger this way.

@johnrengelman

johnrengelman Jul 7, 2014

Owner

I'd prefer using the @Slf4j annotation on the class instead of declaring the logger this way.

@@ -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.info("Excluding: ${it}")}

This comment has been minimized.

@johnrengelman

johnrengelman Jul 7, 2014

Owner

Log this out at debug level instead of info.

@johnrengelman

johnrengelman Jul 7, 2014

Owner

Log this out at debug level instead of info.

}
- matched.addAll(findMatchingDependencies(spec, it.children))

This comment has been minimized.

@johnrengelman

johnrengelman Jul 7, 2014

Owner

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(....)
}
@johnrengelman

johnrengelman Jul 7, 2014

Owner

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(....)
}

This comment has been minimized.

@brandonkearby

brandonkearby Jul 7, 2014

Contributor

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

@brandonkearby

brandonkearby Jul 7, 2014

Contributor

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

@johnrengelman johnrengelman added the bug label Jul 7, 2014

@johnrengelman johnrengelman added this to the 1.0.3 milestone Jul 7, 2014

@@ -406,4 +406,43 @@ class FilteringSpec extends PluginSpecification {
and:
doesNotContain(output, ['a2.properties'])
}
+
+ @Issue("SHADOW-69")
+ def "handle exclude with circular dependency"() {

This comment has been minimized.

@johnrengelman

johnrengelman Jul 7, 2014

Owner

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.

@johnrengelman

johnrengelman Jul 7, 2014

Owner

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.

This comment has been minimized.

@johnrengelman

johnrengelman Jul 7, 2014

Owner

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

@johnrengelman

johnrengelman Jul 7, 2014

Owner

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

This comment has been minimized.

@brandonkearby

brandonkearby Jul 7, 2014

Contributor

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.

@brandonkearby

brandonkearby Jul 7, 2014

Contributor

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.

johnrengelman added a commit that referenced this pull request Jul 16, 2014

Merge pull request #69 from brandonkearby/master
Fixed StackOverflow when adding exclusions

@johnrengelman johnrengelman merged commit 3349095 into johnrengelman:master Jul 16, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment