Merge "Keep disabled resources out of final apk" into main

8 files changed, 176 insertions, 13 deletions

diff --git a/core/tests/resourceflaggingtests/src/com/android/resourceflaggingtests/ResourceFlaggingTest.java b/core/tests/resourceflaggingtests/src/com/android/resourceflaggingtests/ResourceFlaggingTest.java
index a3d8d3dbf31a..c1e357864fff 100644
--- a/core/tests/resourceflaggingtests/src/com/android/resourceflaggingtests/ResourceFlaggingTest.java
+++ b/core/tests/resourceflaggingtests/src/com/android/resourceflaggingtests/ResourceFlaggingTest.java
@@ -68,11 +68,6 @@ public class ResourceFlaggingTest {
         assertThat(getBoolean("res3")).isTrue();
     }
 
-    @Test
-    public void testFlagDisabledNoValue() {
-        assertThat(getString("str1")).isEqualTo("");
-    }
-
     private boolean getBoolean(String name) {
         int resId = mResources.getIdentifier(
                 name,
diff --git a/tools/aapt2/Android.bp b/tools/aapt2/Android.bp
index 7b227cebe2ae..f43cf521edf5 100644
--- a/tools/aapt2/Android.bp
+++ b/tools/aapt2/Android.bp
@@ -113,6 +113,7 @@ cc_library_host_static {
         "io/ZipArchive.cpp",
         "link/AutoVersioner.cpp",
         "link/FeatureFlagsFilter.cpp",
+        "link/FlagDisabledResourceRemover.cpp",
         "link/ManifestFixer.cpp",
         "link/NoDefaultResourceRemover.cpp",
         "link/PrivateAttributeMover.cpp",
@@ -190,6 +191,7 @@ cc_test_host {
         "integration-tests/ConvertTest/**/*",
         "integration-tests/DumpTest/**/*",
         ":resource-flagging-test-app-apk",
+        ":resource-flagging-test-app-r-java",
     ],
 }
 
diff --git a/tools/aapt2/cmd/Link.cpp b/tools/aapt2/cmd/Link.cpp
index 88cc4dab2a96..56f52885b36d 100644
--- a/tools/aapt2/cmd/Link.cpp
+++ b/tools/aapt2/cmd/Link.cpp
@@ -57,6 +57,7 @@
 #include "java/ManifestClassGenerator.h"
 #include "java/ProguardRules.h"
 #include "link/FeatureFlagsFilter.h"
+#include "link/FlagDisabledResourceRemover.h"
 #include "link/Linkers.h"
 #include "link/ManifestFixer.h"
 #include "link/NoDefaultResourceRemover.h"
@@ -1885,6 +1886,12 @@ class Linker {
       }
     }
 
+    if (!FlagDisabledResourceRemover{}.Consume(context_, table)) {
+      context_->GetDiagnostics()->Error(android::DiagMessage()
+                                        << "failed removing resources behind disabled flags");
+      return 1;
+    }
+
     const bool keep_raw_values = (context_->GetPackageType() == PackageType::kStaticLib)
                                  || options_.keep_raw_values;
     bool result = FlattenXml(context_, *manifest, kAndroidManifestPath, keep_raw_values,
@@ -2371,6 +2378,12 @@ class Linker {
       return 1;
     };
 
+    if (options_.generate_java_class_path || options_.generate_text_symbols_path) {
+      if (!GenerateJavaClasses()) {
+        return 1;
+      }
+    }
+
     if (!WriteApk(archive_writer.get(), &proguard_keep_set, manifest_xml.get(), &final_table_)) {
       return 1;
     }
@@ -2379,12 +2392,6 @@ class Linker {
       return 1;
     }
 
-    if (options_.generate_java_class_path || options_.generate_text_symbols_path) {
-      if (!GenerateJavaClasses()) {
-        return 1;
-      }
-    }
-
     if (!WriteProguardFile(options_.generate_proguard_rules_path, proguard_keep_set)) {
       return 1;
     }
diff --git a/tools/aapt2/integration-tests/FlaggedResourcesTest/Android.bp b/tools/aapt2/integration-tests/FlaggedResourcesTest/Android.bp
index efa8e0494856..5932271d4d28 100644
--- a/tools/aapt2/integration-tests/FlaggedResourcesTest/Android.bp
+++ b/tools/aapt2/integration-tests/FlaggedResourcesTest/Android.bp
@@ -47,10 +47,26 @@ genrule {
         "AndroidManifest.xml",
         ":resource-flagging-test-app-compile",
     ],
-    out: ["resapp.apk"],
+    out: [
+        "resapp.apk",
+    ],
     cmd: "$(location aapt2) link -o $(out) --manifest $(in)",
 }
 
+genrule {
+    name: "resource-flagging-test-app-r-java",
+    tools: ["aapt2"],
+    // The first input file in the list must be the manifest
+    srcs: [
+        "AndroidManifest.xml",
+        ":resource-flagging-test-app-compile",
+    ],
+    out: [
+        "resource-flagging-java/com/android/intenal/flaggedresources/R.java",
+    ],
+    cmd: "$(location aapt2) link -o $(genDir)/resapp.apk --java $(genDir)/resource-flagging-java --manifest $(in)",
+}
+
 java_genrule {
     name: "resource-flagging-test-app-apk-as-resource",
     srcs: [
diff --git a/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/bools.xml b/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/bools.xml
index 8d0146511d1d..3e094fbd669c 100644
--- a/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/bools.xml
+++ b/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/bools.xml
@@ -7,4 +7,6 @@
     <bool name="res2" android:featureFlag="test.package.trueFlag">true</bool>
 
     <bool name="res3">false</bool>
+
+    <bool name="res4" android:featureFlag="test.package.falseFlag">true</bool>
 </resources>
\ No newline at end of file
diff --git a/tools/aapt2/link/FlagDisabledResourceRemover.cpp b/tools/aapt2/link/FlagDisabledResourceRemover.cpp
new file mode 100644
index 000000000000..e3289e2a173a
--- /dev/null
+++ b/tools/aapt2/link/FlagDisabledResourceRemover.cpp
@@ -0,0 +1,59 @@
+/*
+ * Copyright (C) 2024 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "link/FlagDisabledResourceRemover.h"
+
+#include <algorithm>
+
+#include "ResourceTable.h"
+
+using android::ConfigDescription;
+
+namespace aapt {
+
+static bool KeepResourceEntry(const std::unique_ptr<ResourceEntry>& entry) {
+  if (entry->values.empty()) {
+    return true;
+  }
+  const auto end_iter = entry->values.end();
+  const auto remove_iter =
+      std::stable_partition(entry->values.begin(), end_iter,
+                            [](const std::unique_ptr<ResourceConfigValue>& value) -> bool {
+                              return value->flag_status != FlagStatus::Disabled;
+                            });
+
+  bool keep = remove_iter != entry->values.begin();
+
+  entry->values.erase(remove_iter, end_iter);
+  return keep;
+}
+
+bool FlagDisabledResourceRemover::Consume(IAaptContext* context, ResourceTable* table) {
+  for (auto& pkg : table->packages) {
+    for (auto& type : pkg->types) {
+      const auto end_iter = type->entries.end();
+      const auto remove_iter = std::stable_partition(
+          type->entries.begin(), end_iter, [](const std::unique_ptr<ResourceEntry>& entry) -> bool {
+            return KeepResourceEntry(entry);
+          });
+
+      type->entries.erase(remove_iter, end_iter);
+    }
+  }
+  return true;
+}
+
+}  // namespace aapt
\ No newline at end of file
diff --git a/tools/aapt2/link/FlagDisabledResourceRemover.h b/tools/aapt2/link/FlagDisabledResourceRemover.h
new file mode 100644
index 000000000000..2db2cb44c4cc
--- /dev/null
+++ b/tools/aapt2/link/FlagDisabledResourceRemover.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright (C) 2024 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include "android-base/macros.h"
+#include "process/IResourceTableConsumer.h"
+
+namespace aapt {
+
+// Removes any resource that are behind disabled flags.
+class FlagDisabledResourceRemover : public IResourceTableConsumer {
+ public:
+  FlagDisabledResourceRemover() = default;
+
+  bool Consume(IAaptContext* context, ResourceTable* table) override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(FlagDisabledResourceRemover);
+};
+
+}  // namespace aapt
diff --git a/tools/aapt2/link/FlaggedResources_test.cpp b/tools/aapt2/link/FlaggedResources_test.cpp
index 03c4ac9c83a2..c901b5866279 100644
--- a/tools/aapt2/link/FlaggedResources_test.cpp
+++ b/tools/aapt2/link/FlaggedResources_test.cpp
@@ -40,7 +40,25 @@ void DumpStringPoolToString(LoadedApk* loaded_apk, std::string* output) {
   output_stream.Flush();
 }
 
-TEST_F(FlaggedResourcesTest, DisabledStringRemoved) {
+void DumpResourceTableToString(LoadedApk* loaded_apk, std::string* output) {
+  StringOutputStream output_stream(output);
+  Printer printer(&output_stream);
+
+  DumpTableCommand command(&printer, &noop_diag);
+  ASSERT_EQ(command.Dump(loaded_apk), 0);
+  output_stream.Flush();
+}
+
+void DumpChunksToString(LoadedApk* loaded_apk, std::string* output) {
+  StringOutputStream output_stream(output);
+  Printer printer(&output_stream);
+
+  DumpChunks command(&printer, &noop_diag);
+  ASSERT_EQ(command.Dump(loaded_apk), 0);
+  output_stream.Flush();
+}
+
+TEST_F(FlaggedResourcesTest, DisabledStringRemovedFromPool) {
   auto apk_path = file::BuildPath({android::base::GetExecutableDirectory(), "resapp.apk"});
   auto loaded_apk = LoadedApk::LoadApkFromPath(apk_path, &noop_diag);
 
@@ -51,4 +69,33 @@ TEST_F(FlaggedResourcesTest, DisabledStringRemoved) {
   ASSERT_EQ(output.find(excluded), std::string::npos);
 }
 
+TEST_F(FlaggedResourcesTest, DisabledResourcesRemovedFromTable) {
+  auto apk_path = file::BuildPath({android::base::GetExecutableDirectory(), "resapp.apk"});
+  auto loaded_apk = LoadedApk::LoadApkFromPath(apk_path, &noop_diag);
+
+  std::string output;
+  DumpResourceTableToString(loaded_apk.get(), &output);
+}
+
+TEST_F(FlaggedResourcesTest, DisabledResourcesRemovedFromTableChunks) {
+  auto apk_path = file::BuildPath({android::base::GetExecutableDirectory(), "resapp.apk"});
+  auto loaded_apk = LoadedApk::LoadApkFromPath(apk_path, &noop_diag);
+
+  std::string output;
+  DumpChunksToString(loaded_apk.get(), &output);
+
+  ASSERT_EQ(output.find("res4"), std::string::npos);
+  ASSERT_EQ(output.find("str1"), std::string::npos);
+}
+
+TEST_F(FlaggedResourcesTest, DisabledResourcesInRJava) {
+  auto r_path = file::BuildPath({android::base::GetExecutableDirectory(), "resource-flagging-java",
+                                 "com", "android", "intenal", "flaggedresources", "R.java"});
+  std::string r_contents;
+  ::android::base::ReadFileToString(r_path, &r_contents);
+
+  ASSERT_NE(r_contents.find("public static final int res4"), std::string::npos);
+  ASSERT_NE(r_contents.find("public static final int str1"), std::string::npos);
+}
+
 }  // namespace aapt