From 11cdc1cf7ab2e915611e2bb7264bd312012a9b64 Mon Sep 17 00:00:00 2001 From: Shane Farmer Date: Wed, 31 Jan 2018 16:43:24 -0800 Subject: [PATCH] AAPT2: Add order attribute to groups Require explicit ordering of groups in the configuration file to ensure that the correct version code is set. Ordering based on a single ABI is straight forward to ensure Play Store delivers the correct APK, but when an APK needs more than one ABI things get messy quickly. This also goes for screen density etc. The only thing that is easily sorted without this attribute is android-sdk since an artifact can only reference a single SDK. Test: unit tests Test: manually split an APK with update config.xml Change-Id: I37a2b8b8a8409d6d6ff27c7142d4c8c8065a7a51 --- .../configuration/ConfigurationParser.cpp | 80 +++++++++-- .../ConfigurationParser.internal.h | 34 ++++- .../ConfigurationParser_test.cpp | 129 +++++++++++++----- tools/aapt2/configuration/aapt2.xsd | 5 + tools/aapt2/configuration/example/config.xml | 28 ++-- 5 files changed, 214 insertions(+), 62 deletions(-) diff --git a/tools/aapt2/configuration/ConfigurationParser.cpp b/tools/aapt2/configuration/ConfigurationParser.cpp index eabeb47fccec4..902334b98d004 100644 --- a/tools/aapt2/configuration/ConfigurationParser.cpp +++ b/tools/aapt2/configuration/ConfigurationParser.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include "android-base/file.h" @@ -93,6 +94,7 @@ class NoopDiagnostics : public IDiagnostics { }; NoopDiagnostics noop_; +/** Returns the value of the label attribute for a given element. */ std::string GetLabel(const Element* element, IDiagnostics* diag) { std::string label; for (const auto& attr : element->attributes) { @@ -108,6 +110,18 @@ std::string GetLabel(const Element* element, IDiagnostics* diag) { return label; } +/** Returns the value of the version-code-order attribute for a given element. */ +Maybe GetVersionCodeOrder(const Element* element, IDiagnostics* diag) { + const xml::Attribute* version = element->FindAttribute("", "version-code-order"); + if (version == nullptr) { + std::string label = GetLabel(element, diag); + diag->Error(DiagMessage() << "No version-code-order found for element '" << element->name + << "' with label '" << label << "'"); + return {}; + } + return std::stoi(version->value); +} + /** XML node visitor that removes all of the namespace URIs from the node and all children. */ class NamespaceVisitor : public xml::Visitor { public: @@ -437,26 +451,37 @@ Maybe> ConfigurationParser::Parse( // Convert from a parsed configuration to a list of artifacts for processing. const std::string& apk_name = file::GetFilename(apk_path).to_string(); std::vector output_artifacts; - bool has_errors = false; PostProcessingConfiguration& config = maybe_config.value(); - config.SortArtifacts(); + bool valid = true; int version = 1; + for (const ConfiguredArtifact& artifact : config.artifacts) { Maybe output_artifact = ToOutputArtifact(artifact, apk_name, config, diag_); if (!output_artifact) { // Defer return an error condition so that all errors are reported. - has_errors = true; + valid = false; } else { output_artifact.value().version = version++; output_artifacts.push_back(std::move(output_artifact.value())); } } - if (has_errors) { + if (!config.ValidateVersionCodeOrdering(diag_)) { + diag_->Error(DiagMessage() << "could not validate post processing configuration"); + valid = false; + } + + if (valid) { + // Sorting artifacts requires that all references are valid as it uses them to determine order. + config.SortArtifacts(); + } + + if (!valid) { return {}; } + return {output_artifacts}; } @@ -509,8 +534,15 @@ bool AbiGroupTagHandler(PostProcessingConfiguration* config, Element* root_eleme return false; } - auto& group = GetOrCreateGroup(label, &config->abi_groups); bool valid = true; + OrderedEntry& entry = config->abi_groups[label]; + Maybe order = GetVersionCodeOrder(root_element, diag); + if (!order) { + valid = false; + } else { + entry.order = order.value(); + } + auto& group = entry.entry; // Special case for empty abi-group tag. Label will be used as the ABI. if (root_element->GetChildElements().empty()) { @@ -519,7 +551,7 @@ bool AbiGroupTagHandler(PostProcessingConfiguration* config, Element* root_eleme return false; } group.push_back(abi->second); - return true; + return valid; } for (auto* child : root_element->GetChildElements()) { @@ -553,8 +585,15 @@ bool ScreenDensityGroupTagHandler(PostProcessingConfiguration* config, Element* return false; } - auto& group = GetOrCreateGroup(label, &config->screen_density_groups); bool valid = true; + OrderedEntry& entry = config->screen_density_groups[label]; + Maybe order = GetVersionCodeOrder(root_element, diag); + if (!order) { + valid = false; + } else { + entry.order = order.value(); + } + auto& group = entry.entry; // Special case for empty screen-density-group tag. Label will be used as the screen density. if (root_element->GetChildElements().empty()) { @@ -613,8 +652,15 @@ bool LocaleGroupTagHandler(PostProcessingConfiguration* config, Element* root_el return false; } - auto& group = GetOrCreateGroup(label, &config->locale_groups); bool valid = true; + OrderedEntry& entry = config->locale_groups[label]; + Maybe order = GetVersionCodeOrder(root_element, diag); + if (!order) { + valid = false; + } else { + entry.order = order.value(); + } + auto& group = entry.entry; // Special case to auto insert a locale for an empty group. Label will be used for locale. if (root_element->GetChildElements().empty()) { @@ -728,8 +774,15 @@ bool GlTextureGroupTagHandler(PostProcessingConfiguration* config, Element* root return false; } - auto& group = GetOrCreateGroup(label, &config->gl_texture_groups); bool valid = true; + OrderedEntry& entry = config->gl_texture_groups[label]; + Maybe order = GetVersionCodeOrder(root_element, diag); + if (!order) { + valid = false; + } else { + entry.order = order.value(); + } + auto& group = entry.entry; GlTexture result; for (auto* child : root_element->GetChildElements()) { @@ -771,8 +824,15 @@ bool DeviceFeatureGroupTagHandler(PostProcessingConfiguration* config, Element* return false; } - auto& group = GetOrCreateGroup(label, &config->device_feature_groups); bool valid = true; + OrderedEntry& entry = config->device_feature_groups[label]; + Maybe order = GetVersionCodeOrder(root_element, diag); + if (!order) { + valid = false; + } else { + entry.order = order.value(); + } + auto& group = entry.entry; for (auto* child : root_element->GetChildElements()) { if (child->name != "supports-feature") { diff --git a/tools/aapt2/configuration/ConfigurationParser.internal.h b/tools/aapt2/configuration/ConfigurationParser.internal.h index a583057427e60..f071a69fc9e34 100644 --- a/tools/aapt2/configuration/ConfigurationParser.internal.h +++ b/tools/aapt2/configuration/ConfigurationParser.internal.h @@ -33,18 +33,31 @@ namespace configuration { template struct OrderedEntry { - size_t order; + int32_t order; std::vector entry; }; -/** A mapping of group labels to group of configuration items. */ -template -using Group = std::unordered_map>; - /** A mapping of group label to a single configuration item. */ template using Entry = std::unordered_map; +/** A mapping of group labels to group of configuration items. */ +template +using Group = Entry>; + +template +bool IsGroupValid(const Group& group, const std::string& name, IDiagnostics* diag) { + std::set orders; + for (const auto& p : group) { + orders.insert(p.second.order); + } + bool valid = orders.size() == group.size(); + if (!valid) { + diag->Error(DiagMessage() << name << " have overlapping version-code-order attributes"); + } + return valid; +} + /** Retrieves an entry from the provided Group, creating a new instance if one does not exist. */ template std::vector& GetOrCreateGroup(std::string label, Group* group) { @@ -93,7 +106,7 @@ class ComparisonChain { private: template - inline size_t GetGroupOrder(const Group& groups, const Maybe& label) { + inline size_t GetGroupOrder(const Entry& groups, const Maybe& label) { if (!label) { return std::numeric_limits::max(); } @@ -141,6 +154,15 @@ struct PostProcessingConfiguration { Group gl_texture_groups; Entry android_sdks; + bool ValidateVersionCodeOrdering(IDiagnostics* diag) { + bool valid = IsGroupValid(abi_groups, "abi-groups", diag); + valid &= IsGroupValid(screen_density_groups, "screen-density-groups", diag); + valid &= IsGroupValid(locale_groups, "locale-groups", diag); + valid &= IsGroupValid(device_feature_groups, "device-feature-groups", diag); + valid &= IsGroupValid(gl_texture_groups, "gl-texture-groups", diag); + return valid; + } + /** * Sorts the configured artifacts based on the ordering of the groups in the configuration file. * The only exception to this rule is Android SDK versions. Larger SDK versions will have a larger diff --git a/tools/aapt2/configuration/ConfigurationParser_test.cpp b/tools/aapt2/configuration/ConfigurationParser_test.cpp index 0329846a5bb59..febbb2ed11a05 100644 --- a/tools/aapt2/configuration/ConfigurationParser_test.cpp +++ b/tools/aapt2/configuration/ConfigurationParser_test.cpp @@ -82,22 +82,22 @@ using ::testing::StrEq; constexpr const char* kValidConfig = R"( - - armeabi-v7a - arm64-v8a - - + x86 mips + + armeabi-v7a + arm64-v8a + - + xhdpi xxhdpi xxxhdpi - + ldpi mdpi hdpi @@ -107,17 +107,20 @@ constexpr const char* kValidConfig = R"( - + en es fr de - + en es-rMX fr-rCA + + + - + assets/dxt1/* - + android.hardware.audio.low_latency @@ -188,19 +191,22 @@ TEST_F(ConfigurationParserTest, ExtractConfiguration) { auto& arm = config.abi_groups["arm"]; auto& other = config.abi_groups["other"]; - EXPECT_EQ(arm.order, 1ul); - EXPECT_EQ(other.order, 2ul); + EXPECT_EQ(arm.order, 1); + EXPECT_EQ(other.order, 2); auto& large = config.screen_density_groups["large"]; auto& alldpi = config.screen_density_groups["alldpi"]; - EXPECT_EQ(large.order, 1ul); - EXPECT_EQ(alldpi.order, 2ul); + EXPECT_EQ(large.order, 2); + EXPECT_EQ(alldpi.order, 1); auto& north_america = config.locale_groups["north-america"]; auto& europe = config.locale_groups["europe"]; + auto& all = config.locale_groups["all"]; // Checked in reverse to make sure access order does not matter. - EXPECT_EQ(north_america.order, 2ul); - EXPECT_EQ(europe.order, 1ul); + EXPECT_EQ(north_america.order, 2); + EXPECT_EQ(europe.order, 1); + EXPECT_EQ(all.order, -1); + EXPECT_EQ(3ul, config.locale_groups.size()); } TEST_F(ConfigurationParserTest, ValidateFile) { @@ -392,7 +398,7 @@ TEST_F(ConfigurationParserTest, ArtifactFormatAction) { TEST_F(ConfigurationParserTest, AbiGroupAction) { static constexpr const char* xml = R"xml( - + armeabi-v7a @@ -415,7 +421,8 @@ TEST_F(ConfigurationParserTest, AbiGroupAction) { } TEST_F(ConfigurationParserTest, AbiGroupAction_EmptyGroup) { - static constexpr const char* xml = R"xml()xml"; + static constexpr const char* xml = + R"xml()xml"; auto doc = test::BuildXmlDom(xml); @@ -426,12 +433,23 @@ TEST_F(ConfigurationParserTest, AbiGroupAction_EmptyGroup) { EXPECT_THAT(config.abi_groups, SizeIs(1ul)); ASSERT_EQ(1u, config.abi_groups.count("arm64-v8a")); - auto& out = config.abi_groups["arm64-v8a"].entry; - ASSERT_THAT(out, ElementsAre(Abi::kArm64V8a)); + auto& out = config.abi_groups["arm64-v8a"]; + ASSERT_THAT(out.entry, ElementsAre(Abi::kArm64V8a)); + EXPECT_EQ(3, out.order); +} + +TEST_F(ConfigurationParserTest, AbiGroupAction_EmptyGroup_NoOrder) { + static constexpr const char* xml = R"xml()xml"; + + auto doc = test::BuildXmlDom(xml); + + PostProcessingConfiguration config; + bool ok = AbiGroupTagHandler(&config, NodeCast(doc.get()->root.get()), &diag_); + ASSERT_FALSE(ok); } TEST_F(ConfigurationParserTest, AbiGroupAction_InvalidEmptyGroup) { - static constexpr const char* xml = R"xml()xml"; + static constexpr const char* xml = R"xml()xml"; auto doc = test::BuildXmlDom(xml); @@ -442,7 +460,7 @@ TEST_F(ConfigurationParserTest, AbiGroupAction_InvalidEmptyGroup) { TEST_F(ConfigurationParserTest, ScreenDensityGroupAction) { static constexpr const char* xml = R"xml( - + xhdpi xxhdpi @@ -471,7 +489,8 @@ TEST_F(ConfigurationParserTest, ScreenDensityGroupAction) { } TEST_F(ConfigurationParserTest, ScreenDensityGroupAction_EmtpyGroup) { - static constexpr const char* xml = R"xml()xml"; + static constexpr const char* xml = + R"xml()xml"; auto doc = test::BuildXmlDom(xml); @@ -485,8 +504,19 @@ TEST_F(ConfigurationParserTest, ScreenDensityGroupAction_EmtpyGroup) { ConfigDescription xhdpi; xhdpi.density = ResTable_config::DENSITY_XHIGH; - auto& out = config.screen_density_groups["xhdpi"].entry; - ASSERT_THAT(out, ElementsAre(xhdpi)); + auto& out = config.screen_density_groups["xhdpi"]; + EXPECT_THAT(out.entry, ElementsAre(xhdpi)); + EXPECT_EQ(4, out.order); +} + +TEST_F(ConfigurationParserTest, ScreenDensityGroupAction_EmtpyGroup_NoVersion) { + static constexpr const char* xml = R"xml()xml"; + + auto doc = test::BuildXmlDom(xml); + + PostProcessingConfiguration config; + bool ok = ScreenDensityGroupTagHandler(&config, NodeCast(doc.get()->root.get()), &diag_); + ASSERT_FALSE(ok); } TEST_F(ConfigurationParserTest, ScreenDensityGroupAction_InvalidEmtpyGroup) { @@ -501,7 +531,7 @@ TEST_F(ConfigurationParserTest, ScreenDensityGroupAction_InvalidEmtpyGroup) { TEST_F(ConfigurationParserTest, LocaleGroupAction) { static constexpr const char* xml = R"xml( - + en es fr @@ -528,7 +558,7 @@ TEST_F(ConfigurationParserTest, LocaleGroupAction) { } TEST_F(ConfigurationParserTest, LocaleGroupAction_EmtpyGroup) { - static constexpr const char* xml = R"xml()xml"; + static constexpr const char* xml = R"xml()xml"; auto doc = test::BuildXmlDom(xml); @@ -539,11 +569,22 @@ TEST_F(ConfigurationParserTest, LocaleGroupAction_EmtpyGroup) { ASSERT_EQ(1ul, config.locale_groups.size()); ASSERT_EQ(1u, config.locale_groups.count("en")); - const auto& out = config.locale_groups["en"].entry; + const auto& out = config.locale_groups["en"]; ConfigDescription en = test::ParseConfigOrDie("en"); - ASSERT_THAT(out, ElementsAre(en)); + EXPECT_THAT(out.entry, ElementsAre(en)); + EXPECT_EQ(6, out.order); +} + +TEST_F(ConfigurationParserTest, LocaleGroupAction_EmtpyGroup_NoOrder) { + static constexpr const char* xml = R"xml()xml"; + + auto doc = test::BuildXmlDom(xml); + + PostProcessingConfiguration config; + bool ok = LocaleGroupTagHandler(&config, NodeCast(doc.get()->root.get()), &diag_); + ASSERT_FALSE(ok); } TEST_F(ConfigurationParserTest, LocaleGroupAction_InvalidEmtpyGroup) { @@ -695,7 +736,7 @@ TEST_F(ConfigurationParserTest, AndroidSdkGroupAction_NonNumeric) { TEST_F(ConfigurationParserTest, GlTextureGroupAction) { static constexpr const char* xml = R"xml( - + assets/dxt1/main/* @@ -726,7 +767,7 @@ TEST_F(ConfigurationParserTest, GlTextureGroupAction) { TEST_F(ConfigurationParserTest, DeviceFeatureGroupAction) { static constexpr const char* xml = R"xml( - + android.hardware.audio.low_latency android.hardware.audio.pro @@ -749,6 +790,30 @@ TEST_F(ConfigurationParserTest, DeviceFeatureGroupAction) { ASSERT_THAT(out, ElementsAre(low_latency, pro)); } +TEST_F(ConfigurationParserTest, Group_Valid) { + Group group; + group["item1"].order = 1; + group["item2"].order = 2; + group["item3"].order = 3; + group["item4"].order = 4; + group["item5"].order = 5; + group["item6"].order = 6; + + EXPECT_TRUE(IsGroupValid(group, "test", &diag_)); +} + +TEST_F(ConfigurationParserTest, Group_OverlappingOrder) { + Group group; + group["item1"].order = 1; + group["item2"].order = 2; + group["item3"].order = 3; + group["item4"].order = 2; + group["item5"].order = 5; + group["item6"].order = 1; + + EXPECT_FALSE(IsGroupValid(group, "test", &diag_)); +} + // Artifact name parser test cases. TEST(ArtifactTest, Simple) { diff --git a/tools/aapt2/configuration/aapt2.xsd b/tools/aapt2/configuration/aapt2.xsd index fb2f49bae7b7a..a28e28be48f4b 100644 --- a/tools/aapt2/configuration/aapt2.xsd +++ b/tools/aapt2/configuration/aapt2.xsd @@ -81,6 +81,7 @@ + @@ -95,6 +96,7 @@ + @@ -102,6 +104,7 @@ + @@ -122,6 +125,7 @@ + @@ -158,6 +162,7 @@ + diff --git a/tools/aapt2/configuration/example/config.xml b/tools/aapt2/configuration/example/config.xml index d8aba09e836d8..e6db2a0f461bc 100644 --- a/tools/aapt2/configuration/example/config.xml +++ b/tools/aapt2/configuration/example/config.xml @@ -36,25 +36,19 @@ - + armeabi-v7a arm64-v8a - + x86 mips - - xhdpi - xxhdpi - xxxhdpi - - - + ldpi mdpi hdpi @@ -62,29 +56,35 @@ xxhdpi xxxhdpi + + + xhdpi + xxhdpi + xxxhdpi + - + - + - + - + assets/dxt1/* @@ -92,7 +92,7 @@ - + android.hardware.audio.low_latency