diff options
author | 2018-04-09 10:59:59 -0700 | |
---|---|---|
committer | 2018-04-09 11:01:17 -0700 | |
commit | e4fda37182b2e9ad72f6a91cc71ccc11f6fdea00 (patch) | |
tree | 66f959d8f815ffb23760e0d9acf5664e806946d3 | |
parent | bef08f66cdaf77e31b3fc44b398b4f8a6e5c0c0b (diff) |
Fix issue with filtered resources
Apparently, I was using a flawed strategy to compute the filtered resource root
paths. Instead, just recalculate those roots from scratch, replicating the
behavior that existed before the change that caused problems.
RELNOTES: none
PiperOrigin-RevId: 192152306
7 files changed, 40 insertions, 35 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResources.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResources.java index b5752873b3..efc3229e9b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResources.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResources.java @@ -351,13 +351,14 @@ public class AndroidResources { * Filters this object, assuming it contains the resources of the current target. * * <p>If this object contains the resources from a dependency of this target, use {@link - * #maybeFilter(ResourceFilter, boolean)} instead. + * #maybeFilter(RuleErrorConsumer, ResourceFilter, boolean)} instead. * * @return a filtered {@link AndroidResources} object. If no filtering was done, this object will * be returned. */ - public AndroidResources filterLocalResources(ResourceFilter resourceFilter) { - return maybeFilter(resourceFilter, /* isDependency = */ false).orElse(this); + public AndroidResources filterLocalResources( + RuleErrorConsumer errorConsumer, ResourceFilter resourceFilter) throws RuleErrorException { + return maybeFilter(errorConsumer, resourceFilter, /* isDependency = */ false).orElse(this); } /** @@ -368,7 +369,8 @@ public class AndroidResources { * filtered. */ public Optional<AndroidResources> maybeFilter( - ResourceFilter resourceFilter, boolean isDependency) { + RuleErrorConsumer errorConsumer, ResourceFilter resourceFilter, boolean isDependency) + throws RuleErrorException { Optional<ImmutableList<Artifact>> filtered = resourceFilter.maybeFilter(resources, /* isDependency= */ isDependency); @@ -377,18 +379,10 @@ public class AndroidResources { return Optional.empty(); } - // If the resources were filtered, also filter the resource roots - ImmutableList.Builder<PathFragment> filteredResourcesRootsBuilder = ImmutableList.builder(); - for (PathFragment resourceRoot : resourceRoots) { - for (Artifact resource : filtered.get()) { - if (resource.getRootRelativePath().startsWith(resourceRoot)) { - filteredResourcesRootsBuilder.add(resourceRoot); - break; - } - } - } - - return Optional.of(new AndroidResources(filtered.get(), filteredResourcesRootsBuilder.build())); + return Optional.of( + new AndroidResources( + filtered.get(), + getResourceRoots(errorConsumer, filtered.get(), DEFAULT_RESOURCES_ATTR))); } public ParsedAndroidResources parse( diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java b/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java index 38069bb0a8..43e52d6927 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java @@ -436,8 +436,8 @@ public final class ApplicationManifest { ResourceFilterFactory.fromRuleContext(ruleContext); ResourceFilter resourceFilter = resourceFilterFactory.getResourceFilter(ruleContext, resourceDeps, resources); - resources = resources.filterLocalResources(resourceFilter); - resourceDeps = resourceDeps.filter(resourceFilter); + resources = resources.filterLocalResources(ruleContext, resourceFilter); + resourceDeps = resourceDeps.filter(ruleContext, resourceFilter); // Now that the LocalResourceContainer has been filtered, we can build a filtered resource // container from it. @@ -506,8 +506,8 @@ public final class ApplicationManifest { AndroidResources resources = AndroidResources.from(ruleContext, "resource_files"); ResourceFilter resourceFilter = resourceFilterFactory.getResourceFilter(ruleContext, resourceDeps, resources); - resources = resources.filterLocalResources(resourceFilter); - resourceDeps = resourceDeps.filter(resourceFilter); + resources = resources.filterLocalResources(ruleContext, resourceFilter); + resourceDeps = resourceDeps.filter(ruleContext, resourceFilter); // Now that the LocalResourceContainer has been filtered, we can build a filtered resource // container from it. @@ -628,7 +628,7 @@ public final class ApplicationManifest { .setCompiledSymbolsOutput(resourceContainer.getCompiledSymbols()); if (dataBindingInfoZip != null && resourceContainer.getCompiledSymbols() != null) { - PathFragment unusedInfo = dataBindingInfoZip.getRootRelativePath(); + PathFragment unusedInfo = dataBindingInfoZip.getRootRelativePath(); // TODO(corysmith): Centralize the data binding processing and zipping into a single // action. Data binding processing needs to be triggered here as well as the merger to // avoid aapt2 from throwing an error during compilation. diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceContainer.java b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceContainer.java index 2bb66189a7..971fed1c3f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceContainer.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceContainer.java @@ -23,6 +23,8 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; +import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException; +import com.google.devtools.build.lib.packages.RuleErrorConsumer; import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable; import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; @@ -193,8 +195,11 @@ public abstract class ResourceContainer implements MergableAndroidData { * Returns a copy of this container with filtered resources, or the original if no resources * should be filtered. The original container is unchanged. */ - public ResourceContainer filter(ResourceFilter filter, boolean isDependency) { - Optional<AndroidResources> filteredResources = getResources().maybeFilter(filter, isDependency); + public ResourceContainer filter( + RuleErrorConsumer errorConsumer, ResourceFilter filter, boolean isDependency) + throws RuleErrorException { + Optional<AndroidResources> filteredResources = + getResources().maybeFilter(errorConsumer, filter, isDependency); if (!filteredResources.isPresent()) { // No filtering was done; return this container diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceDependencies.java b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceDependencies.java index a9ee3b34fa..7a9b91046e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceDependencies.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceDependencies.java @@ -25,6 +25,8 @@ import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.packages.AttributeMap; import com.google.devtools.build.lib.packages.BuildType; +import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException; +import com.google.devtools.build.lib.packages.RuleErrorConsumer; import java.util.Optional; import javax.annotation.Nullable; @@ -195,7 +197,8 @@ public final class ResourceDependencies { } /** Returns a copy of this instance with filtered resources. The original object is unchanged. */ - public ResourceDependencies filter(ResourceFilter resourceFilter) { + public ResourceDependencies filter(RuleErrorConsumer errorConsumer, ResourceFilter resourceFilter) + throws RuleErrorException { Optional<NestedSet<Artifact>> filteredResources = resourceFilter.maybeFilterDependencies(transitiveResources); @@ -208,8 +211,8 @@ public final class ResourceDependencies { // means that if any resource changes, the corresponding actions will get // re-executed return withResources( - resourceFilter.filterDependencyContainers(transitiveResourceContainers), - resourceFilter.filterDependencyContainers(directResourceContainers), + resourceFilter.filterDependencyContainers(errorConsumer, transitiveResourceContainers), + resourceFilter.filterDependencyContainers(errorConsumer, directResourceContainers), filteredResources.get()); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceFilter.java b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceFilter.java index 74f7b27f67..ff8c8112d6 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceFilter.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceFilter.java @@ -18,6 +18,8 @@ import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; +import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException; +import com.google.devtools.build.lib.packages.RuleErrorConsumer; import java.util.List; import java.util.Optional; import java.util.function.Consumer; @@ -64,7 +66,8 @@ public class ResourceFilter { } public NestedSet<ResourceContainer> filterDependencyContainers( - NestedSet<ResourceContainer> resourceContainers) { + RuleErrorConsumer errorConsumer, NestedSet<ResourceContainer> resourceContainers) + throws RuleErrorException { if (isEmpty) { return resourceContainers; } @@ -73,7 +76,7 @@ public class ResourceFilter { new NestedSetBuilder<>(resourceContainers.getOrder()); for (ResourceContainer container : resourceContainers) { - builder.add(container.filter(this, /* isDependency = */ true)); + builder.add(container.filter(errorConsumer, this, /* isDependency = */ true)); } return builder.build(); diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidResourcesTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidResourcesTest.java index 1f81b1300c..fdb5794913 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidResourcesTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidResourcesTest.java @@ -154,7 +154,8 @@ public class AndroidResourcesTest extends ResourceTestBase { ResourceFilter fakeFilter = ResourceFilter.of(ImmutableSet.copyOf(filteredResources), filteredDepsBuilder::add); - Optional<AndroidResources> filtered = unfiltered.maybeFilter(fakeFilter, isDependency); + Optional<AndroidResources> filtered = + unfiltered.maybeFilter(errorConsumer, fakeFilter, isDependency); if (filteredResources.equals(unfilteredResources)) { // We expect filtering to have been a no-op @@ -245,8 +246,7 @@ public class AndroidResourcesTest extends ResourceTestBase { .add(getManifest()) .build(), /* outputs = */ ImmutableList.of( - parsed.getCompiledSymbols(), - DataBinding.getSuffixedInfoFile(ruleContext, "_unused"))); + parsed.getCompiledSymbols(), DataBinding.getSuffixedInfoFile(ruleContext, "_unused"))); } /** diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterFactoryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterFactoryTest.java index 0ddd905060..0da528a31c 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterFactoryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterFactoryTest.java @@ -383,10 +383,10 @@ public class ResourceFilterFactoryTest extends ResourceTestBase { resourceFilterFactory.getResourceFilter( errorConsumer, resourceDependencies, localResources); - assertThat(localResources.filterLocalResources(filter).getResources()) + assertThat(localResources.filterLocalResources(errorConsumer, filter).getResources()) .containsExactly(localResourceToKeep); - ResourceDependencies filteredResourceDeps = resourceDependencies.filter(filter); + ResourceDependencies filteredResourceDeps = resourceDependencies.filter(errorConsumer, filter); // TODO: Remove - assert was same order before assertThat(resourceDependencies.getTransitiveResources()) @@ -500,8 +500,8 @@ public class ResourceFilterFactoryTest extends ResourceTestBase { ResourceFilter filter = resourceFilterFactory.getResourceFilter(errorConsumer, resourceDeps, localResources); - assertThat(resourceDeps.filter(filter)).isSameAs(resourceDeps); + assertThat(resourceDeps.filter(errorConsumer, filter)).isSameAs(resourceDeps); - return localResources.filterLocalResources(filter).getResources(); + return localResources.filterLocalResources(errorConsumer, filter).getResources(); } } |