From 4c3219ec5fc2f74b45f7b29b029caf56ed8a1b4d Mon Sep 17 00:00:00 2001 From: Tim Peut Date: Fri, 27 May 2022 17:51:49 -0700 Subject: [PATCH] Add new min_sdk_version attribute on android_binary and pipe to dex/desugar actions. Also introduce an allowlist to restrict packages able to set this new attribute. Eventually we may roll this out more broadly, but there are outstanding concerns around useability and scalability. RELNOTES: None PiperOrigin-RevId: 451524093 Change-Id: I9383cac05a20c5115b2202828ca0cea951244d53 --- .../lib/rules/android/AndroidBinary.java | 103 +++++++++++++----- .../android/AndroidBinaryMobileInstall.java | 2 +- .../lib/rules/android/AndroidCommon.java | 4 + .../lib/rules/android/AndroidRuleClasses.java | 9 ++ .../lib/rules/android/DexArchiveAspect.java | 35 +++++- .../lib/rules/android/AndroidBinaryTest.java | 88 +++++++++++++++ 6 files changed, 209 insertions(+), 32 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java index f6bcde64cb78d6..7a3ddc91750ff3 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java @@ -184,6 +184,13 @@ private static void validateRuleContext(RuleContext ruleContext, AndroidDataCont ruleContext.attributeError("multidex", "Multidex must be enabled"); } + if (ruleContext.attributes().isAttributeValueExplicitlySpecified("min_sdk_version") + && Allowlist.hasAllowlist(ruleContext, "allow_min_sdk_version") + && !Allowlist.isAvailable(ruleContext, "allow_min_sdk_version")) { + ruleContext.attributeError( + "min_sdk_version", "Target is not permitted to set min_sdk_version"); + } + if (AndroidCommon.getAndroidConfig(ruleContext).desugarJava8Libs() && getMultidexMode(ruleContext) == MultidexMode.OFF) { // Multidex is required so we can include legacy libs as a separate .dex file. @@ -263,30 +270,6 @@ private static RuleConfiguredTargetBuilder init( : null); } - Artifact manifestValidation = null; - if (Allowlist.hasAllowlist(ruleContext, "android_multidex_native_min_sdk_allowlist") - && !Allowlist.isAvailable(ruleContext, "android_multidex_native_min_sdk_allowlist") - && getMultidexMode(ruleContext) == MultidexMode.NATIVE - && ruleContext.isAttrDefined("$validate_manifest", LABEL)) { - manifestValidation = - ruleContext.getPackageRelativeArtifact( - ruleContext.getLabel().getName() + "_manifest_validation_output", - ruleContext.getBinOrGenfilesDirectory()); - ruleContext.registerAction( - createSpawnActionBuilder(ruleContext) - .setExecutable(ruleContext.getExecutablePrerequisite("$validate_manifest")) - .setProgressMessage("Validating %{input}") - .setMnemonic("ValidateManifest") - .addInput(manifest.getManifest()) - .addOutput(manifestValidation) - .addCommandLine( - CustomCommandLine.builder() - .addExecPath("--manifest", manifest.getManifest()) - .addExecPath("--output", manifestValidation) - .build()) - .build(ruleContext)); - } - boolean shrinkResourceCycles = shouldShrinkResourceCycles( dataContext.getAndroidConfig(), ruleContext, dataContext.isResourceShrinkingEnabled()); @@ -415,6 +398,38 @@ && getMultidexMode(ruleContext) == MultidexMode.NATIVE AndroidBinaryMobileInstall.createMobileInstallResourceApks( ruleContext, dataContext, manifest); + Artifact manifestValidation = null; + boolean shouldValidateMultidex = + (Allowlist.hasAllowlist(ruleContext, "android_multidex_native_min_sdk_allowlist") + && !Allowlist.isAvailable(ruleContext, "android_multidex_native_min_sdk_allowlist") + && getMultidexMode(ruleContext) == MultidexMode.NATIVE); + boolean shouldValidateMinSdk = getMinSdkVersion(ruleContext) > 0; + if (ruleContext.isAttrDefined("$validate_manifest", LABEL) + && (shouldValidateMultidex || shouldValidateMinSdk)) { + manifestValidation = + ruleContext.getPackageRelativeArtifact( + ruleContext.getLabel().getName() + "_manifest_validation_output", + ruleContext.getBinOrGenfilesDirectory()); + ruleContext.registerAction( + createSpawnActionBuilder(ruleContext) + .setExecutable(ruleContext.getExecutablePrerequisite("$validate_manifest")) + .setProgressMessage("Validating %{input}") + .setMnemonic("ValidateManifest") + .addInput(manifest.getManifest()) + .addOutput(manifestValidation) + .addCommandLine( + CustomCommandLine.builder() + .addExecPath("--manifest", manifest.getManifest()) + .addExecPath("--output", manifestValidation) + .addFormatted( + "--validate_multidex=%s", Boolean.toString(shouldValidateMultidex)) + .add( + "--expected_min_sdk_version", + Integer.toString(getMinSdkVersion(ruleContext))) + .build()) + .build(ruleContext)); + } + return createAndroidBinary( ruleContext, dataContext, @@ -1267,6 +1282,8 @@ private static DexingOutput dex( + "\" not supported by this version of the Android SDK"); } + int minSdkVersion = getMinSdkVersion(ruleContext); + int dexShards = ruleContext.attributes().get("dex_shards", Type.INTEGER).toIntUnchecked(); if (dexShards > 1) { if (multidexMode == MultidexMode.OFF) { @@ -1306,6 +1323,7 @@ private static DexingOutput dex( common, inclusionFilterJar, dexopts, + minSdkVersion, androidSemantics, attributes, derivedJarFunction, @@ -1320,6 +1338,7 @@ private static DexingOutput dex( proguardedJar, classesDex, dexopts, + minSdkVersion, /*multidex=*/ false, /*mainDexList=*/ null); } @@ -1356,6 +1375,7 @@ private static DexingOutput dex( common, inclusionFilterJar, dexopts, + minSdkVersion, androidSemantics, attributes, derivedJarFunction, @@ -1381,7 +1401,13 @@ private static DexingOutput dex( dexopts); } else { AndroidCommon.createDexAction( - ruleContext, shard, shardDex, dexopts, /*multidex=*/ true, /*mainDexList=*/ null); + ruleContext, + shard, + shardDex, + dexopts, + minSdkVersion, + /*multidex=*/ true, + /*mainDexList=*/ null); } } ImmutableList shardDexes = shardDexesBuilder.build(); @@ -1417,6 +1443,7 @@ private static DexingOutput dex( common, inclusionFilterJar, dexopts, + minSdkVersion, androidSemantics, attributes, derivedJarFunction, @@ -1436,6 +1463,7 @@ private static DexingOutput dex( proguardedJar, classesDexIntermediate, dexopts, + minSdkVersion, /*multidex=*/ true, mainDexList); createCleanDexZipAction(ruleContext, classesDexIntermediate, classesDex); @@ -1455,6 +1483,7 @@ private static void createIncrementalDexingActions( AndroidCommon common, @Nullable Artifact inclusionFilterJar, List dexopts, + int minSdkVersion, AndroidSemantics androidSemantics, JavaTargetAttributes attributes, Function derivedJarFunction, @@ -1471,7 +1500,12 @@ private static void createIncrementalDexingActions( ruleContext, collectRuntimeJars(common, attributes), collectDexArchives( - ruleContext, common, dexopts, androidSemantics, derivedJarFunction)); + ruleContext, + common, + dexopts, + minSdkVersion, + androidSemantics, + derivedJarFunction)); } else { if (proguardedJar != null && AndroidCommon.getAndroidConfig(ruleContext).incrementalDexingShardsAfterProguard() @@ -1493,6 +1527,7 @@ private static void createIncrementalDexingActions( "$dexbuilder_after_proguard", proguardedJar, DexArchiveAspect.topLevelDexbuilderDexopts(dexopts), + minSdkVersion, dexArchives.get(0)); } else { createShuffleJarActions( @@ -1503,6 +1538,7 @@ private static void createIncrementalDexingActions( common, inclusionFilterJar, dexopts, + minSdkVersion, androidSemantics, attributes, derivedJarFunction, @@ -1771,6 +1807,7 @@ public static Function collectDesugaredJars( jar, attributes.getBootClassPath().bootclasspath(), attributes.getCompileTimeClassPath(), + getMinSdkVersion(ruleContext), ruleContext.getDerivedArtifact( jarPath.replaceName(jarPath.getBaseName() + "_desugared.jar"), jar.getRoot())); result.addDesugaredJar(jar, desugared); @@ -1797,6 +1834,7 @@ private static Map collectDexArchives( RuleContext ruleContext, AndroidCommon common, List dexopts, + int minSdkVersion, AndroidSemantics semantics, Function derivedJarFunction) { DexArchiveProvider.Builder result = new DexArchiveProvider.Builder(); @@ -1820,6 +1858,7 @@ private static Map collectDexArchives( "$dexbuilder", derivedJarFunction.apply(jar), incrementalDexopts, + minSdkVersion, ruleContext.getDerivedArtifact( jarPath.replaceName(jarPath.getBaseName() + ".dex.zip"), jar.getRoot())); result.addDexArchive(incrementalDexopts, dexArchive, jar); @@ -1835,6 +1874,7 @@ private static Artifact createShuffleJarActions( AndroidCommon common, @Nullable Artifact inclusionFilterJar, List dexopts, + int minSdkVersion, AndroidSemantics semantics, JavaTargetAttributes attributes, Function derivedJarFunction, @@ -1889,7 +1929,8 @@ private static Artifact createShuffleJarActions( // there should be very few or no Jar files that still end up in shards. The dexing // step below will have to deal with those in addition to merging .dex files together. Map dexArchives = - collectDexArchives(ruleContext, common, dexopts, semantics, derivedJarFunction); + collectDexArchives( + ruleContext, common, dexopts, minSdkVersion, semantics, derivedJarFunction); classpath = toDexedClasspath(ruleContext, classpath, dexArchives); shardCommandLine.add("--split_dexed_classes"); } else { @@ -1916,6 +1957,7 @@ private static Artifact createShuffleJarActions( "$dexbuilder_after_proguard", shuffleOutputs.get(i), DexArchiveAspect.topLevelDexbuilderDexopts(dexopts), + minSdkVersion, shards.get(i)); } } @@ -2178,6 +2220,13 @@ public static MultidexMode getMultidexMode(RuleContext ruleContext) { } } + private static int getMinSdkVersion(RuleContext ruleContext) { + if (ruleContext.getRule().isAttrDefined("min_sdk_version", Type.INTEGER)) { + return ruleContext.attributes().get("min_sdk_version", Type.INTEGER).toIntUnchecked(); + } + return 0; + } + /** * List of Android SDKs that contain runtimes that do not support the native multidexing * introduced in Android L. If someone tries to build an android_binary that has multidex=native diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinaryMobileInstall.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinaryMobileInstall.java index 17467d3f42276f..42cb0971649d3c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinaryMobileInstall.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinaryMobileInstall.java @@ -369,7 +369,7 @@ private static Artifact getStubDex( ruleContext, split ? "split_stub_application/classes.dex" : "stub_application/classes.dex"); AndroidCommon.createDexAction( - ruleContext, stubDeployJar, stubDex, ImmutableList.of(), false, null); + ruleContext, stubDeployJar, stubDex, ImmutableList.of(), 0, false, null); return stubDex; } 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 38a39b11b6e8a6..28c705cf25a8f7 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 @@ -187,6 +187,7 @@ public static void createDexAction( Artifact jarToDex, Artifact classesDex, List dexOptions, + int minSdkVersion, boolean multidex, Artifact mainDexList) { CustomCommandLine.Builder commandLine = CustomCommandLine.builder(); @@ -201,6 +202,9 @@ public static void createDexAction( } commandLine.addAll(dexOptions); + if (minSdkVersion > 0) { + commandLine.add("--min_sdk_version", Integer.toString(minSdkVersion)); + } if (multidex) { commandLine.add("--multi-dex"); if (mainDexList != null) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java index 6d42fc9d837a48..880d6a1f03f5c5 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java @@ -571,6 +571,15 @@ Do PNG crunching (or not). This is independent of nine-patch processing, which i en_XA and/or ar_XB pseudo-locales. */ .add(attr(ResourceFilterFactory.RESOURCE_CONFIGURATION_FILTERS_NAME, STRING_LIST)) + /* + The minSdkVersion. This must match the minSdkVersion specified in the AndroidManifest.xml. + If set, will be used for dex/desugaring. + */ + .add( + attr("min_sdk_version", INTEGER) + .undocumented("experimental") + .value(StarlarkInt.of(0)) + .nonconfigurable("AspectParameters don't support configurations.")) /* Whether to perform resource shrinking. Resources that are not used by the binary will be removed from the APK. This is only supported for rules using local resources (i.e. the diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java b/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java index 9d5a3337e8f232..9779543751607b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java @@ -19,6 +19,7 @@ import static com.google.devtools.build.lib.packages.BuildType.LABEL; import static com.google.devtools.build.lib.packages.BuildType.TRISTATE; import static com.google.devtools.build.lib.packages.StarlarkProviderIdentifier.forKey; +import static com.google.devtools.build.lib.packages.Type.INTEGER; import static com.google.devtools.build.lib.rules.android.AndroidCommon.getAndroidConfig; import com.google.common.base.Function; @@ -97,6 +98,8 @@ public class DexArchiveAspect extends NativeAspectClass implements ConfiguredAsp AspectParameters.Builder result = new AspectParameters.Builder(); TriState incrementalAttr = attributes.get("incremental_dexing", TRISTATE); result.addAttribute("incremental_dexing", incrementalAttr.name()); + result.addAttribute( + "min_sdk_version", attributes.get("min_sdk_version", INTEGER).toString()); return result.build(); }; /** @@ -239,9 +242,14 @@ public ConfiguredAspect create( Iterable extraToolchainJars = getPlatformBasedToolchainJars(ruleContext); + int minSdkVersion = 0; + if (!params.getAttribute("min_sdk_version").isEmpty()) { + minSdkVersion = Integer.valueOf(params.getOnlyValueOfAttribute("min_sdk_version")); + } + Function desugaredJars = desugarJarsIfRequested( - ctadBase.getConfiguredTarget(), ruleContext, result, extraToolchainJars); + ctadBase.getConfiguredTarget(), ruleContext, minSdkVersion, result, extraToolchainJars); TriState incrementalAttr = TriState.valueOf(params.getOnlyValueOfAttribute("incremental_dexing")); @@ -280,6 +288,7 @@ public ConfiguredAspect create( ASPECT_DEXBUILDER_PREREQ, desugaredJars.apply(jar), incrementalDexopts, + minSdkVersion, AndroidBinary.getDxArtifact(ruleContext, uniqueFilename)); dexArchives.addDexArchive(incrementalDexopts, dexArchive, jar); } @@ -296,6 +305,7 @@ public ConfiguredAspect create( private Function desugarJarsIfRequested( ConfiguredTarget base, RuleContext ruleContext, + int minSdkVersion, ConfiguredAspect.Builder result, Iterable extraToolchainJars) { if (!getAndroidConfig(ruleContext).desugarJava8()) { @@ -344,7 +354,12 @@ private Function desugarJarsIfRequested( for (Artifact jar : jarsToProcess) { Artifact desugared = createDesugarAction( - ruleContext, basenameClash, jar, bootclasspath, compileTimeClasspath); + ruleContext, + basenameClash, + jar, + bootclasspath, + compileTimeClasspath, + minSdkVersion); newlyDesugared.put(jar, desugared); desugaredJars.addDesugaredJar(jar, desugared); } @@ -466,13 +481,15 @@ private Artifact createDesugarAction( boolean disambiguateBasenames, Artifact jar, NestedSet bootclasspath, - NestedSet compileTimeClasspath) { + NestedSet compileTimeClasspath, + int minSdkVersion) { return createDesugarAction( ruleContext, ASPECT_DESUGAR_PREREQ, jar, bootclasspath, compileTimeClasspath, + minSdkVersion, AndroidBinary.getDxArtifact( ruleContext, (disambiguateBasenames ? jar.getRootRelativePathString() : jar.getFilename()) @@ -485,6 +502,7 @@ private static Artifact createDesugarAction( Artifact jar, NestedSet bootclasspath, NestedSet classpath, + int minSdkVersion, Artifact result) { SpawnAction.Builder action = new SpawnAction.Builder() @@ -519,6 +537,9 @@ private static Artifact createDesugarAction( if (stripOutputPaths) { args.stripOutputPaths(result.getExecPath().subFragment(0, 1)); } + if (minSdkVersion > 0) { + args.add("--min_sdk_version", Integer.toString(minSdkVersion)); + } action .addCommandLine( @@ -545,8 +566,10 @@ static Artifact desugar( Artifact jar, NestedSet bootclasspath, NestedSet classpath, + int minSdkVersion, Artifact result) { - return createDesugarAction(ruleContext, "$desugar", jar, bootclasspath, classpath, result); + return createDesugarAction( + ruleContext, "$desugar", jar, bootclasspath, classpath, minSdkVersion, result); } /** @@ -576,6 +599,7 @@ static Artifact createDexArchiveAction( String dexbuilderPrereq, Artifact jar, Set incrementalDexopts, + int minSdkVersion, Artifact dexArchive) { SpawnAction.Builder dexbuilder = new SpawnAction.Builder() @@ -602,6 +626,9 @@ static Artifact createDexArchiveAction( .addExecPath("--input_jar", jar) .addExecPath("--output_zip", dexArchive) .addAll(ImmutableList.copyOf(incrementalDexopts)); + if (minSdkVersion > 0) { + args.add("--min_sdk_version", Integer.toString(minSdkVersion)); + } if (stripOutputPaths) { args.stripOutputPaths(dexArchive.getExecPath().subFragment(0, 1)); } 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 3ca0159419ce32..0983bfbc79cd55 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 @@ -696,6 +696,94 @@ public void testSimpleBinary_checkDesugarDepsAlwaysHappens() throws Exception { assertThat(found).isEqualTo(2 /* signed and unsigned apks */); } + @Test + public void testSimpleBinary_dexNoMinSdkVersion() throws Exception { + scratch.overwriteFile( + "java/android/BUILD", + "android_binary(name = 'app',", + " srcs = ['A.java'],", + " manifest = 'AndroidManifest.xml',", + " resource_files = glob(['res/**']),", + " multidex = 'legacy',", + " )"); + useConfiguration("--experimental_desugar_java8_libs"); + ConfiguredTarget binary = getConfiguredTarget("//java/android:app"); + + List args = + getGeneratingSpawnActionArgs( + ActionsTestUtil.getFirstArtifactEndingWith( + actionsTestUtil().artifactClosureOf(getFilesToBuild(binary)), + "/libapp.jar.dex.zip")); + assertThat(args).doesNotContain("--min_sdk_version"); + } + + @Test + public void testSimpleBinary_dexMinSdkVersion() throws Exception { + scratch.overwriteFile( + "java/android/BUILD", + "android_binary(name = 'app',", + " srcs = ['A.java'],", + " manifest = 'AndroidManifest.xml',", + " resource_files = glob(['res/**']),", + " min_sdk_version = 28,", + " multidex = 'legacy',", + " )"); + useConfiguration("--experimental_desugar_java8_libs"); + ConfiguredTarget binary = getConfiguredTarget("//java/android:app"); + + List args = + getGeneratingSpawnActionArgs( + ActionsTestUtil.getFirstArtifactEndingWith( + actionsTestUtil().artifactClosureOf(getFilesToBuild(binary)), + "/libapp.jar.dex.zip")); + assertThat(args).contains("--min_sdk_version"); + assertThat(args).contains("28"); + } + + @Test + public void testSimpleBinary_desugarNoMinSdkVersion() throws Exception { + scratch.overwriteFile( + "java/android/BUILD", + "android_binary(name = 'app',", + " srcs = ['A.java'],", + " manifest = 'AndroidManifest.xml',", + " resource_files = glob(['res/**']),", + " multidex = 'legacy',", + " )"); + useConfiguration("--experimental_desugar_java8_libs"); + ConfiguredTarget binary = getConfiguredTarget("//java/android:app"); + + List args = + getGeneratingSpawnActionArgs( + ActionsTestUtil.getFirstArtifactEndingWith( + actionsTestUtil().artifactClosureOf(getFilesToBuild(binary)), + "/libapp.jar_desugared.jar")); + assertThat(args).doesNotContain("--min_sdk_version"); + } + + @Test + public void testSimpleBinary_desugarMinSdkVersion() throws Exception { + scratch.overwriteFile( + "java/android/BUILD", + "android_binary(name = 'app',", + " srcs = ['A.java'],", + " manifest = 'AndroidManifest.xml',", + " resource_files = glob(['res/**']),", + " min_sdk_version = 28,", + " multidex = 'legacy',", + " )"); + useConfiguration("--experimental_desugar_java8_libs"); + ConfiguredTarget binary = getConfiguredTarget("//java/android:app"); + + List args = + getGeneratingSpawnActionArgs( + ActionsTestUtil.getFirstArtifactEndingWith( + actionsTestUtil().artifactClosureOf(getFilesToBuild(binary)), + "/libapp.jar_desugared.jar")); + assertThat(args).contains("--min_sdk_version"); + assertThat(args).contains("28"); + } + // regression test for #3169099 @Test public void testBinarySrcs() throws Exception {