Fix api fingerprinting with apex and update tests

Use run_shell instead of run in _run_apexer because --target_sdk_version may
use the API fingerprinting file contents using bash expansion,
and only run_shell can support that by executing the whole command with
/bin/bash -c. Regular run would quote the --target_sdk_version value with
single quotes ('--target_sdk_version=ABC.$(cat version.txt)'), preventing
bash expansion.

Test: adbd and tzdata have correct target sdk version and all the tests
in apex_test.bzl

Bug: 277921995
Change-Id: I6ec4e9ee4b01acb70b2b38b1416a59b2d1bfe18b

2 files changed, 77 insertions, 50 deletions

diff --git a/rules/apex/apex.bzl b/rules/apex/apex.bzl
index cc679cea..c31b6404 100644
--- a/rules/apex/apex.bzl
+++ b/rules/apex/apex.bzl
@@ -423,51 +423,52 @@ def _run_apexer(ctx, apex_toolchain):
     apexer_files = apex_toolchain.apexer[DefaultInfo].files_to_run
 
     # Arguments
-    args = ctx.actions.args()
-    args.add(file_mapping_file.path)
+    command = [ctx.executable._staging_dir_builder.path, file_mapping_file.path]
 
     # NOTE: When used as inputs to another sandboxed action, this directory
     # artifact's inner files will be made up of symlinks. Ensure that the
     # aforementioned action handles symlinks correctly (e.g. following
     # symlinks).
     staging_dir = ctx.actions.declare_directory(ctx.attr.name + "_staging_dir")
-    args.add(staging_dir.path)
+
+    command.append(staging_dir.path)
 
     # start of apexer cmd
-    args.add(apexer_files.executable.path)
+    command.append(apexer_files.executable.path)
     if ctx.attr._apexer_verbose[BuildSettingInfo].value:
-        args.add("--verbose")
-    args.add("--force")
-    args.add("--include_build_info")
-    args.add_all(["--canned_fs_config", canned_fs_config.path])
-    args.add_all(["--manifest", apex_manifest_pb.path])
-    args.add_all(["--file_contexts", file_contexts.path])
-    args.add_all(["--key", privkey.path])
-    args.add_all(["--pubkey", pubkey.path])
-    args.add_all(["--payload_type", "image"])
-    args.add_all(["--payload_fs_type", "ext4"])
-    args.add_all(["--assets_dir", notices_file.dirname])
+        command.append("--verbose")
+
+    command.append("--force")
+    command.append("--include_build_info")
+    command.extend(["--canned_fs_config", canned_fs_config.path])
+    command.extend(["--manifest", apex_manifest_pb.path])
+    command.extend(["--file_contexts", file_contexts.path])
+    command.extend(["--key", privkey.path])
+    command.extend(["--pubkey", pubkey.path])
+    command.extend(["--payload_type", "image"])
+    command.extend(["--payload_fs_type", "ext4"])
+    command.extend(["--assets_dir", notices_file.dirname])
 
     # Override the package name, if it's expicitly specified
     if ctx.attr.package_name:
-        args.add_all(["--override_apk_package_name", ctx.attr.package_name])
+        command.extend(["--override_apk_package_name", ctx.attr.package_name])
     else:
         override_package_name = _override_manifest_package_name(ctx)
         if override_package_name:
-            args.add_all(["--override_apk_package_name", override_package_name])
+            command.extend(["--override_apk_package_name", override_package_name])
 
     if ctx.attr.logging_parent:
-        args.add_all(["--logging_parent", ctx.attr.logging_parent])
+        command.extend(["--logging_parent", ctx.attr.logging_parent])
 
     use_api_fingerprint = _use_api_fingerprint(ctx)
 
-    target_sdk_version = api.final_or_future(api.default_app_target_sdk())
+    target_sdk_version = str(api.final_or_future(api.default_app_target_sdk()))
     if use_api_fingerprint:
         api_fingerprint_file = ctx.file._api_fingerprint_txt
-        sdk_version_suffix = ".$$(cat {})".format(api_fingerprint_file.path)
+        sdk_version_suffix = ".$(cat {})".format(api_fingerprint_file.path)
         target_sdk_version = ctx.attr._platform_sdk_codename[BuildSettingInfo].value + sdk_version_suffix
-        args.add(api_fingerprint_file.path)
-    args.add_all(["--target_sdk_version", target_sdk_version])
+
+    command.extend(["--target_sdk_version", target_sdk_version])
 
     # TODO(b/215339575): This is a super rudimentary way to convert "current" to a numerical number.
     # Generalize this to API level handling logic in a separate Starlark utility, preferably using
@@ -477,12 +478,12 @@ def _run_apexer(ctx, apex_toolchain):
         min_sdk_version = "10000"
 
     override_min_sdk_version = ctx.attr._apex_global_min_sdk_version_override[BuildSettingInfo].value
-    min_sdk_version = maybe_override_min_sdk_version(min_sdk_version, override_min_sdk_version)
+    min_sdk_version = str(maybe_override_min_sdk_version(min_sdk_version, override_min_sdk_version))
 
     if min_sdk_version == "10000" and use_api_fingerprint:
         min_sdk_version = ctx.attr._platform_sdk_codename[BuildSettingInfo].value + sdk_version_suffix
-        args.add(api_fingerprint_file.path)
-    args.add_all(["--min_sdk_version", min_sdk_version])
+        command.append(api_fingerprint_file.path)
+    command.extend(["--min_sdk_version", min_sdk_version])
 
     # apexer needs the list of directories containing all auxilliary tools invoked during
     # the creation of an apex
@@ -491,6 +492,7 @@ def _run_apexer(ctx, apex_toolchain):
     mke2fs_files = apex_toolchain.mke2fs[DefaultInfo].files_to_run
     resize2fs_files = apex_toolchain.resize2fs[DefaultInfo].files_to_run
     sefcontext_compile_files = apex_toolchain.sefcontext_compile[DefaultInfo].files_to_run
+    staging_dir_builder_files = ctx.attr._staging_dir_builder[DefaultInfo].files_to_run
     apexer_tool_paths = [
         apex_toolchain.aapt2.dirname,
         apexer_files.executable.dirname,
@@ -501,20 +503,21 @@ def _run_apexer(ctx, apex_toolchain):
         sefcontext_compile_files.executable.dirname,
     ]
 
-    args.add_all(["--apexer_tool_path", ":".join(apexer_tool_paths)])
+    command.extend(["--apexer_tool_path", ":".join(apexer_tool_paths)])
 
     android_manifest = ctx.file.android_manifest
     if android_manifest != None:
         if ctx.attr.testonly:
             android_manifest = _mark_manifest_as_test_only(ctx, apex_toolchain)
-        args.add_all(["--android_manifest", android_manifest.path])
+        command.extend(["--android_manifest", android_manifest.path])
     elif ctx.attr.testonly:
-        args.add("--test_only")
+        command.append("--test_only")
 
-    args.add(staging_dir.path)
-    args.add(apex_output_file)
+    command.append(staging_dir.path)
+    command.append(apex_output_file.path)
 
     inputs = [
+        ctx.executable._staging_dir_builder,
         file_mapping_file,
         canned_fs_config,
         apex_manifest_pb,
@@ -538,17 +541,22 @@ def _run_apexer(ctx, apex_toolchain):
         resize2fs_files,
         sefcontext_compile_files,
         apex_toolchain.aapt2,
+        staging_dir_builder_files,
     ]
 
-    ctx.actions.run(
+    # This is run_shell instead of run because --target_sdk_version may
+    # use the API fingerprinting file contents using bash expansion,
+    # and only run_shell can support that by executing the whole command with
+    # /bin/bash -c. Regular run would quote the --target_sdk_version value with
+    # single quotes ('--target_sdk_version=ABC.$(cat version.txt)'), preventing
+    # bash expansion.
+    ctx.actions.run_shell(
         inputs = inputs,
         tools = tools,
         outputs = [apex_output_file, staging_dir],
-        executable = ctx.executable._staging_dir_builder,
-        arguments = [args],
+        command = " ".join(command),
         mnemonic = "Apexer",
     )
-
     return struct(
         unsigned_apex = apex_output_file,
         requires_native_libs = requires_native_libs,
@@ -871,7 +879,7 @@ file mode, and cap is hexadecimal value for the capability.""",
             doc = """The minimum SDK version that this APEX must support at minimum. This is usually set to
 the SDK version that the APEX was first introduced.
 
-When not set, defaults to 10000 (or "current").""",
+When not set, defaults to "10000" (or "current").""",
         ),
         "updatable": attr.bool(default = True, doc = """Whether this APEX is considered updatable or not.
 
diff --git a/rules/apex/apex_test.bzl b/rules/apex/apex_test.bzl
index f714a599..51ba3d07 100644
--- a/rules/apex/apex_test.bzl
+++ b/rules/apex/apex_test.bzl
@@ -455,8 +455,9 @@ def _apex_manifest_test(ctx):
     conv_apex_manifest_action = [a for a in actions if a.mnemonic == "ConvApexManifest"][0]
 
     apexer_action = [a for a in actions if a.mnemonic == "Apexer"][0]
-    manifest_index = apexer_action.argv.index("--manifest")
-    manifest_path = apexer_action.argv[manifest_index + 1]
+    argv = apexer_action.argv[:-1] + apexer_action.argv[-1].split(" ")
+    manifest_index = argv.index("--manifest")
+    manifest_path = argv[manifest_index + 1]
 
     asserts.equals(
         env,
@@ -471,8 +472,8 @@ def _apex_manifest_test(ctx):
     )
 
     if ctx.attr.expected_min_sdk_version != "":
-        flag_index = apexer_action.argv.index("--min_sdk_version")
-        min_sdk_version_argv = apexer_action.argv[flag_index + 1]
+        flag_index = argv.index("--min_sdk_version")
+        min_sdk_version_argv = argv[flag_index + 1]
         asserts.equals(
             env,
             ctx.attr.expected_min_sdk_version,
@@ -492,9 +493,17 @@ apex_manifest_test = analysistest.make(
     **apex_manifest_test_attr
 )
 
+apex_manifest_global_min_sdk_current_test = analysistest.make(
+    config_settings = {
+        "@//build/bazel/rules/apex:unbundled_build_target_sdk_with_api_fingerprint": False,
+    },
+    **apex_manifest_test_attr
+)
+
 apex_manifest_global_min_sdk_override_tiramisu_test = analysistest.make(
     config_settings = {
         "@//build/bazel/rules/apex:apex_global_min_sdk_version_override": "Tiramisu",
+        "@//build/bazel/rules/apex:unbundled_build_target_sdk_with_api_fingerprint": False,
     },
     **apex_manifest_test_attr
 )
@@ -538,7 +547,8 @@ def _test_apex_manifest_min_sdk_version_current():
         min_sdk_version = "current",
     )
 
-    apex_manifest_test(
+    # this test verifies min_sdk_version without use_api_fingerprint
+    apex_manifest_global_min_sdk_current_test(
         name = test_name,
         target_under_test = name,
         expected_min_sdk_version = "10000",
@@ -555,6 +565,7 @@ def _test_apex_manifest_min_sdk_version_override():
         min_sdk_version = "30",
     )
 
+    # this test verifies min_sdk_version without use_api_fingerprint
     apex_manifest_global_min_sdk_override_tiramisu_test(
         name = test_name,
         target_under_test = name,
@@ -999,13 +1010,14 @@ def _action_args_test(ctx):
     actions = analysistest.target_actions(env)
 
     action = [a for a in actions if a.mnemonic == ctx.attr.action_mnemonic][0]
-    flag_idx = action.argv.index(ctx.attr.expected_args[0])
+    argv = action.argv[:-1] + action.argv[-1].split(" ")
+    flag_idx = argv.index(ctx.attr.expected_args[0])
 
     for i, expected_arg in enumerate(ctx.attr.expected_args):
         asserts.equals(
             env,
             expected_arg,
-            action.argv[flag_idx + i],
+            argv[flag_idx + i],
         )
 
     return analysistest.end(env)
@@ -1630,7 +1642,7 @@ def _apex_testonly_without_manifest_test_impl(ctx):
         len(actions) == 1,
         "No apexer action found: %s" % actions,
     )
-    argv = actions[0].argv
+    argv = actions[0].argv[:-1] + actions[0].argv[-1].split(" ")
 
     asserts.true(
         env,
@@ -2572,8 +2584,11 @@ def _min_target_sdk_version_api_fingerprint_test(ctx):
     for action in actions:
         if action.argv == None:
             continue
-        if "--min_sdk_version" in action.argv:
-            apexer_action = action
+        for a in action.argv:
+            if "--min_sdk_version" in a:
+                apexer_action = action
+                break
+        if apexer_action != None:
             break
 
     asserts.true(
@@ -2582,7 +2597,7 @@ def _min_target_sdk_version_api_fingerprint_test(ctx):
         "There is no apexer action in all the actions",
     )
 
-    argv = apexer_action.argv
+    argv = apexer_action.argv[:-1] + apexer_action.argv[-1].split(" ")
     api_fingerprint_in_input = False
     api_fingerprint_path = None
     for f in apexer_action.inputs.to_list():
@@ -2597,21 +2612,25 @@ def _min_target_sdk_version_api_fingerprint_test(ctx):
         "api_fingerprint.txt is not in the input files",
     )
 
-    expected_target_sdk_version = "123" + ".$$(cat {})".format(api_fingerprint_path)
+    expected_target_sdk_version = "123" + ".$(cat {})".format(api_fingerprint_path)
+    target_sdk_version_index = argv.index("--target_sdk_version")
     asserts.equals(
         env,
         expected = expected_target_sdk_version,
-        actual = argv[argv.index("--target_sdk_version") + 1],
+        actual = argv[target_sdk_version_index + 1] + " " + argv[target_sdk_version_index + 2],
     )
 
+    min_sdk_version_index = argv.index("--min_sdk_version")
     if ctx.attr.min_sdk_version in ["current", "10000"]:
-        expected_min_sdk_version = "123" + ".$$(cat {})".format(api_fingerprint_path)
+        expected_min_sdk_version = "123" + ".$(cat {})".format(api_fingerprint_path)
+        actual_min_sdk_version = argv[min_sdk_version_index + 1] + " " + argv[min_sdk_version_index + 2]
     else:
         expected_min_sdk_version = ctx.attr.min_sdk_version
+        actual_min_sdk_version = argv[min_sdk_version_index + 1]
     asserts.equals(
         env,
         expected = expected_min_sdk_version,
-        actual = argv[argv.index("--min_sdk_version") + 1],
+        actual = actual_min_sdk_version,
     )
 
     return analysistest.end(env)