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