Error out if skip_preprocessed_apk_checks is set when it's not necessary

So that we don't erroneously add skip_preprocessed_apk_checks
everywhere.

Bug: 185811447
Test: Presubmits
Change-Id: Icb1c6163d170ca4181c5c6a814b51fda4777746f

java/app_import.go

@@ -19,6 +19,7 @@ package java
 import (
 	"fmt"
 	"reflect"
+	"strings"
 
 	"github.com/google/blueprint"
 
@@ -51,27 +52,11 @@ var (
 		Description: "Uncompress dex files",
 	})
 
-	checkDexLibsAreUncompressedRule = pctx.AndroidStaticRule("check-dex-libs-are-uncompressed", blueprint.RuleParams{
+	checkPresignedApkRule = pctx.AndroidStaticRule("check-presigned-apk", blueprint.RuleParams{
-		// grep -v ' stor ' will search for lines that don't have ' stor '. stor means the file is stored uncompressed
+		Command:     "build/soong/scripts/check_prebuilt_presigned_apk.py --aapt2 ${config.Aapt2Cmd} --zipalign ${config.ZipAlign} $extraArgs $in $out",
-		Command: "if (zipinfo $in '*.dex' 2>/dev/null | grep -v ' stor ' >/dev/null) ; then " +
+		CommandDeps: []string{"build/soong/scripts/check_prebuilt_presigned_apk.py", "${config.Aapt2Cmd}", "${config.ZipAlign}"},
-			"echo $in: Contains compressed JNI libraries and/or dex files >&2;" +
+		Description: "Check presigned apk",
-			"exit 1; " +
+	}, "extraArgs")
-			"else " +
-			"touch $out; " +
-			"fi",
-		Description: "Check for compressed JNI libs or dex files",
-	})
-
-	checkJniLibsAreUncompressedRule = pctx.AndroidStaticRule("check-jni-libs-are-uncompressed", blueprint.RuleParams{
-		// grep -v ' stor ' will search for lines that don't have ' stor '. stor means the file is stored uncompressed
-		Command: "if (zipinfo $in 'lib/*.so' 2>/dev/null | grep -v ' stor ' >/dev/null) ; then " +
-			"echo $in: Contains compressed JNI libraries >&2;" +
-			"exit 1; " +
-			"else " +
-			"touch $out; " +
-			"fi",
-		Description: "Check for compressed JNI libs or dex files",
-	})
 )
 
 func RegisterAppImportBuildComponents(ctx android.RegistrationContext) {
@@ -352,20 +337,14 @@ func (a *AndroidAppImport) generateAndroidBuildActions(ctx android.ModuleContext
 	// Sign or align the package if package has not been preprocessed
 
 	if proptools.Bool(a.properties.Preprocessed) {
-		var output android.WritablePath
+		validationStamp := a.validatePresignedApk(ctx, srcApk)
-		if !proptools.Bool(a.properties.Skip_preprocessed_apk_checks) {
+		output := android.PathForModuleOut(ctx, apkFilename)
-			output = android.PathForModuleOut(ctx, "validated-prebuilt", apkFilename)
+		ctx.Build(pctx, android.BuildParams{
-			a.validatePreprocessedApk(ctx, srcApk, output)
+			Rule:       android.Cp,
-		} else {
+			Input:      srcApk,
-			// If using the input APK unmodified, still make a copy of it so that the output filename has the
+			Output:     output,
-			// right basename.
+			Validation: validationStamp,
-			output = android.PathForModuleOut(ctx, apkFilename)
+		})
-			ctx.Build(pctx, android.BuildParams{
-				Rule:   android.Cp,
-				Input:  srcApk,
-				Output: output,
-			})
-		}
 		a.outputFile = output
 		a.certificate = PresignedCertificate
 	} else if !Bool(a.properties.Presigned) {
@@ -384,13 +363,9 @@ func (a *AndroidAppImport) generateAndroidBuildActions(ctx android.ModuleContext
 		SignAppPackage(ctx, signed, jnisUncompressed, certificates, nil, lineageFile, rotationMinSdkVersion)
 		a.outputFile = signed
 	} else {
-		// Presigned without Preprocessed shouldn't really be a thing, currently we disallow
+		validationStamp := a.validatePresignedApk(ctx, srcApk)
-		// it for apps with targetSdk >= 30, because on those targetSdks you must be using signature
-		// v2 or later, and signature v2 would be wrecked by uncompressing libs / zipaligning.
-		// But ideally we would disallow it for all prebuilt apks, and remove the presigned property.
-		targetSdkCheck := a.validateTargetSdkLessThan30(ctx, srcApk)
 		alignedApk := android.PathForModuleOut(ctx, "zip-aligned", apkFilename)
-		TransformZipAlign(ctx, alignedApk, jnisUncompressed, []android.Path{targetSdkCheck})
+		TransformZipAlign(ctx, alignedApk, jnisUncompressed, []android.Path{validationStamp})
 		a.outputFile = alignedApk
 		a.certificate = PresignedCertificate
 	}
@@ -406,52 +381,28 @@ func (a *AndroidAppImport) generateAndroidBuildActions(ctx android.ModuleContext
 	// TODO: androidmk converter jni libs
 }
 
-func (a *AndroidAppImport) validatePreprocessedApk(ctx android.ModuleContext, srcApk android.Path, dstApk android.WritablePath) {
+func (a *AndroidAppImport) validatePresignedApk(ctx android.ModuleContext, srcApk android.Path) android.Path {
-	var validations android.Paths
+	stamp := android.PathForModuleOut(ctx, "validated-prebuilt", "check.stamp")
-
+	var extraArgs []string
-	alignmentStamp := android.PathForModuleOut(ctx, "validated-prebuilt", "alignment.stamp")
-	ctx.Build(pctx, android.BuildParams{
-		Rule:   checkZipAlignment,
-		Input:  srcApk,
-		Output: alignmentStamp,
-	})
-
-	validations = append(validations, alignmentStamp)
-	jniCompressionStamp := android.PathForModuleOut(ctx, "validated-prebuilt", "jni_compression.stamp")
-	ctx.Build(pctx, android.BuildParams{
-		Rule:   checkJniLibsAreUncompressedRule,
-		Input:  srcApk,
-		Output: jniCompressionStamp,
-	})
-	validations = append(validations, jniCompressionStamp)
-
 	if a.Privileged() {
-		// It's ok for non-privileged apps to have compressed dex files, see go/gms-uncompressed-jni-slides
+		extraArgs = append(extraArgs, "--privileged")
-		dexCompressionStamp := android.PathForModuleOut(ctx, "validated-prebuilt", "dex_compression.stamp")
+	}
-		ctx.Build(pctx, android.BuildParams{
+	if proptools.Bool(a.properties.Skip_preprocessed_apk_checks) {
-			Rule:   checkDexLibsAreUncompressedRule,
+		extraArgs = append(extraArgs, "--skip-preprocessed-apk-checks")
-			Input:  srcApk,
+	}
-			Output: dexCompressionStamp,
+	if proptools.Bool(a.properties.Preprocessed) {
-		})
+		extraArgs = append(extraArgs, "--preprocessed")
-		validations = append(validations, dexCompressionStamp)
 	}
 
 	ctx.Build(pctx, android.BuildParams{
-		Rule:        android.Cp,
+		Rule:   checkPresignedApkRule,
-		Input:       srcApk,
-		Output:      dstApk,
-		Validations: validations,
-	})
-}
-
-func (a *AndroidAppImport) validateTargetSdkLessThan30(ctx android.ModuleContext, srcApk android.Path) android.Path {
-	alignmentStamp := android.PathForModuleOut(ctx, "validated-prebuilt", "old_target_sdk.stamp")
-	ctx.Build(pctx, android.BuildParams{
-		Rule:   checkBelowTargetSdk30ForNonPreprocessedApks,
 		Input:  srcApk,
-		Output: alignmentStamp,
+		Output: stamp,
+		Args: map[string]string{
+			"extraArgs": strings.Join(extraArgs, " "),
+		},
 	})
-	return alignmentStamp
+	return stamp
 }
 
 func (a *AndroidAppImport) Prebuilt() *android.Prebuilt {

java/app_import_test.go

@@ -659,14 +659,19 @@ func TestAndroidAppImport_Preprocessed(t *testing.T) {
 
 	apkName := "foo.apk"
 	variant := ctx.ModuleForTests("foo", "android_common")
-	outputBuildParams := variant.Output("validated-prebuilt/" + apkName).BuildParams
+	outputBuildParams := variant.Output(apkName).BuildParams
 	if outputBuildParams.Rule.String() != android.Cp.String() {
 		t.Errorf("Unexpected prebuilt android_app_import rule: " + outputBuildParams.Rule.String())
 	}
 
 	// Make sure compression and aligning were validated.
-	if len(outputBuildParams.Validations) != 2 {
+	if outputBuildParams.Validation == nil {
-		t.Errorf("Expected compression/alignment validation rules, found %d validations", len(outputBuildParams.Validations))
+		t.Errorf("Expected validation rule, but was not found")
+	}
+
+	validationBuildParams := variant.Output("validated-prebuilt/check.stamp").BuildParams
+	if validationBuildParams.Rule.String() != checkPresignedApkRule.String() {
+		t.Errorf("Unexpected validation rule: " + validationBuildParams.Rule.String())
 	}
 }
 

java/builder.go

@@ -259,32 +259,11 @@ var (
 		},
 	)
 
-	checkZipAlignment = pctx.AndroidStaticRule("checkzipalign",
-		blueprint.RuleParams{
-			Command: "if ! ${config.ZipAlign} -c -p 4 $in > /dev/null; then " +
-				"echo $in: Improper package alignment >&2; " +
-				"exit 1; " +
-				"else " +
-				"touch $out; " +
-				"fi",
-			CommandDeps: []string{"${config.ZipAlign}"},
-			Description: "Check zip alignment",
-		},
-	)
-
 	convertImplementationJarToHeaderJarRule = pctx.AndroidStaticRule("convertImplementationJarToHeaderJar",
 		blueprint.RuleParams{
 			Command:     `${config.Zip2ZipCmd} -i ${in} -o ${out} -x 'META-INF/services/**/*'`,
 			CommandDeps: []string{"${config.Zip2ZipCmd}"},
 		})
-
-	checkBelowTargetSdk30ForNonPreprocessedApks = pctx.AndroidStaticRule("checkBelowTargetSdk30ForNonPreprocessedApks",
-		blueprint.RuleParams{
-			Command:     "build/soong/scripts/check_target_sdk_less_than_30.py ${config.Aapt2Cmd} $in $out",
-			CommandDeps: []string{"build/soong/scripts/check_target_sdk_less_than_30.py", "${config.Aapt2Cmd}"},
-			Description: "Check prebuilt target sdk version",
-		},
-	)
 )
 
 func init() {

scripts/check_prebuilt_presigned_apk.py

@@ -0,0 +1,70 @@
+#!/usr/bin/env python3
+
+import subprocess
+import argparse
+import re
+import sys
+import zipfile
+
+def check_target_sdk_less_than_30(args):
+    if not args.aapt2:
+        sys.exit('--aapt2 is required')
+    regex = re.compile(r"targetSdkVersion: *'([0-9]+)'")
+    output = subprocess.check_output([args.aapt2, "dump", "badging", args.apk], text=True)
+    targetSdkVersion = None
+    for line in output.splitlines():
+        match = regex.fullmatch(line.strip())
+        if match:
+            targetSdkVersion = int(match.group(1))
+            break
+
+    if targetSdkVersion is None or targetSdkVersion >= 30:
+        sys.exit(args.apk + ": Prebuilt, presigned apks with targetSdkVersion >= 30 (or a codename targetSdkVersion) must set preprocessed: true in the Android.bp definition (because they must be signed with signature v2, and the build system would wreck that signature otherwise)")
+
+def has_preprocessed_issues(args, *, fail=False):
+    if not args.zipalign:
+        sys.exit('--zipalign is required')
+    ret = subprocess.run([args.zipalign, '-c', '-p', '4', args.apk], stdout=subprocess.DEVNULL).returncode
+    if ret != 0:
+        if fail:
+            sys.exit(args.apk + ': Improper zip alignment')
+        return True
+
+    with zipfile.ZipFile(args.apk) as zf:
+        for info in zf.infolist():
+            if info.filename.startswith('lib/') and info.filename.endswith('.so') and info.compress_type != zipfile.ZIP_STORED:
+                if fail:
+                    sys.exit(args.apk + ': Contains compressed JNI libraries')
+                return True
+            # It's ok for non-privileged apps to have compressed dex files, see go/gms-uncompressed-jni-slides
+            if args.privileged:
+                if info.filename.endswith('.dex') and info.compress_type != zipfile.ZIP_STORED:
+                    if fail:
+                        sys.exit(args.apk + ': Contains compressed dex files and is privileged')
+                    return True
+    return False
+
+
+def main():
+    parser = argparse.ArgumentParser()
+    parser.add_argument('--aapt2', help = "the path to the aapt2 executable")
+    parser.add_argument('--zipalign', help = "the path to the zipalign executable")
+    parser.add_argument('--skip-preprocessed-apk-checks', action = 'store_true', help = "the value of the soong property with the same name")
+    parser.add_argument('--preprocessed', action = 'store_true', help = "the value of the soong property with the same name")
+    parser.add_argument('--privileged', action = 'store_true', help = "the value of the soong property with the same name")
+    parser.add_argument('apk', help = "the apk to check")
+    parser.add_argument('stampfile', help = "a file to touch if successful")
+    args = parser.parse_args()
+
+    if not args.preprocessed:
+        check_target_sdk_less_than_30(args)
+    elif args.skip_preprocessed_apk_checks:
+        if not has_preprocessed_issues(args):
+            sys.exit('This module sets `skip_preprocessed_apk_checks: true`, but does not actually have any issues. Please remove `skip_preprocessed_apk_checks`.')
+    else:
+        has_preprocessed_issues(args, fail=True)
+
+    subprocess.check_call(["touch", args.stampfile])
+
+if __name__ == "__main__":
+    main()

scripts/check_target_sdk_less_than_30.py

@@ -1,30 +0,0 @@
-#!/usr/bin/env python3
-
-import subprocess
-import argparse
-import re
-import sys
-
-def main():
-    parser = argparse.ArgumentParser()
-    parser.add_argument('aapt2', help = "the path to the aapt2 executable")
-    parser.add_argument('apk', help = "the apk to check")
-    parser.add_argument('stampfile', help = "a file to touch if successful")
-    args = parser.parse_args()
-
-    regex = re.compile(r"targetSdkVersion: *'([0-9]+)'")
-    output = subprocess.check_output([args.aapt2, "dump", "badging", args.apk], text=True)
-    targetSdkVersion = None
-    for line in output.splitlines():
-        match = regex.fullmatch(line.strip())
-        if match:
-            targetSdkVersion = int(match.group(1))
-            break
-
-    if targetSdkVersion is None or targetSdkVersion >= 30:
-        sys.exit(args.apk + ": Prebuilt, presigned apks with targetSdkVersion >= 30 (or a codename targetSdkVersion) must set preprocessed: true in the Android.bp definition (because they must be signed with signature v2, and the build system would wreck that signature otherwise)")
-
-    subprocess.check_call(["touch", args.stampfile])
-
-if __name__ == "__main__":
-    main()