Migrate all actions to JCommander

PiperOrigin-RevId: 667604461 Change-Id: I740e31cd4d96ea6d874badd3d91de0dda0442e27
iancha1992 · Aug 26, 2024 · 05013c1 · 05013c1
1 parent ba23690
commit 05013c1
Show file tree

Hide file tree

Showing 23 changed files with 616 additions and 829 deletions.
diff --git a/src/test/java/com/google/devtools/build/android/AarGeneratorActionTest.java b/src/test/java/com/google/devtools/build/android/AarGeneratorActionTest.java
@@ -19,12 +19,12 @@
 import static org.junit.Assert.assertNotNull;
 
 import com.android.builder.core.VariantTypeImpl;
+import com.beust.jcommander.JCommander;
+import com.beust.jcommander.ParameterException;
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.android.AarGeneratorAction.AarGeneratorOptions;
 import com.google.devtools.build.zip.ZipReader;
-import com.google.devtools.common.options.OptionsParser;
-import com.google.devtools.common.options.OptionsParsingException;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import java.io.BufferedReader;
 import java.io.ByteArrayOutputStream;
@@ -309,7 +309,20 @@ private Set<Long> getZipEntryTimestamps(Path zip) throws IOException {
 
   }
 
-  @Test public void testCheckFlags() throws IOException, OptionsParsingException {
+  private AarGeneratorOptions parseFlags(String[] args)
+      throws CompatOptionsParsingException, ParameterException {
+    AarGeneratorOptions options = new AarGeneratorOptions();
+    JCommander jc = new JCommander(options);
+    String[] preprocessedArgs = AndroidOptionsUtils.runArgFilePreprocessor(jc, args);
+    String[] normalizedArgs =
+        AndroidOptionsUtils.normalizeBooleanOptions(options, preprocessedArgs);
+    jc.parse(normalizedArgs);
+    return options;
+  }
+
+  @Test
+  public void testCheckFlags()
+      throws CompatOptionsParsingException, IOException, ParameterException {
     Path manifest = tempDir.resolve("AndroidManifest.xml");
     Files.createFile(manifest);
     Path rtxt = tempDir.resolve("R.txt");
@@ -319,38 +332,33 @@ private Set<Long> getZipEntryTimestamps(Path zip) throws IOException {
 
     String[] args = new String[] {"--manifest", manifest.toString(), "--rtxt", rtxt.toString(),
         "--classes", classes.toString()};
-    OptionsParser optionsParser =
-        OptionsParser.builder().optionsClasses(AarGeneratorOptions.class).build();
-    optionsParser.parse(args);
-    AarGeneratorOptions options = optionsParser.getOptions(AarGeneratorOptions.class);
+    AarGeneratorOptions options = parseFlags(args);
     AarGeneratorAction.checkFlags(options);
   }
 
-  @Test public void testCheckFlags_MissingClasses() throws IOException, OptionsParsingException {
+  @Test
+  public void testCheckFlags_MissingClasses()
+      throws CompatOptionsParsingException, IOException, ParameterException {
     Path manifest = tempDir.resolve("AndroidManifest.xml");
     Files.createFile(manifest);
     Path rtxt = tempDir.resolve("R.txt");
     Files.createFile(rtxt);
 
     String[] args = new String[] {"--manifest", manifest.toString(), "--rtxt", rtxt.toString()};
-    OptionsParser optionsParser =
-        OptionsParser.builder().optionsClasses(AarGeneratorOptions.class).build();
-    optionsParser.parse(args);
-    AarGeneratorOptions options = optionsParser.getOptions(AarGeneratorOptions.class);
+    AarGeneratorOptions options = parseFlags(args);
     thrown.expect(IllegalArgumentException.class);
     thrown.expectMessage("classes must be specified. Building an .aar without"
           + " classes is unsupported.");
     AarGeneratorAction.checkFlags(options);
   }
 
-  @Test public void testCheckFlags_MissingMultiple() throws IOException, OptionsParsingException {
+  @Test
+  public void testCheckFlags_MissingMultiple()
+      throws CompatOptionsParsingException, IOException, ParameterException {
     Path manifest = tempDir.resolve("AndroidManifest.xml");
     Files.createFile(manifest);
     String[] args = new String[] {"--manifest", manifest.toString()};
-    OptionsParser optionsParser =
-        OptionsParser.builder().optionsClasses(AarGeneratorOptions.class).build();
-    optionsParser.parse(args);
-    AarGeneratorOptions options = optionsParser.getOptions(AarGeneratorOptions.class);
+    AarGeneratorOptions options = parseFlags(args);
     thrown.expect(IllegalArgumentException.class);
     thrown.expectMessage("rtxt, classes must be specified. Building an .aar without"
           + " rtxt, classes is unsupported.");

diff --git a/src/test/java/com/google/devtools/build/android/BUILD b/src/test/java/com/google/devtools/build/android/BUILD
@@ -29,6 +29,7 @@ java_test(
         "//third_party:guava",
         "//third_party:junit4",
         "//third_party:truth",
+        "//third_party/java/jcommander",
     ],
 )
 

diff --git a/src/tools/android/java/com/google/devtools/build/android/Aapt2OptimizeAction.java b/src/tools/android/java/com/google/devtools/build/android/Aapt2OptimizeAction.java
@@ -13,12 +13,11 @@
 // limitations under the License.
 package com.google.devtools.build.android;
 
-
+import com.beust.jcommander.JCommander;
+import com.beust.jcommander.Parameters;
+import com.beust.jcommander.ParametersDelegate;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.android.aapt2.Aapt2ConfigOptions;
-import com.google.devtools.common.options.OptionsParser;
-import com.google.devtools.common.options.ShellQuotedParamsFilePreProcessor;
-import java.nio.file.FileSystems;
 import java.util.List;
 import java.util.logging.Logger;
 
@@ -33,19 +32,38 @@ public static void main(String... args) throws Exception {
     logger.fine(CommandHelper.execute("Optimizing resources", buildCommand(args)));
   }
 
-  private static List<String> buildCommand(String... args) {
-    OptionsParser optionsParser =
-        OptionsParser.builder()
-            .optionsClasses(Aapt2ConfigOptions.class, ResourceProcessorCommonOptions.class)
-            .argsPreProcessor(new ShellQuotedParamsFilePreProcessor(FileSystems.getDefault()))
-            .allowResidue(true)
-            .build();
-    optionsParser.parseAndExitUponError(args);
+  @Parameters(separators = "= ")
+  static class Options extends OptionsBaseWithResidue {
+    // NOTE: This options class needs the ParametersDelegate feature from JCommander for several
+    // reasons:
+    // * There are no Aapt2OptimizeAction-specific options, just fake "inheritance" from
+    //   Aapt2ConfigOptions and ResourceProcessorCommonOptions.
+    // * The action itself reads the residue from the command line
+    // In JCommander, residue collection is done at a per-option-class basis, not per OptionsParser,
+    // as was the case with the Bazel OptionsParser. Simultaneously, only _one_ object in a list of
+    // Objects passed to JCommander can have residue collection.
+    // Therefore, the most straightforward option here is to "inherit" the sub-option classes into a
+    // super Options class, with residue collection enabled for Options.
+
+    @ParametersDelegate public Aapt2ConfigOptions aapt2ConfigOptions = new Aapt2ConfigOptions();
+
+    @ParametersDelegate
+    public ResourceProcessorCommonOptions resourceProcessorCommonOptions =
+        new ResourceProcessorCommonOptions();
+  }
+
+  private static List<String> buildCommand(String... args) throws CompatOptionsParsingException {
+    Options options = new Options();
+    JCommander jc = new JCommander(options);
+    String[] preprocessedArgs = AndroidOptionsUtils.runArgFilePreprocessor(jc, args);
+    String[] normalizedArgs =
+        AndroidOptionsUtils.normalizeBooleanOptions(options, preprocessedArgs);
+    jc.parse(normalizedArgs);
 
     return ImmutableList.<String>builder()
-        .add(optionsParser.getOptions(Aapt2ConfigOptions.class).aapt2.toString())
+        .add(options.aapt2ConfigOptions.aapt2.toString())
         .add("optimize")
-        .addAll(optionsParser.getResidue())
+        .addAll(options.getResidue())
         .build();
   }
 

diff --git a/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java b/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java
@@ -38,11 +38,8 @@
 import com.google.devtools.build.android.aapt2.ResourceCompiler;
 import com.google.devtools.build.android.aapt2.ResourceLinker;
 import com.google.devtools.build.android.aapt2.StaticLibrary;
-import com.google.devtools.common.options.OptionsParser;
-import com.google.devtools.common.options.ShellQuotedParamsFilePreProcessor;
 import com.google.devtools.common.options.TriState;
 import java.io.IOException;
-import java.nio.file.FileSystems;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.List;
@@ -73,7 +70,7 @@ public class Aapt2ResourcePackagingAction {
 
   /** Flag specifications for this action. */
   @Parameters(separators = "= ")
-  public static final class Options extends OptionsBaseWithResidue {
+  public static final class Options {
     @Parameter(
         names = "--primaryData",
         converter = CompatUnvalidatedAndroidDataConverter.class,
@@ -229,18 +226,23 @@ public static final class Options extends OptionsBaseWithResidue {
 
     @Parameter(
         names = "--throwOnResourceConflict",
+        arity = 1,
         description =
             "If passed, resource merge conflicts will be treated as errors instead of warnings")
     public boolean throwOnResourceConflict;
 
-    @Parameter(names = "--packageUnderTest", description = "Unused/deprecated option.")
+    @Parameter(names = "--packageUnderTest", arity = 1, description = "Unused/deprecated option.")
     public String packageUnderTest;
 
-    @Parameter(names = "--isTestWithResources", description = "Unused/deprecated option.")
+    @Parameter(
+        names = "--isTestWithResources",
+        arity = 1,
+        description = "Unused/deprecated option.")
     public boolean isTestWithResources;
 
     @Parameter(
         names = "--includeProguardLocationReferences",
+        arity = 1,
         description = "When generating proguard configurations, include location references.")
     public boolean includeProguardLocationReferences;
 
@@ -255,18 +257,15 @@ public static final class Options extends OptionsBaseWithResidue {
   public static void main(String[] args) throws Exception {
     Profiler profiler = InMemoryProfiler.createAndStart("setup");
     Options options = new Options();
-    String[] normalizedArgs = AndroidOptionsUtils.normalizeBooleanOptions(options, args);
-    JCommander jc = new JCommander(options);
+    Aapt2ConfigOptions aaptConfigOptions = new Aapt2ConfigOptions();
+    ResourceProcessorCommonOptions resourceProcessorCommonOptions =
+        new ResourceProcessorCommonOptions();
+    Object[] allOptions = new Object[] {options, aaptConfigOptions, resourceProcessorCommonOptions};
+    JCommander jc = new JCommander(allOptions);
+    String[] preprocessedArgs = AndroidOptionsUtils.runArgFilePreprocessor(jc, args);
+    String[] normalizedArgs =
+        AndroidOptionsUtils.normalizeBooleanOptions(allOptions, preprocessedArgs);
     jc.parse(normalizedArgs);
-    List<String> residue = options.getResidue();
-    OptionsParser optionsParser =
-        OptionsParser.builder()
-            .optionsClasses(Aapt2ConfigOptions.class, ResourceProcessorCommonOptions.class)
-            .argsPreProcessor(new ShellQuotedParamsFilePreProcessor(FileSystems.getDefault()))
-            .build();
-    optionsParser.parseAndExitUponError(residue.toArray(new String[0]));
-
-    Aapt2ConfigOptions aaptConfigOptions = optionsParser.getOptions(Aapt2ConfigOptions.class);
 
     Preconditions.checkArgument(
         options.packageId == -1 || (options.packageId >= 2 && options.packageId <= 255),
@@ -312,8 +311,7 @@ public static void main(String[] args) throws Exception {
                               options.versionName,
                               manifest,
                               processedManifest,
-                              optionsParser.getOptions(ResourceProcessorCommonOptions.class)
-                                  .logWarnings))
+                              resourceProcessorCommonOptions.logWarnings))
               .processManifest(
                   manifest ->
                       new DensitySpecificManifestProcessor(options.densities, densityManifest)

diff --git a/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourceShrinkingAction.java b/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourceShrinkingAction.java
@@ -20,12 +20,8 @@
 import com.google.devtools.build.android.aapt2.Aapt2ConfigOptions;
 import com.google.devtools.build.android.aapt2.ResourceLinker;
 import com.google.devtools.build.android.aapt2.StaticLibrary;
-import com.google.devtools.common.options.OptionsParser;
-import com.google.devtools.common.options.ShellQuotedParamsFilePreProcessor;
-import java.nio.file.FileSystems;
 import java.nio.file.Files;
 import java.util.LinkedHashSet;
-import java.util.List;
 import java.util.Set;
 
 /**
@@ -49,17 +45,15 @@ public static void main(String[] args) throws Exception {
     final Profiler profiler = LoggingProfiler.createAndStart("shrink").startTask("flags");
     // Parse arguments.
     Options options = new Options();
-    JCommander jc = new JCommander(options);
-    jc.parse(args);
-    List<String> residue = options.getResidue();
+    Aapt2ConfigOptions aapt2ConfigOptions = new Aapt2ConfigOptions();
+    Object[] allOptions =
+        new Object[] {options, aapt2ConfigOptions, new ResourceProcessorCommonOptions()};
+    JCommander jc = new JCommander(allOptions);
+    String[] preprocessedArgs = AndroidOptionsUtils.runArgFilePreprocessor(jc, args);
+    String[] normalizedArgs =
+        AndroidOptionsUtils.normalizeBooleanOptions(allOptions, preprocessedArgs);
+    jc.parse(normalizedArgs);
 
-    OptionsParser optionsParser =
-        OptionsParser.builder()
-            .optionsClasses(Aapt2ConfigOptions.class, ResourceProcessorCommonOptions.class)
-            .argsPreProcessor(new ShellQuotedParamsFilePreProcessor(FileSystems.getDefault()))
-            .build();
-    optionsParser.parseAndExitUponError(residue.toArray(new String[0]));
-    Aapt2ConfigOptions aapt2ConfigOptions = optionsParser.getOptions(Aapt2ConfigOptions.class);
     profiler.recordEndOf("flags").startTask("setup");
 
     try (ScopedTemporaryDirectory scopedTmp =