diff options
11 files changed, 115 insertions, 77 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/RClassGeneratorActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/android/RClassGeneratorActionBuilder.java index 49ef64770c..0c40818e16 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/RClassGeneratorActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/RClassGeneratorActionBuilder.java @@ -100,9 +100,9 @@ public class RClassGeneratorActionBuilder { if (dependencies != null) { // TODO(corysmith): Remove NestedSet as we are already flattening it. Iterable<ResourceContainer> depResources = dependencies.getResources(); - if (depResources.iterator().hasNext()) { - builder.addJoinStrings( - "--libraries", ",", Iterables.transform(depResources, chooseDepsToArg(version))); + if (!Iterables.isEmpty(depResources)) { + builder.addBeforeEach( + "--library", Iterables.transform(depResources, chooseDepsToArg(version))); inputs.addTransitive( NestedSetBuilder.wrap( Order.NAIVE_LINK_ORDER, @@ -159,7 +159,7 @@ public class RClassGeneratorActionBuilder { public String apply(ResourceContainer container) { Artifact rTxt = chooseRTxt(container, version); return (rTxt != null ? rTxt.getExecPath() : "") - + ":" + + "," + (container.getManifest() != null ? container.getManifest().getExecPath() : ""); } }; diff --git a/src/test/java/com/google/devtools/build/android/ConvertersTest.java b/src/test/java/com/google/devtools/build/android/ConvertersTest.java index 937435da67..4e8f7058c1 100644 --- a/src/test/java/com/google/devtools/build/android/ConvertersTest.java +++ b/src/test/java/com/google/devtools/build/android/ConvertersTest.java @@ -21,10 +21,8 @@ import com.google.devtools.build.android.Converters.PathConverter; import com.google.devtools.build.android.Converters.PathListConverter; import com.google.devtools.build.android.Converters.StringDictionaryConverter; import com.google.devtools.common.options.OptionsParsingException; -import java.io.File; import java.nio.file.Path; import java.nio.file.Paths; -import java.util.List; import java.util.Map; import org.junit.Rule; import org.junit.Test; @@ -39,8 +37,6 @@ import org.junit.runners.JUnit4; @RunWith(JUnit4.class) public final class ConvertersTest { - private static final String SEPARATOR = File.pathSeparator; - @Rule public final TemporaryFolder tmp = new TemporaryFolder(); @Rule @@ -89,9 +85,7 @@ public final class ConvertersTest { @Test public void testPathListConverter() throws Exception { PathListConverter converter = new PathListConverter(); - List<Path> result = - converter.convert("foo" + SEPARATOR + "bar" + SEPARATOR + SEPARATOR + "baz" + SEPARATOR); - assertThat(result) + assertThat(converter.convert("foo:bar::baz:")) .containsAllOf(Paths.get("foo"), Paths.get("bar"), Paths.get("baz")) .inOrder(); } diff --git a/src/test/java/com/google/devtools/build/android/RClassGeneratorActionTest.java b/src/test/java/com/google/devtools/build/android/RClassGeneratorActionTest.java index e18f466060..e03348cdaa 100644 --- a/src/test/java/com/google/devtools/build/android/RClassGeneratorActionTest.java +++ b/src/test/java/com/google/devtools/build/android/RClassGeneratorActionTest.java @@ -19,7 +19,6 @@ import com.google.common.base.Function; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; -import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; @@ -111,20 +110,16 @@ public class RClassGeneratorActionTest { RClassGeneratorAction.main( ImmutableList.<String>of( - "--primaryRTxt", - binarySymbols.toString(), - "--primaryManifest", - binaryManifest.toString(), - "--libraries", - libFooSymbols - + File.pathSeparator - + libFooManifest - + "," - + libBarSymbols - + File.pathSeparator - + libBarManifest, - "--classJarOutput", - jarPath.toString()) + "--primaryRTxt", + binarySymbols.toString(), + "--primaryManifest", + binaryManifest.toString(), + "--library", + libFooSymbols + "," + libFooManifest, + "--library", + libBarSymbols + "," + libBarManifest, + "--classJarOutput", + jarPath.toString()) .toArray(new String[0])); assertThat(Files.exists(jarPath)).isTrue(); @@ -171,14 +166,10 @@ public class RClassGeneratorActionTest { RClassGeneratorAction.main( ImmutableList.<String>of( - "--libraries", - libFooSymbols - + File.pathSeparator - + libFooManifest - + "," - + libBarSymbols - + File.pathSeparator - + libBarManifest, + "--library", + libFooSymbols + "," + libFooManifest, + "--library", + libBarSymbols + "," + libBarManifest, "--classJarOutput", jarPath.toString()) .toArray(new String[0])); @@ -281,15 +272,16 @@ public class RClassGeneratorActionTest { Path jarPath = tempDir.resolve("app_resources.jar"); RClassGeneratorAction.main( ImmutableList.<String>of( - "--primaryRTxt", - binarySymbols.toString(), - "--primaryManifest", - binaryManifest.toString(), - "--packageForR", "com.custom.er", - "--libraries", - libFooSymbols + File.pathSeparator + libFooManifest, - "--classJarOutput", - jarPath.toString()) + "--primaryRTxt", + binarySymbols.toString(), + "--primaryManifest", + binaryManifest.toString(), + "--packageForR", + "com.custom.er", + "--library", + libFooSymbols + "," + libFooManifest, + "--classJarOutput", + jarPath.toString()) .toArray(new String[0])); assertThat(Files.exists(jarPath)).isTrue(); 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 62c5057d8e..f305c3efa1 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 @@ -1810,7 +1810,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase { assertThat(compilerAction.getMnemonic()).isEqualTo("RClassGenerator"); List<String> args = compilerAction.getArguments(); assertThat(args) - .containsAllOf("--primaryRTxt", "--primaryManifest", "--libraries", "--classJarOutput"); + .containsAllOf("--primaryRTxt", "--primaryManifest", "--library", "--classJarOutput"); } @Test @@ -1853,7 +1853,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase { assertThat(compilerAction.getMnemonic()).isEqualTo("RClassGenerator"); List<String> args = compilerAction.getArguments(); assertThat(args) - .containsAllOf("--primaryRTxt", "--primaryManifest", "--libraries", "--classJarOutput", + .containsAllOf("--primaryRTxt", "--primaryManifest", "--library", "--classJarOutput", "--packageForR", "com.binary.custom"); } diff --git a/src/tools/android/java/com/google/devtools/build/android/AndroidResourceProcessor.java b/src/tools/android/java/com/google/devtools/build/android/AndroidResourceProcessor.java index 65092555d0..68eed92c55 100644 --- a/src/tools/android/java/com/google/devtools/build/android/AndroidResourceProcessor.java +++ b/src/tools/android/java/com/google/devtools/build/android/AndroidResourceProcessor.java @@ -554,7 +554,7 @@ public class AndroidResourceProcessor { } public ResourceSymbols loadResourceSymbolTable( - List<SymbolFileProvider> libraries, + Iterable<SymbolFileProvider> libraries, String appPackageName, Path primaryRTxt, Multimap<String, ResourceSymbols> libMap) diff --git a/src/tools/android/java/com/google/devtools/build/android/Converters.java b/src/tools/android/java/com/google/devtools/build/android/Converters.java index 80bf6652ad..f991a076c6 100644 --- a/src/tools/android/java/com/google/devtools/build/android/Converters.java +++ b/src/tools/android/java/com/google/devtools/build/android/Converters.java @@ -23,7 +23,6 @@ import com.google.common.collect.Iterables; import com.google.devtools.common.options.Converter; import com.google.devtools.common.options.EnumConverter; import com.google.devtools.common.options.OptionsParsingException; -import java.io.File; import java.lang.reflect.ParameterizedType; import java.nio.file.FileSystems; import java.nio.file.Files; @@ -177,10 +176,36 @@ public final class Converters { } } + /** Converter for a single {@link DependencySymbolFileProvider}. */ + public static class DependencySymbolFileProviderConverter + implements Converter<DependencySymbolFileProvider> { + + @Override + public DependencySymbolFileProvider convert(String input) throws OptionsParsingException { + try { + return DependencySymbolFileProvider.valueOf(input); + } catch (IllegalArgumentException e) { + throw new OptionsParsingException( + String.format("invalid DependencyAndroidData: %s", e.getMessage()), e); + } + } + + @Override + public String getTypeDescription() { + return String.format( + "a dependency android data in the format: %s[%s]", + DependencySymbolFileProvider.commandlineFormat("1"), + DependencySymbolFileProvider.commandlineFormat("2")); + } + } + /** * Converter for a list of {@link DependencySymbolFileProvider}. Relies on * {@code DependencySymbolFileProvider#valueOf(String)} to perform conversion and validation. + * + * @deprecated use multi-value flags and {@link DependencySymbolFileProviderConverter} instead. */ + @Deprecated public static class DependencySymbolFileProviderListConverter implements Converter<List<DependencySymbolFileProvider>> { @@ -288,6 +313,16 @@ public final class Converters { } } + public static <T> List<T> concatLists( + @Nullable List<? extends T> a, @Nullable List<? extends T> b) { + @SuppressWarnings("unchecked") List<T> la = (List<T>) a; + @SuppressWarnings("unchecked") List<T> lb = (List<T>) b; + if (la == null || la.isEmpty()) { + return (lb == null || lb.isEmpty()) ? ImmutableList.of() : lb; + } + return (lb == null || lb.isEmpty()) ? la : ImmutableList.copyOf(Iterables.concat(la, lb)); + } + /** * Validating converter for a list of Paths. * A Path is considered valid if it resolves to a file. @@ -295,13 +330,6 @@ public final class Converters { @Deprecated public static class PathListConverter implements Converter<List<Path>> { - public static List<Path> concatLists(@Nullable List<Path> a, @Nullable List<Path> b) { - if (a == null || a.isEmpty()) { - return (b == null || b.isEmpty()) ? ImmutableList.of() : b; - } - return (b == null || b.isEmpty()) ? a : ImmutableList.copyOf(Iterables.concat(a, b)); - } - private final PathConverter baseConverter; public PathListConverter() { @@ -315,7 +343,7 @@ public final class Converters { @Override public List<Path> convert(String input) throws OptionsParsingException { List<Path> list = new ArrayList<>(); - for (String piece : input.split(File.pathSeparator)) { + for (String piece : input.split(":")) { if (!piece.isEmpty()) { list.add(baseConverter.convert(piece)); } diff --git a/src/tools/android/java/com/google/devtools/build/android/DependencySymbolFileProvider.java b/src/tools/android/java/com/google/devtools/build/android/DependencySymbolFileProvider.java index 871dd7f16c..b6f86bd636 100644 --- a/src/tools/android/java/com/google/devtools/build/android/DependencySymbolFileProvider.java +++ b/src/tools/android/java/com/google/devtools/build/android/DependencySymbolFileProvider.java @@ -20,15 +20,12 @@ import java.nio.file.FileSystem; import java.nio.file.FileSystems; import java.nio.file.Files; import java.nio.file.Path; -import java.util.regex.Pattern; /** * Represents the R.txt symbol file and AndroidManifest (provides Java package) of libraries. */ class DependencySymbolFileProvider implements SymbolFileProvider { - private static final Pattern VALID_REGEX = Pattern.compile(".*:.*"); - private final File symbolFile; private final File manifest; @@ -57,12 +54,21 @@ class DependencySymbolFileProvider implements SymbolFileProvider { } private static DependencySymbolFileProvider valueOf(String text, FileSystem fileSystem) { - if (!VALID_REGEX.matcher(text).find()) { + int separatorIndex = text.indexOf(','); + if (separatorIndex == -1) { + // TODO(laszlocsomor): remove support for ":" after 2018-02-28 (about 6 months from now). + // Everyone should have updated to newer Bazel versions by then. + // ":" is supported for the sake of the deprecated --libraries flag whose format is + // --libraries=key1:value1,key2:value2,keyN:valueN . The new flag is --library with + // multi-value support and expected format: --library=key1,value1 --library=key2,value2 . + separatorIndex = text.indexOf(':'); + } + if (separatorIndex == -1) { throw new IllegalArgumentException(text + " is not in the format " + commandlineFormat("")); } - String[] parts = text.split(File.pathSeparator); - return new DependencySymbolFileProvider(getFile(parts[0], fileSystem), - getFile(parts[1], fileSystem)); + return new DependencySymbolFileProvider( + getFile(text.substring(0, separatorIndex), fileSystem), + getFile(text.substring(separatorIndex + 1), fileSystem)); } private static File getFile(String pathString, FileSystem fileSystem) { @@ -78,7 +84,7 @@ class DependencySymbolFileProvider implements SymbolFileProvider { } public static String commandlineFormat(String libNum) { - return String.format("lib%s/R.txt:lib%s/AndroidManifest.xml", libNum, libNum); + return String.format("lib%s/R.txt,lib%s/AndroidManifest.xml", libNum, libNum); } @Override diff --git a/src/tools/android/java/com/google/devtools/build/android/LibraryRClassGeneratorAction.java b/src/tools/android/java/com/google/devtools/build/android/LibraryRClassGeneratorAction.java index 9709583dda..fef8a94ce8 100644 --- a/src/tools/android/java/com/google/devtools/build/android/LibraryRClassGeneratorAction.java +++ b/src/tools/android/java/com/google/devtools/build/android/LibraryRClassGeneratorAction.java @@ -105,7 +105,7 @@ public class LibraryRClassGeneratorAction { optionsParser.parseAndExitUponError(args); AaptConfigOptions aaptConfigOptions = optionsParser.getOptions(AaptConfigOptions.class); Options options = optionsParser.getOptions(Options.class); - options.symbols = PathListConverter.concatLists(options.symbols, options.deprecatedSymbols); + options.symbols = Converters.concatLists(options.symbols, options.deprecatedSymbols); logger.fine( String.format("Option parsing finished at %sms", timer.elapsed(TimeUnit.MILLISECONDS))); try (ScopedTemporaryDirectory scopedTmp = diff --git a/src/tools/android/java/com/google/devtools/build/android/RClassGeneratorAction.java b/src/tools/android/java/com/google/devtools/build/android/RClassGeneratorAction.java index ee64b76934..daab1f4b3b 100644 --- a/src/tools/android/java/com/google/devtools/build/android/RClassGeneratorAction.java +++ b/src/tools/android/java/com/google/devtools/build/android/RClassGeneratorAction.java @@ -20,18 +20,19 @@ import com.google.common.base.Preconditions; import com.google.common.base.Stopwatch; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.Multimap; +import com.google.devtools.build.android.Converters.DependencySymbolFileProviderConverter; import com.google.devtools.build.android.Converters.DependencySymbolFileProviderListConverter; import com.google.devtools.build.android.Converters.PathConverter; import com.google.devtools.build.android.resources.ResourceSymbols; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; +import com.google.devtools.common.options.OptionMetadataTag; import com.google.devtools.common.options.OptionsBase; import com.google.devtools.common.options.OptionsParser; import java.nio.file.FileSystems; import java.nio.file.Files; import java.nio.file.Path; -import java.util.ArrayList; import java.util.List; import java.util.concurrent.TimeUnit; import java.util.logging.Logger; @@ -40,15 +41,15 @@ import java.util.logging.Logger; * Provides an entry point for the compiling resource classes using a custom compiler (simply parse * R.txt and make a jar, which is simpler than parsing R.java and running errorprone, etc.). * - * For now, we assume this is only worthwhile for android_binary and not libraries. + * <p>For now, we assume this is only worthwhile for android_binary and not libraries. * * <pre> * Example Usage: * java/com/google/build/android/RClassGeneratorAction\ * --primaryRTxt path/to/R.txt\ * --primaryManifest path/to/AndroidManifest.xml\ - * --libraries p/t/1/AndroidManifest.txt:p/t/1/R.txt,\ - * p/t/2/AndroidManifest.txt:p/t/2/R.txt\ + * --library p/t/1/AndroidManifest.txt,p/t/1/R.txt\ + * --library p/t/2/AndroidManifest.txt,p/t/2/R.txt\ * --classJarOutput path/to/write/archive_resources.jar * </pre> */ @@ -97,9 +98,10 @@ public class RClassGeneratorAction { public String packageForR; @Option( - name = "libraries", + name = "library", + allowMultiple = true, defaultValue = "", - converter = DependencySymbolFileProviderListConverter.class, + converter = DependencySymbolFileProviderConverter.class, category = "input", documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, effectTags = {OptionEffectTag.UNKNOWN}, @@ -109,6 +111,24 @@ public class RClassGeneratorAction { ) public List<DependencySymbolFileProvider> libraries; + // TODO(laszlocsomor): remove this flag after 2018-02-28 (about 6 months from now). Everyone + // should have updated to newer Bazel versions by then. + @Deprecated + @Option( + name = "libraries", + defaultValue = "", + deprecationWarning = "Deprecated in favour of \"--library\"", + converter = DependencySymbolFileProviderListConverter.class, + category = "input", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + help = + "R.txt and manifests for the libraries in this binary's deps. We will write " + + "class files for the libraries as well. Expected format: lib1/R.txt[:lib2/R.txt]", + metadataTags = {OptionMetadataTag.DEPRECATED} + ) + public List<DependencySymbolFileProvider> deprecatedLibraries; + @Option( name = "classJarOutput", defaultValue = "null", @@ -135,10 +155,8 @@ public class RClassGeneratorAction { Path classOutPath = tmp.resolve("compiled_classes"); logger.fine(String.format("Setup finished at %sms", timer.elapsed(TimeUnit.MILLISECONDS))); - List<SymbolFileProvider> libraries = new ArrayList<>(); - for (DependencySymbolFileProvider library : options.libraries) { - libraries.add(library); - } + List<SymbolFileProvider> libraries = + Converters.concatLists(options.libraries, options.deprecatedLibraries); // Note that we need to write the R class for the main binary (so proceed even if there // are no libraries). if (options.primaryRTxt != null) { diff --git a/src/tools/android/java/com/google/devtools/build/android/ResourceShrinkerAction.java b/src/tools/android/java/com/google/devtools/build/android/ResourceShrinkerAction.java index dbf10163ac..f8cd48722e 100644 --- a/src/tools/android/java/com/google/devtools/build/android/ResourceShrinkerAction.java +++ b/src/tools/android/java/com/google/devtools/build/android/ResourceShrinkerAction.java @@ -254,8 +254,8 @@ public class ResourceShrinkerAction { optionsParser.parseAndExitUponError(args); aaptConfigOptions = optionsParser.getOptions(AaptConfigOptions.class); options = optionsParser.getOptions(Options.class); - options.dependencyManifests = PathListConverter.concatLists( - options.dependencyManifests, options.deprecatedDependencyManifests); + options.dependencyManifests = + Converters.concatLists(options.dependencyManifests, options.deprecatedDependencyManifests); AndroidResourceProcessor resourceProcessor = new AndroidResourceProcessor(stdLogger); // Setup temporary working directories. diff --git a/src/tools/android/java/com/google/devtools/build/android/resources/ResourceSymbols.java b/src/tools/android/java/com/google/devtools/build/android/resources/ResourceSymbols.java index b98dfefa64..29ca09281a 100644 --- a/src/tools/android/java/com/google/devtools/build/android/resources/ResourceSymbols.java +++ b/src/tools/android/java/com/google/devtools/build/android/resources/ResourceSymbols.java @@ -125,7 +125,7 @@ public class ResourceSymbols { * @throws InterruptedException when there is an error loading the symbols. */ public static Multimap<String, ListenableFuture<ResourceSymbols>> loadFrom( - Collection<SymbolFileProvider> dependencies, + Iterable<SymbolFileProvider> dependencies, ListeningExecutorService executor, @Nullable String packageToExclude) throws InterruptedException, ExecutionException { |