Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit bcbc12d9 authored by Mark Punzalan's avatar Mark Punzalan
Browse files

Update VersionCollapser to consider minorVersion 

The tests have been updated to use `AddString` instead of `AddSimple`, so that we can ensure that the correct configs were removed and renamed. They have also been refactored to be more readable.

This change also fixes a bug in the `ResTable_config` compare functions, where `version` was being compared. Since it is in a union with a struct of `sdkVersion` and `minorVersion`, the endianness determines which of the members is more significant. We now explicitly compare both fields to ensure consistency.

Bug: 383177182
Flag: EXEMPT Aconfig not supported on host tools
Test: atest aapt2_tests
Change-Id: Ib61918765da8864d26535c90d41df5a5d055d66b
parent 64671271

libs/androidfw/ResourceTypes.cpp

+10 −4
Original line number Diff line number Diff line
@@ -2202,8 +2202,11 @@ int ResTable_config::compare(const ResTable_config& o) const { 
    if (screenSize != o.screenSize) {

        return (screenSize > o.screenSize) ? 1 : -1;

    }

    if (version != o.version) {

        return (version > o.version) ? 1 : -1;

    if (sdkVersion != o.sdkVersion) {

        return (sdkVersion > o.sdkVersion) ? 1 : -1;

    }

    if (minorVersion != o.minorVersion) {

        return (minorVersion > o.minorVersion) ? 1 : -1;

    }

    if (screenLayout != o.screenLayout) {

        return (screenLayout > o.screenLayout) ? 1 : -1;
@@ -2286,8 +2289,11 @@ int ResTable_config::compareLogical(const ResTable_config& o) const { 
    if (uiMode != o.uiMode) {

        return uiMode < o.uiMode ? -1 : 1;

    }

    if (version != o.version) {

        return version < o.version ? -1 : 1;

    if (sdkVersion != o.sdkVersion) {

        return sdkVersion < o.sdkVersion ? -1 : 1;

    }

    if (minorVersion != o.minorVersion) {

        return minorVersion < o.minorVersion ? -1 : 1;

    }

    return 0;

}

tools/aapt2/optimize/VersionCollapser.cpp

+13 −8
Original line number Diff line number Diff line
@@ -72,6 +72,8 @@ FilterIterator<Iterator, Pred> make_filter_iterator(Iterator begin, 
 */

static void CollapseVersions(IAaptContext* context, int min_sdk, ResourceEntry* entry) {

  // First look for all sdks less than minSdk.

  // Note that we iterate in reverse order, meaning we will first encounter entries with the

  // highest SDK level and work our way down.

  for (auto iter = entry->values.rbegin(); iter != entry->values.rend();

       ++iter) {

    // Check if the item was already marked for removal.
@@ -80,10 +82,11 @@ static void CollapseVersions(IAaptContext* context, int min_sdk, ResourceEntry* 
    }



    const ConfigDescription& config = (*iter)->config;

    if (config.sdkVersion <= min_sdk) {

    if (config.sdkVersion != 0 && (config.sdkVersion < min_sdk ||

                                   (config.sdkVersion == min_sdk && config.minorVersion == 0))) {

      // This is the first configuration we've found with a smaller or equal SDK level to the

      // minimum. We MUST keep this one, but remove all others we find, which get overridden by this

      // one.

      // minimum. We MUST keep this one, but remove all others we find, which will be smaller and

      // therefore get overridden by this one.



      ConfigDescription config_without_sdk = config.CopyWithoutSdkVersion();

      auto pred = [&](const std::unique_ptr<ResourceConfigValue>& val) -> bool {
@@ -93,12 +96,12 @@ static void CollapseVersions(IAaptContext* context, int min_sdk, ResourceEntry* 
        }



        // Only return Configs that differ in SDK version.

        config_without_sdk.sdkVersion = val->config.sdkVersion;

        config_without_sdk.version = val->config.version;

        return config_without_sdk == val->config &&

               val->config.sdkVersion <= min_sdk;

      };



      // Remove the rest that match.

      // Remove the rest that match; all of them will be overridden by this one.

      auto filter_iter =

          make_filter_iterator(iter + 1, entry->values.rend(), pred);

      while (filter_iter.HasNext()) {
@@ -107,7 +110,8 @@ static void CollapseVersions(IAaptContext* context, int min_sdk, ResourceEntry* 
          context->GetDiagnostics()->Note(android::DiagMessage()

                                          << "removing configuration " << next->config.to_string()

                                          << " for entry: " << entry->name

                                          << ", because its SDK version is smaller than minSdk");

                                          << ", because its SDK version is smaller than minSdk "

                                          << min_sdk);

        }

        next = {};

      }
@@ -126,8 +130,9 @@ static void CollapseVersions(IAaptContext* context, int min_sdk, ResourceEntry* 
  // space in the resources.arsc table.

  bool modified = false;

  for (std::unique_ptr<ResourceConfigValue>& config_value : entry->values) {

    if (config_value->config.sdkVersion != 0 &&

        config_value->config.sdkVersion <= min_sdk) {

    const auto& config = config_value->config;

    if (config.sdkVersion != 0 && (config.sdkVersion < min_sdk ||

                                   (config.sdkVersion == min_sdk && config.minorVersion == 0))) {

      // Override the resource with a Configuration without an SDK.

      std::unique_ptr<ResourceConfigValue> new_value =

          util::make_unique<ResourceConfigValue>(

tools/aapt2/optimize/VersionCollapser_test.cpp

+147 −66
Original line number Diff line number Diff line
@@ -18,102 +18,183 @@ 


#include "test/Test.h"



using android::StringPiece;

using ::android::ConfigDescription;

using ::android::StringPiece;

using ::testing::IsNull;

using ::testing::NotNull;

using ::testing::StrEq;



namespace aapt {



static std::unique_ptr<ResourceTable> BuildTableWithConfigs(

    StringPiece name, std::initializer_list<std::string> list) {

    StringPiece name, std::initializer_list<std::string> configs) {

  test::ResourceTableBuilder builder;

  for (const std::string& item : list) {

    builder.AddSimple(name, test::ParseConfigOrDie(item));

  for (const std::string& config : configs) {

    // Use the config as the string value, so we can verify the correct config was renamed.

    builder.AddString(name, {}, test::ParseConfigOrDie(config), config);

  }

  return builder.Build();

}



TEST(VersionCollapserTest, CollapseVersions) {

  std::unique_ptr<IAaptContext> context =

      test::ContextBuilder().SetMinSdkVersion(7).Build();

static constexpr auto kResName = "@android:string/foo";



  const StringPiece res_name = "@android:string/foo";

static void ExpectConfigValueIsNull(ResourceTable* table, StringPiece config_str) {

  EXPECT_THAT(test::GetValueForConfig<String>(table, kResName, test::ParseConfigOrDie(config_str)),

              IsNull());

}



  std::unique_ptr<ResourceTable> table = BuildTableWithConfigs(

      res_name,

      {"land-v4", "land-v5", "sw600dp", "land-v6", "land-v14", "land-v21"});

static void ExpectConfigValueIs(ResourceTable* table, const ConfigDescription& config,

                                StringPiece expected_value_str) {

  auto value = test::GetValueForConfig<String>(table, kResName, config);

  ASSERT_THAT(value, NotNull());

  EXPECT_THAT(*value->value, StrEq(expected_value_str));

}



static void ExpectConfigValueIs(ResourceTable* table, StringPiece config_str,

                                StringPiece expected_value_str) {

  ExpectConfigValueIs(table, test::ParseConfigOrDie(config_str), expected_value_str);

}



TEST(VersionCollapserTest, CollapseVersions) {

  std::unique_ptr<IAaptContext> context = test::ContextBuilder().SetMinSdkVersion(7).Build();

  std::unique_ptr<ResourceTable> table = BuildTableWithConfigs(

      kResName, {"land-v4", "land-v5", "sw600dp", "land-v6", "land-v14", "land-v21"});

  VersionCollapser collapser;

  ASSERT_TRUE(collapser.Consume(context.get(), table.get()));



  // These should be removed.

  EXPECT_EQ(nullptr,

            test::GetValueForConfig<Id>(table.get(), res_name,

                                        test::ParseConfigOrDie("land-v4")));

  EXPECT_EQ(nullptr,

            test::GetValueForConfig<Id>(table.get(), res_name,

                                        test::ParseConfigOrDie("land-v5")));

  // This one should be removed because it was renamed to 'land', with the

  // version dropped.

  EXPECT_EQ(nullptr,

            test::GetValueForConfig<Id>(table.get(), res_name,

                                        test::ParseConfigOrDie("land-v6")));

  ExpectConfigValueIsNull(table.get(), "land-v4");

  ExpectConfigValueIsNull(table.get(), "land-v5");



  // land-v6 should have been converted to land.

  ExpectConfigValueIsNull(table.get(), "land-v6");

  ExpectConfigValueIs(table.get(), "land", "land-v6");



  // These should remain.

  EXPECT_NE(nullptr,

            test::GetValueForConfig<Id>(table.get(), res_name,

                                        test::ParseConfigOrDie("sw600dp")));



  // 'land' should be present because it was renamed from 'land-v6'.

  EXPECT_NE(nullptr,

            test::GetValueForConfig<Id>(table.get(), res_name,

                                        test::ParseConfigOrDie("land")));

  EXPECT_NE(nullptr,

            test::GetValueForConfig<Id>(table.get(), res_name,

                                        test::ParseConfigOrDie("land-v14")));

  EXPECT_NE(nullptr,

            test::GetValueForConfig<Id>(table.get(), res_name,

                                        test::ParseConfigOrDie("land-v21")));

  ExpectConfigValueIs(table.get(), "sw600dp", "sw600dp");

  ExpectConfigValueIs(table.get(), "land-v14", "land-v14");

  ExpectConfigValueIs(table.get(), "land-v21", "land-v21");

}



TEST(VersionCollapserTest, CollapseVersionsWhenMinSdkIsHighest) {

  std::unique_ptr<IAaptContext> context =

      test::ContextBuilder().SetMinSdkVersion(21).Build();

TEST(VersionCollapserTest, CollapseVersionsWhenMinSdkMatchesConfigVersionExactly) {

  std::unique_ptr<IAaptContext> context = test::ContextBuilder().SetMinSdkVersion(14).Build();

  std::unique_ptr<ResourceTable> table = BuildTableWithConfigs(

      kResName, {"land-v4", "land-v5", "sw600dp", "land-v6", "land-v14", "land-v21", "land-v22"});

  VersionCollapser collapser;

  ASSERT_TRUE(collapser.Consume(context.get(), table.get()));



  const StringPiece res_name = "@android:string/foo";

  // These should all be removed.

  ExpectConfigValueIsNull(table.get(), "land-v4");

  ExpectConfigValueIsNull(table.get(), "land-v5");

  ExpectConfigValueIsNull(table.get(), "land-v6");



  // land-v14 should have been converted to land.

  ExpectConfigValueIsNull(table.get(), "land-v14");

  ExpectConfigValueIs(table.get(), "land", "land-v14");



  // These should remain.

  ExpectConfigValueIs(table.get(), test::ParseConfigOrDie("sw600dp").CopyWithoutSdkVersion(),

                      "sw600dp");

  ExpectConfigValueIs(table.get(), "land-v21", "land-v21");

  ExpectConfigValueIs(table.get(), "land-v22", "land-v22");

}



TEST(VersionCollapserTest, CollapseVersionsWhenMinSdkIsHighest) {

  std::unique_ptr<IAaptContext> context = test::ContextBuilder().SetMinSdkVersion(21).Build();

  std::unique_ptr<ResourceTable> table = BuildTableWithConfigs(

      res_name, {"land-v4", "land-v5", "sw600dp", "land-v6", "land-v14",

                 "land-v21", "land-v22"});

      kResName, {"land-v4", "land-v5", "sw600dp", "land-v6", "land-v14", "land-v21"});

  VersionCollapser collapser;

  ASSERT_TRUE(collapser.Consume(context.get(), table.get()));



  // These should all be removed.

  EXPECT_EQ(nullptr,

            test::GetValueForConfig<Id>(table.get(), res_name,

                                        test::ParseConfigOrDie("land-v4")));

  EXPECT_EQ(nullptr,

            test::GetValueForConfig<Id>(table.get(), res_name,

                                        test::ParseConfigOrDie("land-v5")));

  EXPECT_EQ(nullptr,

            test::GetValueForConfig<Id>(table.get(), res_name,

                                        test::ParseConfigOrDie("land-v6")));

  EXPECT_EQ(nullptr,

            test::GetValueForConfig<Id>(table.get(), res_name,

                                        test::ParseConfigOrDie("land-v14")));

  ExpectConfigValueIsNull(table.get(), "land-v4");

  ExpectConfigValueIsNull(table.get(), "land-v5");

  ExpectConfigValueIsNull(table.get(), "land-v6");

  ExpectConfigValueIsNull(table.get(), "land-v14");



  // land-v21 should have been converted to land.

  ExpectConfigValueIsNull(table.get(), "land-v21");

  ExpectConfigValueIs(table.get(), "land", "land-v21");



  // These should remain.

  EXPECT_NE(nullptr,

            test::GetValueForConfig<Id>(

                table.get(), res_name,

                test::ParseConfigOrDie("sw600dp").CopyWithoutSdkVersion()));

  ExpectConfigValueIs(table.get(), test::ParseConfigOrDie("sw600dp").CopyWithoutSdkVersion(),

                      "sw600dp");

}



  // land-v21 should have been converted to land.

  EXPECT_NE(nullptr,

            test::GetValueForConfig<Id>(table.get(), res_name,

                                        test::ParseConfigOrDie("land")));

  // land-v22 should remain as-is.

  EXPECT_NE(nullptr,

            test::GetValueForConfig<Id>(table.get(), res_name,

                                        test::ParseConfigOrDie("land-v22")));

TEST(VersionCollapserTest, CollapseVersionsWithMinorVersion) {

  std::unique_ptr<IAaptContext> context = test::ContextBuilder().SetMinSdkVersion(47).Build();

  std::unique_ptr<ResourceTable> table =

      BuildTableWithConfigs(kResName, {"land-v4", "land-v45.5", "sw600dp", "land-v46", "land-v46.1",

                                       "land-v54.5", "land-v61", "land-v63.2"});

  VersionCollapser collapser;

  ASSERT_TRUE(collapser.Consume(context.get(), table.get()));



  // These should be removed.

  ExpectConfigValueIsNull(table.get(), "land-v4");

  ExpectConfigValueIsNull(table.get(), "land-v45.5");

  ExpectConfigValueIsNull(table.get(), "land-v46");



  // land-v46.1 should have been converted to land.

  ExpectConfigValueIsNull(table.get(), "land-v46.1");

  ExpectConfigValueIs(table.get(), "land", "land-v46.1");



  // These should remain.

  ExpectConfigValueIs(table.get(), test::ParseConfigOrDie("sw600dp").CopyWithoutSdkVersion(),

                      "sw600dp");

  ExpectConfigValueIs(table.get(), "land-v54.5", "land-v54.5");

  ExpectConfigValueIs(table.get(), "land-v61", "land-v61");

  ExpectConfigValueIs(table.get(), "land-v63.2", "land-v63.2");

}



TEST(VersionCollapserTest, CollapseVersionsWithMinorVersionAndMinSdkMatchesConfigVersionExactly) {

  std::unique_ptr<IAaptContext> context = test::ContextBuilder().SetMinSdkVersion(46).Build();

  std::unique_ptr<ResourceTable> table =

      BuildTableWithConfigs(kResName, {"land-v4", "land-v45.5", "sw600dp", "land-v46", "land-v46.1",

                                       "land-v54.5", "land-v61", "land-v63.2"});

  VersionCollapser collapser;

  ASSERT_TRUE(collapser.Consume(context.get(), table.get()));



  // These should be removed.

  ExpectConfigValueIsNull(table.get(), "land-v4");

  ExpectConfigValueIsNull(table.get(), "land-v45.5");



  // land-v46 should have been converted to land.

  ExpectConfigValueIsNull(table.get(), "land-v46");

  ExpectConfigValueIs(table.get(), "land", "land-v46");



  // These should remain.

  ExpectConfigValueIs(table.get(), test::ParseConfigOrDie("sw600dp").CopyWithoutSdkVersion(),

                      "sw600dp");

  ExpectConfigValueIs(table.get(), "land-v46.1", "land-v46.1");

  ExpectConfigValueIs(table.get(), "land-v54.5", "land-v54.5");

  ExpectConfigValueIs(table.get(), "land-v61", "land-v61");

  ExpectConfigValueIs(table.get(), "land-v63.2", "land-v63.2");

}



TEST(VersionCollapserTest, CollapseVersionsWithMinorVersionAndMinSdkIsHighest) {

  std::unique_ptr<IAaptContext> context = test::ContextBuilder().SetMinSdkVersion(64).Build();

  std::unique_ptr<ResourceTable> table =

      BuildTableWithConfigs(kResName, {"land-v4", "land-v45.5", "sw600dp", "land-v46", "land-v46.1",

                                       "land-v54.5", "land-v61", "land-v63.2"});



  VersionCollapser collapser;

  ASSERT_TRUE(collapser.Consume(context.get(), table.get()));



  // These should be removed.

  ExpectConfigValueIsNull(table.get(), "land-v4");

  ExpectConfigValueIsNull(table.get(), "land-v45.5");

  ExpectConfigValueIsNull(table.get(), "land-v46");

  ExpectConfigValueIsNull(table.get(), "land-v46.1");

  ExpectConfigValueIsNull(table.get(), "land-v54.5");

  ExpectConfigValueIsNull(table.get(), "land-v61");



  // land-v63.2 should have been converted to land.

  ExpectConfigValueIsNull(table.get(), "land-v63.2");

  ExpectConfigValueIs(table.get(), "land", "land-v63.2");



  // These should remain.

  ExpectConfigValueIs(table.get(), test::ParseConfigOrDie("sw600dp").CopyWithoutSdkVersion(),

                      "sw600dp");

}



}  // namespace aapt