diff options
9 files changed, 580 insertions, 176 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java index e2e70fb513..dafdb335a6 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java @@ -319,7 +319,7 @@ public class AndroidCommon { } static PathFragment getSourceDirectoryRelativePathFromResource(Artifact resource) { - PathFragment resourceDir = LocalResourceContainer.Builder.findResourceDir(resource); + PathFragment resourceDir = LocalResourceContainer.findResourceDir(resource); if (resourceDir == null) { return null; } 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 81e9000583..d9ce0a1516 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 @@ -31,6 +31,7 @@ import com.google.devtools.build.lib.analysis.actions.FileWriteAction; import com.google.devtools.build.lib.analysis.actions.SpawnAction; import com.google.devtools.build.lib.analysis.config.CompilationMode; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException; import com.google.devtools.build.lib.rules.android.AndroidConfiguration.AndroidManifestMerger; import com.google.devtools.build.lib.rules.android.ResourceContainer.Builder.JavaPackageSource; import com.google.devtools.build.lib.rules.android.ResourceContainer.ResourceType; @@ -387,7 +388,7 @@ public final class ApplicationManifest { List<String> uncompressedExtensions, boolean crunchPng, Artifact proguardCfg) - throws InterruptedException { + throws InterruptedException, RuleErrorException { LocalResourceContainer data = new LocalResourceContainer.Builder(ruleContext) .withAssets( @@ -441,7 +442,7 @@ public final class ApplicationManifest { @Nullable Artifact dataBindingInfoZip, @Nullable Artifact featureOf, @Nullable Artifact featureAfter) - throws InterruptedException { + throws InterruptedException, RuleErrorException { LocalResourceContainer data = new LocalResourceContainer.Builder(ruleContext) .withAssets( AndroidCommon.getAssetDir(ruleContext), @@ -492,7 +493,7 @@ public final class ApplicationManifest { Artifact manifestOut, Artifact mergedResources, Artifact dataBindingInfoZip) - throws InterruptedException { + throws InterruptedException, RuleErrorException { LocalResourceContainer data = new LocalResourceContainer.Builder(ruleContext) .withAssets( diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/LocalResourceContainer.java b/src/main/java/com/google/devtools/build/lib/rules/android/LocalResourceContainer.java index 56f8b37b38..13b610726a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/LocalResourceContainer.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/LocalResourceContainer.java @@ -25,11 +25,13 @@ import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.packages.AttributeMap; import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException; +import com.google.devtools.build.lib.packages.RuleErrorConsumer; import com.google.devtools.build.lib.rules.android.ResourceContainer.ResourceType; import com.google.devtools.build.lib.vfs.PathFragment; -import java.util.Collection; import java.util.LinkedHashSet; +import java.util.Set; +import javax.annotation.Nullable; /** * The collected resources and assets artifacts and roots. @@ -48,6 +50,28 @@ public final class LocalResourceContainer { "exports_manifest" }; + /** Set of allowable android directories prefixes. */ + public static final ImmutableSet<String> RESOURCE_DIRECTORY_TYPES = + ImmutableSet.of( + "animator", + "anim", + "color", + "drawable", + "interpolator", + "layout", + "menu", + "mipmap", + "raw", + "transition", + "values", + "xml"); + + public static final String INCORRECT_RESOURCE_LAYOUT_MESSAGE = + String.format( + "'%%s' is not in the expected resource directory structure of " + + "<resource directory>/{%s}/<file>", + Joiner.on(',').join(RESOURCE_DIRECTORY_TYPES)); + /** * Determines if the attributes contain resource and asset attributes. */ @@ -116,33 +140,12 @@ public final class LocalResourceContainer { } public static class Builder { - /** - * Set of allowable android directories prefixes. - */ - // TODO(bazel-team): Pull from AOSP constant? - public static final ImmutableSet<String> RESOURCE_DIRECTORY_TYPES = ImmutableSet.of( - "animator", - "anim", - "color", - "drawable", - "interpolator", - "layout", - "menu", - "mipmap", - "raw", - "transition", - "values", - "xml"); - - public static final String INCORRECT_RESOURCE_LAYOUT_MESSAGE = String.format( - "'%%s' is not in the expected resource directory structure of " - + "<resource directory>/{%s}/<file>", Joiner.on(',').join(RESOURCE_DIRECTORY_TYPES)); - - private RuleContext ruleContext; - private Collection<Artifact> assets = new LinkedHashSet<>(); - private Collection<Artifact> resources = new LinkedHashSet<>(); - private Collection<PathFragment> resourceRoots = new LinkedHashSet<>(); - private Collection<PathFragment> assetRoots = new LinkedHashSet<>(); + + private final RuleContext ruleContext; + private final Set<Artifact> assets = new LinkedHashSet<>(); + private final Set<Artifact> resources = new LinkedHashSet<>(); + private final Set<PathFragment> resourceRoots = new LinkedHashSet<>(); + private final Set<PathFragment> assetRoots = new LinkedHashSet<>(); public Builder(RuleContext ruleContext) { this.ruleContext = ruleContext; @@ -183,32 +186,14 @@ public final class LocalResourceContainer { * @param targets {@link FileProvider}s for a given set of resource. * @return The Builder. */ - public LocalResourceContainer.Builder withResources(Iterable<FileProvider> targets) { + public LocalResourceContainer.Builder withResources(Iterable<FileProvider> targets) + throws RuleErrorException { PathFragment lastResourceDir = null; Artifact lastFile = null; for (FileProvider target : targets) { for (Artifact file : target.getFilesToBuild()) { - PathFragment packageFragment = file.getArtifactOwner().getLabel() - .getPackageIdentifier().getSourceRoot(); - PathFragment packageRelativePath = - file.getRootRelativePath().relativeTo(packageFragment); - PathFragment resourceDir = findResourceDir(file); - if (resourceDir == null) { - ruleContext.attributeError(ResourceType.RESOURCES.getAttribute(), String.format( - INCORRECT_RESOURCE_LAYOUT_MESSAGE, file.getRootRelativePath())); - return this; - } - if (lastResourceDir == null || resourceDir.equals(lastResourceDir)) { - resourceRoots.add( - trimTail(file.getExecPath(), makeRelativeTo(resourceDir, packageRelativePath))); - } else { - ruleContext.attributeError(ResourceType.RESOURCES.getAttribute(), String.format( - "'%s' (generated by '%s') is not in the same directory '%s' (derived from %s)." - + " All resources must share a common directory.", - file.getRootRelativePath(), file.getArtifactOwner().getLabel(), lastResourceDir, - lastFile.getRootRelativePath())); - return this; - } + PathFragment resourceDir = + addResourceDir(file, lastFile, lastResourceDir, resourceRoots, ruleContext); resources.add(file); lastFile = file; lastResourceDir = resourceDir; @@ -217,66 +202,138 @@ public final class LocalResourceContainer { return this; } - /** - * Finds and validates the resource directory PathFragment from the artifact Path. - * - * <p>If the artifact is not a Fileset, the resource directory is presumed to be the second - * directory from the end. Filesets are expect to have the last directory as the resource - * directory. - * - */ - public static PathFragment findResourceDir(Artifact artifact) { - PathFragment fragment = artifact.getPath().asFragment(); - int segmentCount = fragment.segmentCount(); - if (segmentCount < 3) { - return null; - } - // TODO(bazel-team): Expand Fileset to verify, or remove Fileset as an option for resources. - if (artifact.isFileset() || artifact.isTreeArtifact()) { - return fragment.subFragment(segmentCount - 1, segmentCount); - } + public LocalResourceContainer build() { + return new LocalResourceContainer( + ImmutableList.copyOf(resources), + ImmutableList.copyOf(resourceRoots), + ImmutableList.copyOf(assets), + ImmutableList.copyOf(assetRoots)); + } + } - // Check the resource folder type layout. - // get the prefix of the parent folder of the fragment. - String parentDirectory = fragment.getSegment(segmentCount - 2); - int dashIndex = parentDirectory.indexOf('-'); - String androidFolder = - dashIndex == -1 ? parentDirectory : parentDirectory.substring(0, dashIndex); - if (!RESOURCE_DIRECTORY_TYPES.contains(androidFolder)) { - return null; - } + /** + * Gets the roots of some resources. + * + * @return a list of roots, or an empty list of the passed resources cannot all be contained in a + * single {@link LocalResourceContainer}. If that's the case, it will be reported to the + * {@link RuleErrorConsumer}. + */ + @VisibleForTesting + static ImmutableList<PathFragment> getResourceRoots( + RuleErrorConsumer ruleErrorConsumer, Iterable<Artifact> files) throws RuleErrorException { + Artifact lastFile = null; + PathFragment lastResourceDir = null; + Set<PathFragment> resourceRoots = new LinkedHashSet<>(); + for (Artifact file : files) { + PathFragment resourceDir = + addResourceDir(file, lastFile, lastResourceDir, resourceRoots, ruleErrorConsumer); + lastFile = file; + lastResourceDir = resourceDir; + } - return fragment.subFragment(segmentCount - 3, segmentCount - 2); + return ImmutableList.copyOf(resourceRoots); + } + + /** + * Inner method for adding resource roots to a collection. May fail and report to the {@link + * RuleErrorConsumer} if the input is invalid. + * + * @param file the file to add the resource directory for + * @param lastFile the last file this method was called on. May be null if this is the first call + * for this set of resources. + * @param lastResourceDir the resource directory of the last file, as returned by the most recent + * call to this method, or null if this is the first call. + * @param resourceRoots the collection to add resources to + * @param ruleErrorConsumer for reporting errors + * @return the resource root of {@code file}. + * @throws RuleErrorException if the current resource has no resource directory or if it is + * incompatible with {@code lastResourceDir}. An error will also be reported to the {@link + * RuleErrorConsumer} in this case. + */ + private static PathFragment addResourceDir( + Artifact file, + @Nullable Artifact lastFile, + @Nullable PathFragment lastResourceDir, + Set<PathFragment> resourceRoots, + RuleErrorConsumer ruleErrorConsumer) throws RuleErrorException { + PathFragment resourceDir = findResourceDir(file); + if (resourceDir == null) { + ruleErrorConsumer.attributeError( + ResourceType.RESOURCES.getAttribute(), + String.format(INCORRECT_RESOURCE_LAYOUT_MESSAGE, file.getRootRelativePath())); + throw new RuleErrorException(); } - /** - * Returns the root-part of a given path by trimming off the end specified by - * a given tail. Assumes that the tail is known to match, and simply relies on - * the segment lengths. - */ - private static PathFragment trimTail(PathFragment path, PathFragment tail) { - return path.subFragment(0, path.segmentCount() - tail.segmentCount()); + if (lastResourceDir != null && !resourceDir.equals(lastResourceDir)) { + ruleErrorConsumer.attributeError( + ResourceType.RESOURCES.getAttribute(), + String.format( + "'%s' (generated by '%s') is not in the same directory '%s' (derived from %s)." + + " All resources must share a common directory.", + file.getRootRelativePath(), + file.getArtifactOwner().getLabel(), + lastResourceDir, + lastFile.getRootRelativePath())); + throw new RuleErrorException(); } - private static PathFragment makeRelativeTo(PathFragment ancestor, PathFragment path) { - String cutAtSegment = ancestor.getSegment(ancestor.segmentCount() - 1); - int totalPathSegments = path.segmentCount() - 1; - for (int i = totalPathSegments; i >= 0; i--) { - if (path.getSegment(i).equals(cutAtSegment)) { - return path.subFragment(i, totalPathSegments); - } - } - throw new IllegalArgumentException("PathFragment " + path - + " is not beneath " + ancestor); + PathFragment packageFragment = + file.getArtifactOwner().getLabel().getPackageIdentifier().getSourceRoot(); + PathFragment packageRelativePath = file.getRootRelativePath().relativeTo(packageFragment); + resourceRoots.add( + trimTail(file.getExecPath(), makeRelativeTo(resourceDir, packageRelativePath))); + + return resourceDir; + } + + /** + * Finds and validates the resource directory PathFragment from the artifact Path. + * + * <p>If the artifact is not a Fileset, the resource directory is presumed to be the second + * directory from the end. Filesets are expect to have the last directory as the resource + * directory. + */ + public static PathFragment findResourceDir(Artifact artifact) { + PathFragment fragment = artifact.getPath().asFragment(); + int segmentCount = fragment.segmentCount(); + if (segmentCount < 3) { + return null; + } + // TODO(bazel-team): Expand Fileset to verify, or remove Fileset as an option for resources. + if (artifact.isFileset() || artifact.isTreeArtifact()) { + return fragment.subFragment(segmentCount - 1, segmentCount); } - public LocalResourceContainer build() { - return new LocalResourceContainer( - ImmutableList.copyOf(resources), - ImmutableList.copyOf(resourceRoots), - ImmutableList.copyOf(assets), - ImmutableList.copyOf(assetRoots)); + // Check the resource folder type layout. + // get the prefix of the parent folder of the fragment. + String parentDirectory = fragment.getSegment(segmentCount - 2); + int dashIndex = parentDirectory.indexOf('-'); + String androidFolder = + dashIndex == -1 ? parentDirectory : parentDirectory.substring(0, dashIndex); + if (!RESOURCE_DIRECTORY_TYPES.contains(androidFolder)) { + return null; + } + + return fragment.subFragment(segmentCount - 3, segmentCount - 2); + } + + /** + * Returns the root-part of a given path by trimming off the end specified by a given tail. + * Assumes that the tail is known to match, and simply relies on the segment lengths. + */ + private static PathFragment trimTail(PathFragment path, PathFragment tail) { + return path.subFragment(0, path.segmentCount() - tail.segmentCount()); + } + + private static PathFragment makeRelativeTo(PathFragment ancestor, PathFragment path) { + String cutAtSegment = ancestor.getSegment(ancestor.segmentCount() - 1); + int totalPathSegments = path.segmentCount() - 1; + for (int i = totalPathSegments; i >= 0; i--) { + if (path.getSegment(i).equals(cutAtSegment)) { + return path.subFragment(i, totalPathSegments); + } } + throw new IllegalArgumentException("PathFragment " + path + " is not beneath " + ancestor); } private final ImmutableList<Artifact> resources; @@ -311,4 +368,27 @@ public final class LocalResourceContainer { public ImmutableList<PathFragment> getResourceRoots() { return resourceRoots; } + + /** + * Filters this object. + * + * @return a new {@link LocalResourceContainer} with resources filtered by the passed {@link + * ResourceFilter}, or this object if no resources should be filtered. + */ + public LocalResourceContainer filter( + RuleErrorConsumer ruleErrorConsumer, ResourceFilter resourceFilter) + throws RuleErrorException { + ImmutableList<Artifact> filteredResources = resourceFilter.filter(ruleErrorConsumer, resources); + + if (filteredResources.size() == resources.size()) { + // Nothing was filtered out + return this; + } + + return new LocalResourceContainer( + filteredResources, + getResourceRoots(ruleErrorConsumer, filteredResources), + assets, + assetRoots); + } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java index 888047d871..35f940f6b5 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java @@ -2410,7 +2410,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase { checkError("java/android/resources", "r", "'java/android/resources/res/somefile.xml' is not in the expected resource directory " + "structure of <resource directory>/{" - + Joiner.on(',').join(LocalResourceContainer.Builder.RESOURCE_DIRECTORY_TYPES) + "}", + + Joiner.on(',').join(LocalResourceContainer.RESOURCE_DIRECTORY_TYPES) + "}", "android_binary(name = 'r',", " manifest = 'AndroidManifest.xml',", " resource_files = ['res/somefile.xml', 'r/t/f/m/raw/fold']", @@ -2424,7 +2424,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase { "r", "'java/android/resources/res/other/somefile.xml' is not in the expected resource directory " + "structure of <resource directory>/{" - + Joiner.on(',').join(LocalResourceContainer.Builder.RESOURCE_DIRECTORY_TYPES) + "}", + + Joiner.on(',').join(LocalResourceContainer.RESOURCE_DIRECTORY_TYPES) + "}", "android_binary(name = 'r',", " manifest = 'AndroidManifest.xml',", " resource_files = ['res/other/somefile.xml', 'r/t/f/m/raw/fold']", diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLibraryTest.java index ab4a1c1eac..b8298e2ea9 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLibraryTest.java @@ -822,7 +822,7 @@ public class AndroidLibraryTest extends AndroidBuildViewTestCase { checkError("java/android", "r", "'java/android/res/somefile.xml' is not in the expected resource directory structure of" + " <resource directory>/{" - + Joiner.on(',').join(LocalResourceContainer.Builder.RESOURCE_DIRECTORY_TYPES) + "}", + + Joiner.on(',').join(LocalResourceContainer.RESOURCE_DIRECTORY_TYPES) + "}", "android_library(name = 'r',", " manifest = 'AndroidManifest.xml',", " resource_files = ['res/somefile.xml', 'r/t/f/m/raw/fold']", @@ -834,7 +834,7 @@ public class AndroidLibraryTest extends AndroidBuildViewTestCase { checkError("java/android", "r", "'java/android/res/other/somefile.xml' is not in the expected resource directory structure" + " of <resource directory>/{" - + Joiner.on(',').join(LocalResourceContainer.Builder.RESOURCE_DIRECTORY_TYPES) + "}", + + Joiner.on(',').join(LocalResourceContainer.RESOURCE_DIRECTORY_TYPES) + "}", "android_library(name = 'r',", " manifest = 'AndroidManifest.xml',", " resource_files = ['res/other/somefile.xml', 'r/t/f/m/raw/fold']", diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/BUILD b/src/test/java/com/google/devtools/build/lib/rules/android/BUILD index 96e6b301fe..c011cde25e 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/android/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/android/BUILD @@ -77,6 +77,48 @@ java_test( ) java_test( + name = "ResourceFilterTest", + srcs = ["ResourceFilterTest.java"], + deps = [ + ":ResourceTestBase", + "//src/main/java/com/google/devtools/build/lib:android-rules", + "//src/main/java/com/google/devtools/build/lib/actions", + "//third_party:guava", + "//third_party:junit4", + "//third_party:truth", + ], +) + +java_test( + name = "LocalResourceContainerTest", + srcs = ["LocalResourceContainerTest.java"], + deps = [ + ":ResourceTestBase", + "//src/main/java/com/google/devtools/build/lib:android-rules", + "//src/main/java/com/google/devtools/build/lib:packages-internal", + "//src/main/java/com/google/devtools/build/lib:vfs", + "//src/main/java/com/google/devtools/build/lib/actions", + "//third_party:guava", + "//third_party:junit4", + "//third_party:truth", + ], +) + +java_library( + name = "ResourceTestBase", + srcs = ["ResourceTestBase.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib:inmemoryfs", + "//src/main/java/com/google/devtools/build/lib:packages-internal", + "//src/main/java/com/google/devtools/build/lib:vfs", + "//src/main/java/com/google/devtools/build/lib/actions", + "//third_party:guava", + "//third_party:junit4", + "//third_party:truth", + ], +) + +java_test( name = "AndroidDeviceScriptFixtureTest", srcs = ["AndroidDeviceScriptFixtureTest.java"], deps = [ diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/LocalResourceContainerTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/LocalResourceContainerTest.java new file mode 100644 index 0000000000..6f397bf5fb --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/rules/android/LocalResourceContainerTest.java @@ -0,0 +1,172 @@ +// Copyright 2017 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.rules.android; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException; +import com.google.devtools.build.lib.packages.RuleErrorConsumer; +import com.google.devtools.build.lib.vfs.PathFragment; +import java.util.Map; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests {@link LocalResourceContainer} */ +@RunWith(JUnit4.class) +public class LocalResourceContainerTest extends ResourceTestBase { + private static final PathFragment DEFAULT_RESOURCE_ROOT = PathFragment.create(RESOURCE_ROOT); + private static final ImmutableList<PathFragment> RESOURCES_ROOTS = + ImmutableList.of(DEFAULT_RESOURCE_ROOT); + + @Before + @Test + public void testGetResourceRootsNoResources() throws Exception { + assertThat(getResourceRoots()).isEmpty(); + } + + @Test + public void testGetResourceRootsInvalidResourceDirectory() throws Exception { + try { + getResourceRoots("is-this-drawable-or-values/foo.xml"); + assertWithMessage("Expected exception not thrown!").fail(); + } catch (RuleErrorException e) { + // expected + } + + errorConsumer.assertAttributeError( + "resources", "is not in the expected resource directory structure"); + } + + @Test + public void testGetResourceRootsMultipleRoots() throws Exception { + try { + getResourceRoots("subdir/values/foo.xml", "otherdir/values/bar.xml"); + assertWithMessage("Expected exception not thrown!").fail(); + } catch (RuleErrorException e) { + // expected + } + + errorConsumer.assertAttributeError( + "resources", "All resources must share a common directory"); + } + + @Test + public void testGetResourceRoots() throws Exception { + assertThat(getResourceRoots("values-hdpi/foo.xml", "values-mdpi/bar.xml")) + .isEqualTo(RESOURCES_ROOTS); + } + + @Test + public void testGetResourceRootsCommonSubdirectory() throws Exception { + assertThat(getResourceRoots("subdir/values-hdpi/foo.xml", "subdir/values-mdpi/bar.xml")) + .containsExactly(DEFAULT_RESOURCE_ROOT.getRelative("subdir")); + } + + private ImmutableList<PathFragment> getResourceRoots(String... pathResourceStrings) + throws Exception { + return getResourceRoots(getResources(pathResourceStrings)); + } + + private ImmutableList<PathFragment> getResourceRoots(ImmutableList<Artifact> artifacts) + throws Exception { + return LocalResourceContainer.getResourceRoots(errorConsumer, artifacts); + } + + @Test + public void testFilterEmpty() throws Exception { + assertFilter(ImmutableList.<Artifact>of(), ImmutableList.<Artifact>of()); + } + + @Test + public void testFilterNoop() throws Exception { + ImmutableList<Artifact> resources = getResources("values-en/foo.xml", "values-es/bar.xml"); + assertFilter(resources, resources); + } + + @Test + public void testFilterToEmpty() throws Exception { + assertFilter( + getResources("values-en/foo.xml", "values-es/bar.xml"), ImmutableList.<Artifact>of()); + } + + @Test + public void testPartiallyFilter() throws Exception { + Artifact keptResource = getResource("values-en/foo.xml"); + assertFilter( + ImmutableList.of(keptResource, getResource("values-es/bar.xml")), + ImmutableList.of(keptResource)); + } + + private void assertFilter( + ImmutableList<Artifact> unfilteredResources, ImmutableList<Artifact> filteredResources) + throws Exception { + ResourceFilter filter = + new FakeResourceFilter(ImmutableMap.of(unfilteredResources, filteredResources)); + ImmutableList<PathFragment> unfilteredResourcesRoots = getResourceRoots(unfilteredResources); + LocalResourceContainer unfiltered = + new LocalResourceContainer( + unfilteredResources, + unfilteredResourcesRoots, + unfilteredResources, + unfilteredResourcesRoots); + + LocalResourceContainer filtered = unfiltered.filter(errorConsumer, filter); + + if (unfilteredResources.equals(filteredResources)) { + // The filtering was a no-op; the original object, not a copy, should be returned + assertThat(filtered).isSameAs(unfiltered); + } else { + // The resources and their roots should be filtered + assertThat(filtered.getResources()).containsExactlyElementsIn(filteredResources).inOrder(); + assertThat(filtered.getResourceRoots()) + .containsExactlyElementsIn(getResourceRoots(filteredResources)) + .inOrder(); + + // The assets and their roots should not be filtered; the original objects, not a copy, should + // be returned. + assertThat(filtered.getAssets()).isSameAs(unfiltered.getAssets()); + assertThat(filtered.getAssetRoots()).isSameAs(unfiltered.getAssetRoots()); + } + } + + private static class FakeResourceFilter extends ResourceFilter { + private final Map<ImmutableList<Artifact>, ImmutableList<Artifact>> filterInputToOutputMap; + + FakeResourceFilter( + Map<ImmutableList<Artifact>, ImmutableList<Artifact>> filterInputToOutputMap) { + super( + ImmutableList.<String>of(), + ImmutableList.<String>of(), + FilterBehavior.FILTER_IN_ANALYSIS); + this.filterInputToOutputMap = filterInputToOutputMap; + } + + @Override + public ImmutableList<Artifact> filter( + RuleErrorConsumer errorConsumer, ImmutableList<Artifact> artifacts) { + if (filterInputToOutputMap.containsKey(artifacts)) { + return filterInputToOutputMap.get(artifacts); + } + + assertWithMessage("Called with unexpected input: " + artifacts).fail(); + return artifacts; + } + } +} diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterTest.java index 8dbb18df72..b7aaf7d1d0 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterTest.java @@ -14,22 +14,13 @@ package com.google.devtools.build.lib.rules.android; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth.assertWithMessage; -import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; -import com.google.common.collect.Multimap; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.Root; -import com.google.devtools.build.lib.packages.RuleErrorConsumer; import com.google.devtools.build.lib.rules.android.ResourceFilter.FilterBehavior; -import com.google.devtools.build.lib.vfs.FileSystem; -import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import java.util.ArrayList; -import java.util.Collection; import java.util.List; -import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -38,53 +29,7 @@ import org.junit.runners.JUnit4; // TODO(asteinb): Test behavior not already covered in this test, and, when practical, move unit // tests currently located in {@link AndroidBinaryTest} to this class instead. @RunWith(JUnit4.class) -public class ResourceFilterTest { - private FakeRuleErrorConsumer errorConsumer; - - @Before - public void setup() { - errorConsumer = new FakeRuleErrorConsumer(); - } - - private static final class FakeRuleErrorConsumer implements RuleErrorConsumer { - // Use an ArrayListMultimap since it allows duplicates - we'll want to know if a warning is - // reported twice. - private final Multimap<String, String> attributeWarnings = ArrayListMultimap.create(); - - @Override - public void ruleWarning(String message) {} - - @Override - public void ruleError(String message) { - assertWithMessage(message).fail(); - } - - @Override - public void attributeWarning(String attrName, String message) { - attributeWarnings.put(attrName, message); - } - - @Override - public void attributeError(String attrName, String message) { - assertWithMessage(message + " (attribute: " + attrName + ")").fail(); - } - - public Collection<String> getAndClearAttributeWarnings(String attrName) { - if (!attributeWarnings.containsKey(attrName)) { - return ImmutableList.of(); - } - - return attributeWarnings.removeAll(attrName); - } - - public void assertNoAttributeWarnings(String attrName) { - assertThat(attributeWarnings).doesNotContainKey(attrName); - } - }; - - private static final FileSystem FILE_SYSTEM = new InMemoryFileSystem(); - private static final Root ROOT = Root.asSourceRoot(FILE_SYSTEM.getRootDirectory()); - +public class ResourceFilterTest extends ResourceTestBase { @Test public void testFilterInExecution() { testNoopFilter( @@ -98,10 +43,10 @@ public class ResourceFilterTest { @Test public void testFilterEmpty() { testNoopFilter( - ImmutableList.of(), - ImmutableList.of(), + ImmutableList.<String>of(), + ImmutableList.<String>of(), FilterBehavior.FILTER_IN_ANALYSIS, - ImmutableList.of()); + ImmutableList.<String>of()); } @Test @@ -128,7 +73,7 @@ public class ResourceFilterTest { @Test public void testFilterByDensityPersistsOrdering() { testFilter( - ImmutableList.of(), + ImmutableList.<String>of(), ImmutableList.of("hdpi", "ldpi"), FilterBehavior.FILTER_IN_ANALYSIS, // If we add resources to the output list in density order, these resources will be @@ -306,7 +251,11 @@ public class ResourceFilterTest { FilterBehavior filterBehavior, List<String> resources) { testFilter( - resourceConfigurationFilters, densities, filterBehavior, resources, ImmutableList.of()); + resourceConfigurationFilters, + densities, + filterBehavior, + resources, + ImmutableList.<String>of()); } private void testFilter( @@ -317,12 +266,12 @@ public class ResourceFilterTest { List<String> resourcesToDiscard) { List<Artifact> unexpectedResources = new ArrayList<>(); for (String resource : resourcesToDiscard) { - unexpectedResources.add(getArtifact(resource)); + unexpectedResources.add(getResource(resource)); } List<Artifact> expectedResources = new ArrayList<>(); for (String resource : resourcesToKeep) { - expectedResources.add(getArtifact(resource)); + expectedResources.add(getResource(resource)); } ImmutableList<Artifact> allArtifacts = @@ -333,8 +282,4 @@ public class ResourceFilterTest { assertThat(filtered).containsExactlyElementsIn(expectedResources).inOrder(); } - - private static Artifact getArtifact(String pathString) { - return new Artifact(FILE_SYSTEM.getPath("/java/android/res/" + pathString), ROOT); - } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/ResourceTestBase.java b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceTestBase.java new file mode 100644 index 0000000000..cada5518d9 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceTestBase.java @@ -0,0 +1,164 @@ +// Copyright 2017 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.rules.android; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; + +import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Multimap; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.ArtifactOwner; +import com.google.devtools.build.lib.actions.Root; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.LabelSyntaxException; +import com.google.devtools.build.lib.packages.RuleErrorConsumer; +import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import java.util.Collection; +import org.junit.After; +import org.junit.Before; + +/** Base class for tests that work with resource artifacts. */ +public abstract class ResourceTestBase { + public static final String RESOURCE_ROOT = "java/android/res"; + + private static final ArtifactOwner OWNER = () -> { + try { + return Label.create("java", "all"); + } catch (LabelSyntaxException e) { + assertWithMessage(e.getMessage()).fail(); + return null; + } + }; + + /** A faked {@link RuleErrorConsumer} that validates that only expected errors were reported. */ + public static final class FakeRuleErrorConsumer implements RuleErrorConsumer { + private String ruleErrorMessage = null; + private String attributeErrorAttribute = null; + private String attributeErrorMessage = null; + + // Use an ArrayListMultimap since it allows duplicates - we'll want to know if a warning is + // reported twice. + private final Multimap<String, String> attributeWarnings = ArrayListMultimap.create(); + + @Override + public void ruleWarning(String message) {} + + @Override + public void ruleError(String message) { + ruleErrorMessage = message; + } + + @Override + public void attributeWarning(String attrName, String message) { + attributeWarnings.put(attrName, message); + } + + @Override + public void attributeError(String attrName, String message) { + attributeErrorAttribute = attrName; + attributeErrorMessage = message; + } + + public Collection<String> getAndClearAttributeWarnings(String attrName) { + if (!attributeWarnings.containsKey(attrName)) { + return ImmutableList.of(); + } + + return attributeWarnings.removeAll(attrName); + } + + public void assertNoAttributeWarnings(String attrName) { + assertThat(attributeWarnings).doesNotContainKey(attrName); + } + + /** + * Called at the end of a test to assert that that test produced a rule error + * + * @param expectedMessage a substring of the expected message + */ + public void assertRuleError(String expectedMessage) { + // Clear the message before asserting so that if we fail here the error is not masked by the + // @After call to assertNoUnexpectedErrors. + + String message = ruleErrorMessage; + ruleErrorMessage = null; + + assertThat(message).contains(expectedMessage); + } + + /** + * Called at the end of a test to assert that that test produced an attribute error + * + * @param expectedAttribute the attribute that caused the error + * @param expectedMessage a substring of the expected message + */ + public void assertAttributeError(String expectedAttribute, String expectedMessage) { + // Clear the message before asserting so that if we fail here the error is not masked by the + // @After call to assertNoUnexpectedErrors. + String attr = attributeErrorAttribute; + String message = attributeErrorMessage; + attributeErrorAttribute = null; + attributeErrorMessage = null; + + assertThat(message).contains(expectedMessage); + assertThat(attr).isEqualTo(expectedAttribute); + } + + /** + * Asserts this {@link RuleErrorConsumer} encountered no unexpected errors. To consume an + * expected error, call {@link #assertRuleError(String)} or {@link #assertAttributeError(String, + * String)} in your test after the error is produced. + */ + private void assertNoUnexpectedErrors() { + assertThat(ruleErrorMessage).isNull(); + assertThat(attributeErrorMessage).isNull(); + assertThat(attributeErrorAttribute).isNull(); + } + }; + + public FakeRuleErrorConsumer errorConsumer; + public FileSystem fileSystem; + public Root root; + + @Before + public void setup() { + errorConsumer = new FakeRuleErrorConsumer(); + fileSystem = new InMemoryFileSystem(); + root = Root.asDerivedRoot(fileSystem.getRootDirectory()); + } + + @After + public void assertNoErrors() { + errorConsumer.assertNoUnexpectedErrors(); + } + + public ImmutableList<Artifact> getResources(String... pathStrings) { + ImmutableList.Builder<Artifact> builder = ImmutableList.builder(); + for (String pathString : pathStrings) { + builder.add(getResource(pathString)); + } + + return builder.build(); + } + + public Artifact getResource(String pathString) { + Path path = fileSystem.getPath("/" + RESOURCE_ROOT + "/" + pathString); + return new Artifact( + path, root, root.getExecPath().getRelative(path.relativeTo(root.getPath())), OWNER); + } +} |