diff options
author | Ryan Mitchell <rtmitchell@google.com> | 2018-11-16 11:21:41 -0800 |
---|---|---|
committer | Ryan Mitchell <rtmitchell@google.com> | 2018-12-11 13:48:45 -0800 |
commit | 1bb1fe068a7e719711963c3cf3a50209e083a17f (patch) | |
tree | 70a6d9fbaa6e7f03626b92d345f73b48fcc3fa4a | |
parent | c622083df99a87afef8348dd8e4bdfecf3050d94 (diff) |
Refactor policy parsing
This change removes the ability for an overlayable resource to be
defined in multiple policy blocks within the same overlayable. This
change also changes aapt2 to use a bit mask to keep track of the parsed
policies.
Bug: 110869880
Bug: 120298168
Test: aapt2_tests
Change-Id: Ie26cd913f94a16c0b312f222bccfa48f62feceaa
22 files changed, 412 insertions, 515 deletions
diff --git a/libs/androidfw/include/androidfw/ResourceTypes.h b/libs/androidfw/include/androidfw/ResourceTypes.h index 91261aa3e4f9..cf2d8fb3251c 100644 --- a/libs/androidfw/include/androidfw/ResourceTypes.h +++ b/libs/androidfw/include/androidfw/ResourceTypes.h @@ -1622,7 +1622,7 @@ struct ResTable_overlayable_policy_header { struct ResChunk_header header; - enum PolicyFlags { + enum PolicyFlags : uint32_t { // Any overlay can overlay these resources. POLICY_PUBLIC = 0x00000001, diff --git a/tools/aapt2/Debug.cpp b/tools/aapt2/Debug.cpp index 583f14ac0cbd..9460c9e596e9 100644 --- a/tools/aapt2/Debug.cpp +++ b/tools/aapt2/Debug.cpp @@ -306,31 +306,6 @@ void Debug::PrintTable(const ResourceTable& table, const DebugPrintTableOptions& break; } - for (size_t i = 0; i < entry->overlayable_declarations.size(); i++) { - printer->Print((i == 0) ? " " : "|"); - printer->Print("OVERLAYABLE"); - - if (entry->overlayable_declarations[i].policy) { - switch (entry->overlayable_declarations[i].policy.value()) { - case Overlayable::Policy::kProduct: - printer->Print("_PRODUCT"); - break; - case Overlayable::Policy::kProductServices: - printer->Print("_PRODUCT_SERVICES"); - break; - case Overlayable::Policy::kSystem: - printer->Print("_SYSTEM"); - break; - case Overlayable::Policy::kVendor: - printer->Print("_VENDOR"); - break; - case Overlayable::Policy::kPublic: - printer->Print("_PUBLIC"); - break; - } - } - } - printer->Println(); if (options.show_values) { diff --git a/tools/aapt2/ResourceParser.cpp b/tools/aapt2/ResourceParser.cpp index 4f25e0968c0e..95877045072b 100644 --- a/tools/aapt2/ResourceParser.cpp +++ b/tools/aapt2/ResourceParser.cpp @@ -99,7 +99,7 @@ struct ParsedResource { ResourceId id; Visibility::Level visibility_level = Visibility::Level::kUndefined; bool allow_new = false; - std::vector<Overlayable> overlayable_declarations; + Maybe<Overlayable> overlayable; std::string comment; std::unique_ptr<Value> value; @@ -133,8 +133,8 @@ static bool AddResourcesToTable(ResourceTable* table, IDiagnostics* diag, Parsed } } - for (auto& overlayable : res->overlayable_declarations) { - if (!table->AddOverlayable(res->name, overlayable, diag)) { + if (res->overlayable) { + if (!table->SetOverlayable(res->name, res->overlayable.value(), diag)) { return false; } } @@ -1063,20 +1063,19 @@ bool ResourceParser::ParseOverlayable(xml::XmlPullParser* parser, ParsedResource << "' for <overlayable> tag"); } - std::string comment; - std::vector<Overlayable::Policy> policies; - bool error = false; + std::string comment; + Overlayable::PolicyFlags current_policies = Overlayable::Policy::kNone; const size_t start_depth = parser->depth(); while (xml::XmlPullParser::IsGoodEvent(parser->Next())) { xml::XmlPullParser::Event event = parser->event(); if (event == xml::XmlPullParser::Event::kEndElement && parser->depth() == start_depth) { - // Break the loop when exiting the overyabale element + // Break the loop when exiting the overlayable element break; } else if (event == xml::XmlPullParser::Event::kEndElement && parser->depth() == start_depth + 1) { // Clear the current policies when exiting the policy element - policies.clear(); + current_policies = Overlayable::Policy::kNone; continue; } else if (event == xml::XmlPullParser::Event::kComment) { // Get the comment of individual item elements @@ -1090,43 +1089,71 @@ bool ResourceParser::ParseOverlayable(xml::XmlPullParser* parser, ParsedResource const Source item_source = source_.WithLine(parser->line_number()); const std::string& element_name = parser->element_name(); const std::string& element_namespace = parser->element_namespace(); - if (element_namespace.empty() && element_name == "item") { - if (!ParseOverlayableItem(parser, policies, comment, out_resource)) { + // Items specify the name and type of resource that should be overlayable + Maybe<StringPiece> maybe_name = xml::FindNonEmptyAttribute(parser, "name"); + if (!maybe_name) { + diag_->Error(DiagMessage(item_source) + << "<item> within an <overlayable> tag must have a 'name' attribute"); + error = true; + continue; + } + + Maybe<StringPiece> maybe_type = xml::FindNonEmptyAttribute(parser, "type"); + if (!maybe_type) { + diag_->Error(DiagMessage(item_source) + << "<item> within an <overlayable> tag must have a 'type' attribute"); error = true; + continue; + } + + const ResourceType* type = ParseResourceType(maybe_type.value()); + if (type == nullptr) { + diag_->Error(DiagMessage(item_source) + << "invalid resource type '" << maybe_type.value() + << "' in <item> within an <overlayable>"); + error = true; + continue; } + + ParsedResource child_resource; + child_resource.name.type = *type; + child_resource.name.entry = maybe_name.value().to_string(); + child_resource.overlayable = Overlayable{current_policies, item_source, comment}; + out_resource->child_resources.push_back(std::move(child_resource)); + } else if (element_namespace.empty() && element_name == "policy") { - if (!policies.empty()) { + if (current_policies != Overlayable::Policy::kNone) { // If the policy list is not empty, then we are currently inside a policy element diag_->Error(DiagMessage(item_source) << "<policy> blocks cannot be recursively nested"); error = true; break; } else if (Maybe<StringPiece> maybe_type = xml::FindNonEmptyAttribute(parser, "type")) { // Parse the polices separated by vertical bar characters to allow for specifying multiple - // policies at once + // policies for (StringPiece part : util::Tokenize(maybe_type.value(), '|')) { StringPiece trimmed_part = util::TrimWhitespace(part); if (trimmed_part == "public") { - policies.push_back(Overlayable::Policy::kPublic); + current_policies |= Overlayable::Policy::kPublic; } else if (trimmed_part == "product") { - policies.push_back(Overlayable::Policy::kProduct); + current_policies |= Overlayable::Policy::kProduct; } else if (trimmed_part == "product_services") { - policies.push_back(Overlayable::Policy::kProductServices); + current_policies |= Overlayable::Policy::kProductServices; } else if (trimmed_part == "system") { - policies.push_back(Overlayable::Policy::kSystem); + current_policies |= Overlayable::Policy::kSystem; } else if (trimmed_part == "vendor") { - policies.push_back(Overlayable::Policy::kVendor); + current_policies |= Overlayable::Policy::kVendor; } else { - diag_->Error(DiagMessage(out_resource->source) - << "<policy> has unsupported type '" << trimmed_part << "'"); + diag_->Error(DiagMessage(item_source) + << "<policy> has unsupported type '" << trimmed_part << "'"); error = true; continue; } } } } else if (!ShouldIgnoreElement(element_namespace, element_name)) { - diag_->Error(DiagMessage(item_source) << "invalid element <" << element_name << "> in " - << " <overlayable>"); + diag_->Error(DiagMessage(item_source) << "invalid element <" << element_name << "> " + << " in <overlayable>"); error = true; break; } @@ -1135,61 +1162,6 @@ bool ResourceParser::ParseOverlayable(xml::XmlPullParser* parser, ParsedResource return !error; } -bool ResourceParser::ParseOverlayableItem(xml::XmlPullParser* parser, - const std::vector<Overlayable::Policy>& policies, - const std::string& comment, - ParsedResource* out_resource) { - const Source item_source = source_.WithLine(parser->line_number()); - - Maybe<StringPiece> maybe_name = xml::FindNonEmptyAttribute(parser, "name"); - if (!maybe_name) { - diag_->Error(DiagMessage(item_source) - << "<item> within an <overlayable> tag must have a 'name' attribute"); - return false; - } - - Maybe<StringPiece> maybe_type = xml::FindNonEmptyAttribute(parser, "type"); - if (!maybe_type) { - diag_->Error(DiagMessage(item_source) - << "<item> within an <overlayable> tag must have a 'type' attribute"); - return false; - } - - const ResourceType* type = ParseResourceType(maybe_type.value()); - if (type == nullptr) { - diag_->Error(DiagMessage(out_resource->source) - << "invalid resource type '" << maybe_type.value() - << "' in <item> within an <overlayable>"); - return false; - } - - ParsedResource child_resource; - child_resource.name.type = *type; - child_resource.name.entry = maybe_name.value().to_string(); - child_resource.source = item_source; - - if (policies.empty()) { - Overlayable overlayable; - overlayable.source = item_source; - overlayable.comment = comment; - child_resource.overlayable_declarations.push_back(overlayable); - } else { - for (Overlayable::Policy policy : policies) { - Overlayable overlayable; - overlayable.policy = policy; - overlayable.source = item_source; - overlayable.comment = comment; - child_resource.overlayable_declarations.push_back(overlayable); - } - } - - if (options_.visibility) { - child_resource.visibility_level = options_.visibility.value(); - } - out_resource->child_resources.push_back(std::move(child_resource)); - return true; -} - bool ResourceParser::ParseAddResource(xml::XmlPullParser* parser, ParsedResource* out_resource) { if (ParseSymbolImpl(parser, out_resource)) { out_resource->visibility_level = Visibility::Level::kUndefined; diff --git a/tools/aapt2/ResourceParser.h b/tools/aapt2/ResourceParser.h index ebacd6f1280e..06bb0c9cf264 100644 --- a/tools/aapt2/ResourceParser.h +++ b/tools/aapt2/ResourceParser.h @@ -96,10 +96,6 @@ class ResourceParser { bool ParseSymbolImpl(xml::XmlPullParser* parser, ParsedResource* out_resource); bool ParseSymbol(xml::XmlPullParser* parser, ParsedResource* out_resource); bool ParseOverlayable(xml::XmlPullParser* parser, ParsedResource* out_resource); - bool ParseOverlayableItem(xml::XmlPullParser* parser, - const std::vector<Overlayable::Policy>& policies, - const std::string& comment, - ParsedResource* out_resource); bool ParseAddResource(xml::XmlPullParser* parser, ParsedResource* out_resource); bool ParseAttr(xml::XmlPullParser* parser, ParsedResource* out_resource); bool ParseAttrImpl(xml::XmlPullParser* parser, ParsedResource* out_resource, bool weak); diff --git a/tools/aapt2/ResourceParser_test.cpp b/tools/aapt2/ResourceParser_test.cpp index c6f29ac53ca6..03e6197027cb 100644 --- a/tools/aapt2/ResourceParser_test.cpp +++ b/tools/aapt2/ResourceParser_test.cpp @@ -905,16 +905,16 @@ TEST_F(ResourceParserTest, ParseOverlayable) { auto search_result = table_.FindResource(test::ParseNameOrDie("string/foo")); ASSERT_TRUE(search_result); ASSERT_THAT(search_result.value().entry, NotNull()); - EXPECT_THAT(search_result.value().entry->visibility.level, Eq(Visibility::Level::kUndefined)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations.size(), Eq(1)); - EXPECT_FALSE(search_result.value().entry->overlayable_declarations[0].policy); + ASSERT_TRUE(search_result.value().entry->overlayable); + EXPECT_THAT(search_result.value().entry->overlayable.value().policies, + Eq(Overlayable::Policy::kNone)); search_result = table_.FindResource(test::ParseNameOrDie("drawable/bar")); ASSERT_TRUE(search_result); ASSERT_THAT(search_result.value().entry, NotNull()); - EXPECT_THAT(search_result.value().entry->visibility.level, Eq(Visibility::Level::kUndefined)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations.size(), Eq(1)); - EXPECT_FALSE(search_result.value().entry->overlayable_declarations[0].policy); + ASSERT_TRUE(search_result.value().entry->overlayable); + EXPECT_THAT(search_result.value().entry->overlayable.value().policies, + Eq(Overlayable::Policy::kNone)); } TEST_F(ResourceParserTest, ParseOverlayablePolicy) { @@ -945,49 +945,44 @@ TEST_F(ResourceParserTest, ParseOverlayablePolicy) { auto search_result = table_.FindResource(test::ParseNameOrDie("string/foo")); ASSERT_TRUE(search_result); ASSERT_THAT(search_result.value().entry, NotNull()); - EXPECT_THAT(search_result.value().entry->visibility.level, Eq(Visibility::Level::kUndefined)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations.size(), Eq(1)); - EXPECT_FALSE(search_result.value().entry->overlayable_declarations[0].policy); + ASSERT_TRUE(search_result.value().entry->overlayable); + Overlayable& overlayable = search_result.value().entry->overlayable.value(); + EXPECT_THAT(overlayable.policies, Eq(Overlayable::Policy::kNone)); search_result = table_.FindResource(test::ParseNameOrDie("string/bar")); ASSERT_TRUE(search_result); ASSERT_THAT(search_result.value().entry, NotNull()); - EXPECT_THAT(search_result.value().entry->visibility.level, Eq(Visibility::Level::kUndefined)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations.size(), Eq(1)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations[0].policy, - Eq(Overlayable::Policy::kProduct)); + ASSERT_TRUE(search_result.value().entry->overlayable); + overlayable = search_result.value().entry->overlayable.value(); + EXPECT_THAT(overlayable.policies, Eq(Overlayable::Policy::kProduct)); search_result = table_.FindResource(test::ParseNameOrDie("string/baz")); ASSERT_TRUE(search_result); ASSERT_THAT(search_result.value().entry, NotNull()); - EXPECT_THAT(search_result.value().entry->visibility.level, Eq(Visibility::Level::kUndefined)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations.size(), Eq(1)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations[0].policy, - Eq(Overlayable::Policy::kProductServices)); + ASSERT_TRUE(search_result.value().entry->overlayable); + overlayable = search_result.value().entry->overlayable.value(); + EXPECT_THAT(overlayable.policies, Eq(Overlayable::Policy::kProductServices)); search_result = table_.FindResource(test::ParseNameOrDie("string/fiz")); ASSERT_TRUE(search_result); ASSERT_THAT(search_result.value().entry, NotNull()); - EXPECT_THAT(search_result.value().entry->visibility.level, Eq(Visibility::Level::kUndefined)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations.size(), Eq(1)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations[0].policy, - Eq(Overlayable::Policy::kSystem)); + ASSERT_TRUE(search_result.value().entry->overlayable); + overlayable = search_result.value().entry->overlayable.value(); + EXPECT_THAT(overlayable.policies, Eq(Overlayable::Policy::kSystem)); search_result = table_.FindResource(test::ParseNameOrDie("string/fuz")); ASSERT_TRUE(search_result); ASSERT_THAT(search_result.value().entry, NotNull()); - EXPECT_THAT(search_result.value().entry->visibility.level, Eq(Visibility::Level::kUndefined)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations.size(), Eq(1)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations[0].policy, - Eq(Overlayable::Policy::kVendor)); + ASSERT_TRUE(search_result.value().entry->overlayable); + overlayable = search_result.value().entry->overlayable.value(); + EXPECT_THAT(overlayable.policies, Eq(Overlayable::Policy::kVendor)); search_result = table_.FindResource(test::ParseNameOrDie("string/faz")); ASSERT_TRUE(search_result); ASSERT_THAT(search_result.value().entry, NotNull()); - EXPECT_THAT(search_result.value().entry->visibility.level, Eq(Visibility::Level::kUndefined)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations.size(), Eq(1)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations[0].policy, - Eq(Overlayable::Policy::kPublic)); + ASSERT_TRUE(search_result.value().entry->overlayable); + overlayable = search_result.value().entry->overlayable.value(); + EXPECT_THAT(overlayable.policies, Eq(Overlayable::Policy::kPublic)); } TEST_F(ResourceParserTest, ParseOverlayableBadPolicyError) { @@ -1031,22 +1026,18 @@ TEST_F(ResourceParserTest, ParseOverlayableMultiplePolicy) { auto search_result = table_.FindResource(test::ParseNameOrDie("string/foo")); ASSERT_TRUE(search_result); ASSERT_THAT(search_result.value().entry, NotNull()); - EXPECT_THAT(search_result.value().entry->visibility.level, Eq(Visibility::Level::kUndefined)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations.size(), Eq(2)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations[0].policy, - Eq(Overlayable::Policy::kVendor)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations[1].policy, - Eq(Overlayable::Policy::kProductServices)); + ASSERT_TRUE(search_result.value().entry->overlayable); + Overlayable& overlayable = search_result.value().entry->overlayable.value(); + EXPECT_THAT(overlayable.policies, Eq(Overlayable::Policy::kVendor + | Overlayable::Policy::kProductServices)); search_result = table_.FindResource(test::ParseNameOrDie("string/bar")); ASSERT_TRUE(search_result); ASSERT_THAT(search_result.value().entry, NotNull()); - EXPECT_THAT(search_result.value().entry->visibility.level, Eq(Visibility::Level::kUndefined)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations.size(), Eq(2)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations[0].policy, - Eq(Overlayable::Policy::kProduct)); - EXPECT_THAT(search_result.value().entry->overlayable_declarations[1].policy, - Eq(Overlayable::Policy::kSystem)); + ASSERT_TRUE(search_result.value().entry->overlayable); + overlayable = search_result.value().entry->overlayable.value(); + EXPECT_THAT(overlayable.policies, Eq(Overlayable::Policy::kProduct + | Overlayable::Policy::kSystem)); } TEST_F(ResourceParserTest, DuplicateOverlayableIsError) { @@ -1067,7 +1058,7 @@ TEST_F(ResourceParserTest, DuplicateOverlayableIsError) { EXPECT_FALSE(TestParse(input)); input = R"( - <overlayable"> + <overlayable> <policy type="product"> <item type="string" name="foo" /> <item type="string" name="foo" /> @@ -1080,45 +1071,30 @@ TEST_F(ResourceParserTest, DuplicateOverlayableIsError) { <policy type="product"> <item type="string" name="foo" /> </policy> - </overlayable> + <item type="string" name="foo" /> + </overlayable>)"; + EXPECT_FALSE(TestParse(input)); + input = R"( <overlayable> <policy type="product"> <item type="string" name="foo" /> </policy> - </overlayable>)"; - EXPECT_FALSE(TestParse(input)); -} - -TEST_F(ResourceParserTest, PolicyAndNonPolicyOverlayableError) { - std::string input = R"( - <overlayable policy="product"> - <item type="string" name="foo" /> - </overlayable> - <overlayable policy=""> + <policy type="vendor"> <item type="string" name="foo" /> - </overlayable>)"; + </policy> + </overlayable>)"; EXPECT_FALSE(TestParse(input)); input = R"( - <overlayable policy=""> - <item type="string" name="foo" /> - </overlayable> - <overlayable policy="product"> - <item type="string" name="foo" /> - </overlayable>)"; - EXPECT_FALSE(TestParse(input)); -} - -TEST_F(ResourceParserTest, DuplicateOverlayableMultiplePolicyError) { - std::string input = R"( <overlayable> - <policy type="vendor|product"> + <policy type="product"> <item type="string" name="foo" /> </policy> </overlayable> + <overlayable> - <policy type="product_services|vendor"> + <policy type="product"> <item type="string" name="foo" /> </policy> </overlayable>)"; diff --git a/tools/aapt2/ResourceTable.cpp b/tools/aapt2/ResourceTable.cpp index bc8a4d1f85b8..54633ad5c5e3 100644 --- a/tools/aapt2/ResourceTable.cpp +++ b/tools/aapt2/ResourceTable.cpp @@ -625,18 +625,18 @@ bool ResourceTable::SetAllowNewImpl(const ResourceNameRef& name, const AllowNew& return true; } -bool ResourceTable::AddOverlayable(const ResourceNameRef& name, const Overlayable& overlayable, +bool ResourceTable::SetOverlayable(const ResourceNameRef& name, const Overlayable& overlayable, IDiagnostics* diag) { - return AddOverlayableImpl(name, overlayable, ResourceNameValidator, diag); + return SetOverlayableImpl(name, overlayable, ResourceNameValidator, diag); } -bool ResourceTable::AddOverlayableMangled(const ResourceNameRef& name, +bool ResourceTable::SetOverlayableMangled(const ResourceNameRef& name, const Overlayable& overlayable, IDiagnostics* diag) { - return AddOverlayableImpl(name, overlayable, SkipNameValidator, diag); + return SetOverlayableImpl(name, overlayable, SkipNameValidator, diag); } -bool ResourceTable::AddOverlayableImpl(const ResourceNameRef& name, const Overlayable& overlayable, - NameValidator name_validator, IDiagnostics* diag) { +bool ResourceTable::SetOverlayableImpl(const ResourceNameRef& name, const Overlayable& overlayable, + NameValidator name_validator, IDiagnostics *diag) { CHECK(diag != nullptr); if (!ValidateName(name_validator, name, overlayable.source, diag)) { @@ -647,27 +647,14 @@ bool ResourceTable::AddOverlayableImpl(const ResourceNameRef& name, const Overla ResourceTableType* type = package->FindOrCreateType(name.type); ResourceEntry* entry = type->FindOrCreateEntry(name.entry); - for (auto& overlayable_declaration : entry->overlayable_declarations) { - // An overlayable resource cannot be declared twice with the same policy - if (overlayable.policy == overlayable_declaration.policy) { - diag->Error(DiagMessage(overlayable.source) + if (entry->overlayable) { + diag->Error(DiagMessage(overlayable.source) << "duplicate overlayable declaration for resource '" << name << "'"); - diag->Error(DiagMessage(overlayable_declaration.source) << "previous declaration here"); - return false; - } - - // An overlayable resource cannot be declared once with a policy and without a policy because - // the policy becomes unused - if (!overlayable.policy || !overlayable_declaration.policy) { - diag->Error(DiagMessage(overlayable.source) - << "overlayable resource '" << name << "'" - << " declared once with a policy and once with no policy"); - diag->Error(DiagMessage(overlayable_declaration.source) << "previous declaration here"); - return false; - } + diag->Error(DiagMessage(entry->overlayable.value().source) << "previous declaration here"); + return false; } - entry->overlayable_declarations.push_back(overlayable); + entry->overlayable = overlayable; return true; } @@ -703,7 +690,7 @@ std::unique_ptr<ResourceTable> ResourceTable::Clone() const { new_entry->id = entry->id; new_entry->visibility = entry->visibility; new_entry->allow_new = entry->allow_new; - new_entry->overlayable_declarations = entry->overlayable_declarations; + new_entry->overlayable = entry->overlayable; for (const auto& config_value : entry->values) { ResourceConfigValue* new_value = diff --git a/tools/aapt2/ResourceTable.h b/tools/aapt2/ResourceTable.h index 3dd0a769d944..e646f5be43c7 100644 --- a/tools/aapt2/ResourceTable.h +++ b/tools/aapt2/ResourceTable.h @@ -57,27 +57,32 @@ struct AllowNew { std::string comment; }; -// Represents a declaration that a resource is overayable at runtime. +// Represents a declaration that a resource is overlayable at runtime. struct Overlayable { + // Represents the types overlays that are allowed to overlay the resource. - enum class Policy { + enum Policy : uint32_t { + kNone = 0x00, + // The resource can be overlaid by any overlay. - kPublic, + kPublic = 0x01, // The resource can be overlaid by any overlay on the system partition. - kSystem, + kSystem = 0x02, // The resource can be overlaid by any overlay on the vendor partition. - kVendor, + kVendor = 0x04, // The resource can be overlaid by any overlay on the product partition. - kProduct, + kProduct = 0x08, // The resource can be overlaid by any overlay on the product services partition. - kProductServices, + kProductServices = 0x10 }; - Maybe<Policy> policy; + typedef uint32_t PolicyFlags; + PolicyFlags policies = Policy::kNone; + Source source; std::string comment; }; @@ -116,7 +121,7 @@ class ResourceEntry { Maybe<AllowNew> allow_new; // The declarations of this resource as overlayable for RROs - std::vector<Overlayable> overlayable_declarations; + Maybe<Overlayable> overlayable; // The resource's values for each configuration. std::vector<std::unique_ptr<ResourceConfigValue>> values; @@ -246,9 +251,9 @@ class ResourceTable { bool SetVisibilityWithIdMangled(const ResourceNameRef& name, const Visibility& visibility, const ResourceId& res_id, IDiagnostics* diag); - bool AddOverlayable(const ResourceNameRef& name, const Overlayable& overlayable, - IDiagnostics* diag); - bool AddOverlayableMangled(const ResourceNameRef& name, const Overlayable& overlayable, + bool SetOverlayable(const ResourceNameRef& name, const Overlayable& overlayable, + IDiagnostics *diag); + bool SetOverlayableMangled(const ResourceNameRef& name, const Overlayable& overlayable, IDiagnostics* diag); bool SetAllowNew(const ResourceNameRef& name, const AllowNew& allow_new, IDiagnostics* diag); @@ -323,8 +328,8 @@ class ResourceTable { bool SetAllowNewImpl(const ResourceNameRef& name, const AllowNew& allow_new, NameValidator name_validator, IDiagnostics* diag); - bool AddOverlayableImpl(const ResourceNameRef& name, const Overlayable& overlayable, - NameValidator name_validator, IDiagnostics* diag); + bool SetOverlayableImpl(const ResourceNameRef &name, const Overlayable &overlayable, + NameValidator name_validator, IDiagnostics *diag); bool SetSymbolStateImpl(const ResourceNameRef& name, const ResourceId& res_id, const Visibility& symbol, NameValidator name_validator, diff --git a/tools/aapt2/ResourceTable_test.cpp b/tools/aapt2/ResourceTable_test.cpp index 7c28f07d0f66..31095c4d88c8 100644 --- a/tools/aapt2/ResourceTable_test.cpp +++ b/tools/aapt2/ResourceTable_test.cpp @@ -242,69 +242,50 @@ TEST(ResourceTableTest, SetAllowNew) { ASSERT_THAT(result.value().entry->allow_new.value().comment, StrEq("second")); } -TEST(ResourceTableTest, AddOverlayable) { +TEST(ResourceTableTest, SetOverlayable) { ResourceTable table; - const ResourceName name = test::ParseNameOrDie("android:string/foo"); - - Overlayable overlayable; - overlayable.policy = Overlayable::Policy::kProduct; - overlayable.comment = "first"; - ASSERT_TRUE(table.AddOverlayable(name, overlayable, test::GetDiagnostics())); - Maybe<ResourceTable::SearchResult> result = table.FindResource(name); - ASSERT_TRUE(result); - ASSERT_THAT(result.value().entry->overlayable_declarations.size(), Eq(1)); - ASSERT_THAT(result.value().entry->overlayable_declarations[0].comment, StrEq("first")); - ASSERT_THAT(result.value().entry->overlayable_declarations[0].policy, - Eq(Overlayable::Policy::kProduct)); - - Overlayable overlayable2; - overlayable2.comment = "second"; - overlayable2.policy = Overlayable::Policy::kProductServices; - ASSERT_TRUE(table.AddOverlayable(name, overlayable2, test::GetDiagnostics())); - result = table.FindResource(name); - ASSERT_TRUE(result); - ASSERT_THAT(result.value().entry->overlayable_declarations.size(), Eq(2)); - ASSERT_THAT(result.value().entry->overlayable_declarations[0].comment, StrEq("first")); - ASSERT_THAT(result.value().entry->overlayable_declarations[0].policy, - Eq(Overlayable::Policy::kProduct)); - ASSERT_THAT(result.value().entry->overlayable_declarations[1].comment, StrEq("second")); - ASSERT_THAT(result.value().entry->overlayable_declarations[1].policy, - Eq(Overlayable::Policy::kProductServices)); -} + Overlayable overlayable{}; + overlayable.policies |= Overlayable::Policy::kProduct; + overlayable.policies |= Overlayable::Policy::kProductServices; + overlayable.comment = "comment"; -TEST(ResourceTableTest, AddDuplicateOverlayableFail) { - ResourceTable table; const ResourceName name = test::ParseNameOrDie("android:string/foo"); + ASSERT_TRUE(table.SetOverlayable(name, overlayable, test::GetDiagnostics())); + Maybe<ResourceTable::SearchResult> search_result = table.FindResource(name); - Overlayable overlayable; - overlayable.policy = Overlayable::Policy::kProduct; - ASSERT_TRUE(table.AddOverlayable(name, overlayable, test::GetDiagnostics())); + ASSERT_TRUE(search_result); + ASSERT_TRUE(search_result.value().entry->overlayable); - Overlayable overlayable2; - overlayable2.policy = Overlayable::Policy::kProduct; - ASSERT_FALSE(table.AddOverlayable(name, overlayable2, test::GetDiagnostics())); + Overlayable& result_overlayable = search_result.value().entry->overlayable.value(); + ASSERT_THAT(result_overlayable.comment, StrEq("comment")); + EXPECT_THAT(result_overlayable.policies, Eq(Overlayable::Policy::kProduct + | Overlayable::Policy::kProductServices)); } -TEST(ResourceTableTest, AddOverlayablePolicyAndNoneFirstFail) { +TEST(ResourceTableTest, AddDuplicateOverlayableSamePolicyFail) { ResourceTable table; const ResourceName name = test::ParseNameOrDie("android:string/foo"); - ASSERT_TRUE(table.AddOverlayable(name, {}, test::GetDiagnostics())); + Overlayable overlayable{}; + overlayable.policies = Overlayable::Policy::kProduct; + ASSERT_TRUE(table.SetOverlayable(name, overlayable, test::GetDiagnostics())); - Overlayable overlayable2; - overlayable2.policy = Overlayable::Policy::kProduct; - ASSERT_FALSE(table.AddOverlayable(name, overlayable2, test::GetDiagnostics())); + Overlayable overlayable2{}; + overlayable2.policies = Overlayable::Policy::kProduct; + ASSERT_FALSE(table.SetOverlayable(name, overlayable2, test::GetDiagnostics())); } -TEST(ResourceTableTest, AddOverlayablePolicyAndNoneLastFail) { +TEST(ResourceTableTest, AddDuplicateOverlayableDifferentPolicyFail) { ResourceTable table; const ResourceName name = test::ParseNameOrDie("android:string/foo"); - Overlayable overlayable; - overlayable.policy = Overlayable::Policy::kProduct; - ASSERT_TRUE(table.AddOverlayable(name, overlayable, test::GetDiagnostics())); + Overlayable overlayable{}; + overlayable.policies = Overlayable::Policy::kProduct; + ASSERT_TRUE(table.SetOverlayable(name, overlayable, test::GetDiagnostics())); - ASSERT_FALSE(table.AddOverlayable(name, {}, test::GetDiagnostics())); + Overlayable overlayable2{}; + overlayable2.policies = Overlayable::Policy::kVendor; + ASSERT_FALSE(table.SetOverlayable(name, overlayable2, test::GetDiagnostics())); } TEST(ResourceTableTest, AllowDuplictaeResourcesNames) { diff --git a/tools/aapt2/Resources.proto b/tools/aapt2/Resources.proto index bf9fe49da2d6..81a2c2e5cc02 100644 --- a/tools/aapt2/Resources.proto +++ b/tools/aapt2/Resources.proto @@ -136,12 +136,11 @@ message AllowNew { // Represents a declaration that a resource is overayable at runtime. message Overlayable { enum Policy { - NONE = 0; - PUBLIC = 1; - SYSTEM = 2; - VENDOR = 3; - PRODUCT = 4; - PRODUCT_SERVICES = 5; + PUBLIC = 0; + SYSTEM = 1; + VENDOR = 2; + PRODUCT = 3; + PRODUCT_SERVICES = 4; } // Where this declaration was defined in source. @@ -150,8 +149,8 @@ message Overlayable { // Any comment associated with the declaration. string comment = 2; - // The policy of the overlayable declaration - Policy policy = 3; + // The policy defined in the overlayable declaration. + repeated Policy policy = 3; } // An entry ID in the range [0x0000, 0xffff]. @@ -181,7 +180,7 @@ message Entry { AllowNew allow_new = 4; // Whether this resource can be overlaid by a runtime resource overlay (RRO). - repeated Overlayable overlayable = 5; + Overlayable overlayable = 5; // The set of values defined for this entry, each corresponding to a different // configuration/variant. diff --git a/tools/aapt2/cmd/Dump.h b/tools/aapt2/cmd/Dump.h index 89d19cf4ba08..5cf056e60640 100644 --- a/tools/aapt2/cmd/Dump.h +++ b/tools/aapt2/cmd/Dump.h @@ -255,6 +255,7 @@ class DumpCommand : public Command { AddOptionalSubcommand(util::make_unique<DumpXmlStringsCommand>(printer, diag_)); AddOptionalSubcommand(util::make_unique<DumpXmlTreeCommand>(printer, diag_)); AddOptionalSubcommand(util::make_unique<DumpBadgerCommand>(printer), /* hidden */ true); + // TODO(b/120609160): Add aapt2 overlayable dump command } int Action(const std::vector<std::string>& args) override { diff --git a/tools/aapt2/format/binary/BinaryResourceParser.cpp b/tools/aapt2/format/binary/BinaryResourceParser.cpp index df0daebe8453..61ebd4ee26ca 100644 --- a/tools/aapt2/format/binary/BinaryResourceParser.cpp +++ b/tools/aapt2/format/binary/BinaryResourceParser.cpp @@ -441,25 +441,25 @@ bool BinaryResourceParser::ParseOverlayable(const ResChunk_header* chunk) { const ResTable_overlayable_policy_header* policy_header = ConvertTo<ResTable_overlayable_policy_header>(parser.chunk()); - std::vector<Overlayable::Policy> policies; + Overlayable::PolicyFlags policies = Overlayable::Policy::kNone; if (policy_header->policy_flags & ResTable_overlayable_policy_header::POLICY_PUBLIC) { - policies.push_back(Overlayable::Policy::kPublic); + policies |= Overlayable::Policy::kPublic; } if (policy_header->policy_flags & ResTable_overlayable_policy_header::POLICY_SYSTEM_PARTITION) { - policies.push_back(Overlayable::Policy::kSystem); + policies |= Overlayable::Policy::kSystem; } if (policy_header->policy_flags & ResTable_overlayable_policy_header::POLICY_VENDOR_PARTITION) { - policies.push_back(Overlayable::Policy::kVendor); + policies |= Overlayable::Policy::kVendor; } if (policy_header->policy_flags & ResTable_overlayable_policy_header::POLICY_PRODUCT_PARTITION) { - policies.push_back(Overlayable::Policy::kProduct); + policies |= Overlayable::Policy::kProduct; } if (policy_header->policy_flags & ResTable_overlayable_policy_header::POLICY_PRODUCT_SERVICES_PARTITION) { - policies.push_back(Overlayable::Policy::kProductServices); + policies |= Overlayable::Policy::kProductServices; } const ResTable_ref* const ref_begin = reinterpret_cast<const ResTable_ref*>( @@ -478,13 +478,11 @@ bool BinaryResourceParser::ParseOverlayable(const ResChunk_header* chunk) { return false; } - for (Overlayable::Policy policy : policies) { - Overlayable overlayable; - overlayable.source = source_.WithLine(0); - overlayable.policy = policy; - if (!table_->AddOverlayable(iter->second, overlayable, diag_)) { - return false; - } + Overlayable overlayable{}; + overlayable.source = source_.WithLine(0); + overlayable.policies = policies; + if (!table_->SetOverlayable(iter->second, overlayable, diag_)) { + return false; } } } diff --git a/tools/aapt2/format/binary/TableFlattener.cpp b/tools/aapt2/format/binary/TableFlattener.cpp index 976c3288bfca..200e2d468500 100644 --- a/tools/aapt2/format/binary/TableFlattener.cpp +++ b/tools/aapt2/format/binary/TableFlattener.cpp @@ -429,56 +429,52 @@ class PackageFlattener { CHECK(bool(type->id)) << "type must have an ID set when flattening <overlayable>"; for (auto& entry : type->entries) { CHECK(bool(type->id)) << "entry must have an ID set when flattening <overlayable>"; + if (!entry->overlayable) { + continue; + } - // TODO(b/120298168): Convert the policies vector to a policy set or bitmask - if (!entry->overlayable_declarations.empty()) { - uint16_t policy_flags = 0; - for (Overlayable overlayable : entry->overlayable_declarations) { - if (overlayable.policy) { - switch (overlayable.policy.value()) { - case Overlayable::Policy::kPublic: - policy_flags |= ResTable_overlayable_policy_header::POLICY_PUBLIC; - break; - case Overlayable::Policy::kSystem: - policy_flags |= ResTable_overlayable_policy_header::POLICY_SYSTEM_PARTITION; - break; - case Overlayable::Policy::kVendor: - policy_flags |= ResTable_overlayable_policy_header::POLICY_VENDOR_PARTITION; - break; - case Overlayable::Policy::kProduct: - policy_flags |= ResTable_overlayable_policy_header::POLICY_PRODUCT_PARTITION; - break; - case Overlayable::Policy::kProductServices: - policy_flags |= - ResTable_overlayable_policy_header::POLICY_PRODUCT_SERVICES_PARTITION; - break; - } - } else { - // Encode overlayable entries defined without a policy as publicly overlayable - policy_flags |= ResTable_overlayable_policy_header::POLICY_PUBLIC; - } - } + Overlayable overlayable = entry->overlayable.value(); + uint32_t policy_flags = Overlayable::Policy::kNone; + if (overlayable.policies & Overlayable::Policy::kPublic) { + policy_flags |= ResTable_overlayable_policy_header::POLICY_PUBLIC; + } + if (overlayable.policies & Overlayable::Policy::kSystem) { + policy_flags |= ResTable_overlayable_policy_header::POLICY_SYSTEM_PARTITION; + } + if (overlayable.policies & Overlayable::Policy::kVendor) { + policy_flags |= ResTable_overlayable_policy_header::POLICY_VENDOR_PARTITION; + } + if (overlayable.policies & Overlayable::Policy::kProduct) { + policy_flags |= ResTable_overlayable_policy_header::POLICY_PRODUCT_PARTITION; + } + if (overlayable.policies & Overlayable::Policy::kProductServices) { + policy_flags |= ResTable_overlayable_policy_header::POLICY_PRODUCT_SERVICES_PARTITION; + } - // Find the overlayable policy chunk with the same policies as the entry - PolicyChunk* policy_chunk = nullptr; - for (PolicyChunk& policy : policies) { - if (policy.policy_flags == policy_flags) { - policy_chunk = &policy; - break; - } - } + if (overlayable.policies == Overlayable::Policy::kNone) { + // Encode overlayable entries defined without a policy as publicly overlayable + policy_flags |= ResTable_overlayable_policy_header::POLICY_PUBLIC; + } - // Create a new policy chunk if an existing one with the same policy cannot be found - if (policy_chunk == nullptr) { - PolicyChunk p; - p.policy_flags = policy_flags; - policies.push_back(p); - policy_chunk = &policies.back(); + // Find the overlayable policy chunk with the same policies as the entry + PolicyChunk* policy_chunk = nullptr; + for (PolicyChunk& policy : policies) { + if (policy.policy_flags == policy_flags) { + policy_chunk = &policy; + break; } + } - policy_chunk->ids.insert(android::make_resid(package_->id.value(), type->id.value(), - entry->id.value())); + // Create a new policy chunk if an existing one with the same policy cannot be found + if (policy_chunk == nullptr) { + PolicyChunk p; + p.policy_flags = policy_flags; + policies.push_back(p); + policy_chunk = &policies.back(); } + + policy_chunk->ids.insert(android::make_resid(package_->id.value(), type->id.value(), + entry->id.value())); } } diff --git a/tools/aapt2/format/binary/TableFlattener_test.cpp b/tools/aapt2/format/binary/TableFlattener_test.cpp index 410efbe83b1b..e99ab1f37761 100644 --- a/tools/aapt2/format/binary/TableFlattener_test.cpp +++ b/tools/aapt2/format/binary/TableFlattener_test.cpp @@ -628,14 +628,17 @@ TEST_F(TableFlattenerTest, ObfuscatingResourceNamesWithWhitelistSucceeds) { } TEST_F(TableFlattenerTest, FlattenOverlayable) { + Overlayable overlayable{}; + overlayable.policies |= Overlayable::Policy::kProduct; + overlayable.policies |= Overlayable::Policy::kSystem; + overlayable.policies |= Overlayable::Policy::kVendor; + std::string name = "com.app.test:integer/overlayable"; std::unique_ptr<ResourceTable> table = test::ResourceTableBuilder() .SetPackageId("com.app.test", 0x7f) .AddSimple(name, ResourceId(0x7f020000)) - .AddOverlayable(name, Overlayable::Policy::kProduct) - .AddOverlayable(name, Overlayable::Policy::kSystem) - .AddOverlayable(name, Overlayable::Policy::kVendor) + .SetOverlayable(name, overlayable) .Build(); ResourceTable output_table; @@ -644,39 +647,45 @@ TEST_F(TableFlattenerTest, FlattenOverlayable) { auto search_result = output_table.FindResource(test::ParseNameOrDie(name)); ASSERT_TRUE(search_result); ASSERT_THAT(search_result.value().entry, NotNull()); - EXPECT_EQ(search_result.value().entry->overlayable_declarations.size(), 3); - EXPECT_TRUE(search_result.value().entry->overlayable_declarations[0].policy); - EXPECT_EQ(search_result.value().entry->overlayable_declarations[0].policy, - Overlayable::Policy::kSystem); - EXPECT_TRUE(search_result.value().entry->overlayable_declarations[1].policy); - EXPECT_EQ(search_result.value().entry->overlayable_declarations[1].policy, - Overlayable::Policy::kVendor); - EXPECT_TRUE(search_result.value().entry->overlayable_declarations[2].policy); - EXPECT_EQ(search_result.value().entry->overlayable_declarations[2].policy, - Overlayable::Policy::kProduct); + ASSERT_TRUE(search_result.value().entry->overlayable); + Overlayable& result_overlayable = search_result.value().entry->overlayable.value(); + EXPECT_EQ(result_overlayable.policies, Overlayable::Policy::kSystem + | Overlayable::Policy::kVendor + | Overlayable::Policy::kProduct); } TEST_F(TableFlattenerTest, FlattenMultipleOverlayablePolicies) { std::string name_zero = "com.app.test:integer/overlayable_zero"; + Overlayable overlayable_zero{}; + overlayable_zero.policies |= Overlayable::Policy::kProduct; + overlayable_zero.policies |= Overlayable::Policy::kSystem; + overlayable_zero.policies |= Overlayable::Policy::kProductServices; + std::string name_one = "com.app.test:integer/overlayable_one"; + Overlayable overlayable_one{}; + overlayable_one.policies |= Overlayable::Policy::kPublic; + overlayable_one.policies |= Overlayable::Policy::kProductServices; + std::string name_two = "com.app.test:integer/overlayable_two"; + Overlayable overlayable_two{}; + overlayable_two.policies |= Overlayable::Policy::kProduct; + overlayable_two.policies |= Overlayable::Policy::kSystem; + overlayable_two.policies |= Overlayable::Policy::kVendor; + std::string name_three = "com.app.test:integer/overlayable_three"; + Overlayable overlayable_three{}; + std::unique_ptr<ResourceTable> table = test::ResourceTableBuilder() .SetPackageId("com.app.test", 0x7f) .AddSimple(name_zero, ResourceId(0x7f020000)) - .AddOverlayable(name_zero, Overlayable::Policy::kProduct) - .AddOverlayable(name_zero, Overlayable::Policy::kSystem) - .AddOverlayable(name_zero, Overlayable::Policy::kProductServices) + .SetOverlayable(name_zero, overlayable_zero) .AddSimple(name_one, ResourceId(0x7f020001)) - .AddOverlayable(name_one, Overlayable::Policy::kPublic) - .AddOverlayable(name_one, Overlayable::Policy::kSystem) + .SetOverlayable(name_one, overlayable_one) .AddSimple(name_two, ResourceId(0x7f020002)) - .AddOverlayable(name_two, Overlayable::Policy::kProduct) - .AddOverlayable(name_two, Overlayable::Policy::kSystem) - .AddOverlayable(name_two, Overlayable::Policy::kProductServices) + .SetOverlayable(name_two, overlayable_two) .AddSimple(name_three, ResourceId(0x7f020003)) - .AddOverlayable(name_three, {}) + .SetOverlayable(name_three, overlayable_three) .Build(); ResourceTable output_table; @@ -685,51 +694,35 @@ TEST_F(TableFlattenerTest, FlattenMultipleOverlayablePolicies) { auto search_result = output_table.FindResource(test::ParseNameOrDie(name_zero)); ASSERT_TRUE(search_result); ASSERT_THAT(search_result.value().entry, NotNull()); - EXPECT_EQ(search_result.value().entry->overlayable_declarations.size(), 3); - EXPECT_TRUE(search_result.value().entry->overlayable_declarations[0].policy); - EXPECT_EQ(search_result.value().entry->overlayable_declarations[0].policy, - Overlayable::Policy::kSystem); - EXPECT_TRUE(search_result.value().entry->overlayable_declarations[1].policy); - EXPECT_EQ(search_result.value().entry->overlayable_declarations[1].policy, - Overlayable::Policy::kProduct); - EXPECT_TRUE(search_result.value().entry->overlayable_declarations[2].policy); - EXPECT_EQ(search_result.value().entry->overlayable_declarations[2].policy, - Overlayable::Policy::kProductServices); + ASSERT_TRUE(search_result.value().entry->overlayable); + Overlayable& result_overlayable = search_result.value().entry->overlayable.value(); + EXPECT_EQ(result_overlayable.policies, Overlayable::Policy::kSystem + | Overlayable::Policy::kProduct + | Overlayable::Policy::kProductServices); search_result = output_table.FindResource(test::ParseNameOrDie(name_one)); ASSERT_TRUE(search_result); ASSERT_THAT(search_result.value().entry, NotNull()); - EXPECT_EQ(search_result.value().entry->overlayable_declarations.size(), 2); - EXPECT_TRUE(search_result.value().entry->overlayable_declarations[0].policy); - EXPECT_EQ(search_result.value().entry->overlayable_declarations[0].policy, - Overlayable::Policy::kPublic); - EXPECT_TRUE(search_result.value().entry->overlayable_declarations[1].policy); - EXPECT_EQ(search_result.value().entry->overlayable_declarations[1].policy, - Overlayable::Policy::kSystem); + ASSERT_TRUE(search_result.value().entry->overlayable); + result_overlayable = search_result.value().entry->overlayable.value(); + EXPECT_EQ(result_overlayable.policies, Overlayable::Policy::kPublic + | Overlayable::Policy::kProductServices); search_result = output_table.FindResource(test::ParseNameOrDie(name_two)); ASSERT_TRUE(search_result); ASSERT_THAT(search_result.value().entry, NotNull()); - EXPECT_EQ(search_result.value().entry->overlayable_declarations.size(), 3); - EXPECT_TRUE(search_result.value().entry->overlayable_declarations[0].policy); - EXPECT_EQ(search_result.value().entry->overlayable_declarations[0].policy, - Overlayable::Policy::kSystem); - EXPECT_TRUE(search_result.value().entry->overlayable_declarations[1].policy); - EXPECT_EQ(search_result.value().entry->overlayable_declarations[1].policy, - Overlayable::Policy::kProduct); - EXPECT_TRUE(search_result.value().entry->overlayable_declarations[2].policy); - EXPECT_EQ(search_result.value().entry->overlayable_declarations[2].policy, - Overlayable::Policy::kProductServices); + ASSERT_TRUE(search_result.value().entry->overlayable); + result_overlayable = search_result.value().entry->overlayable.value(); + EXPECT_EQ(result_overlayable.policies, Overlayable::Policy::kSystem + | Overlayable::Policy::kProduct + | Overlayable::Policy::kVendor); search_result = output_table.FindResource(test::ParseNameOrDie(name_three)); ASSERT_TRUE(search_result); ASSERT_THAT(search_result.value().entry, NotNull()); - EXPECT_EQ(search_result.value().entry->overlayable_declarations.size(), 1); - EXPECT_TRUE(search_result.value().entry->overlayable_declarations[0].policy); - EXPECT_EQ(search_result.value().entry->overlayable_declarations[0].policy, - Overlayable::Policy::kPublic); - + ASSERT_TRUE(search_result.value().entry->overlayable); + result_overlayable = search_result.value().entry->overlayable.value(); + EXPECT_EQ(result_overlayable.policies, Overlayable::Policy::kPublic); } - } // namespace aapt diff --git a/tools/aapt2/format/proto/ProtoDeserialize.cpp b/tools/aapt2/format/proto/ProtoDeserialize.cpp index f612914269de..cf2ab0f45ad6 100644 --- a/tools/aapt2/format/proto/ProtoDeserialize.cpp +++ b/tools/aapt2/format/proto/ProtoDeserialize.cpp @@ -437,37 +437,39 @@ static bool DeserializePackageFromPb(const pb::Package& pb_package, const ResStr entry->allow_new = std::move(allow_new); } - for (const pb::Overlayable& pb_overlayable : pb_entry.overlayable()) { - Overlayable overlayable; - switch (pb_overlayable.policy()) { - case pb::Overlayable::NONE: - overlayable.policy = {}; - break; - case pb::Overlayable::PUBLIC: - overlayable.policy = Overlayable::Policy::kPublic; - break; - case pb::Overlayable::PRODUCT: - overlayable.policy = Overlayable::Policy::kProduct; - break; - case pb::Overlayable::PRODUCT_SERVICES: - overlayable.policy = Overlayable::Policy::kProductServices; - break; - case pb::Overlayable::SYSTEM: - overlayable.policy = Overlayable::Policy::kSystem; - break; - case pb::Overlayable::VENDOR: - overlayable.policy = Overlayable::Policy::kVendor; - break; - default: - *out_error = "unknown overlayable policy"; - return false; + if (pb_entry.has_overlayable()) { + Overlayable overlayable{}; + + const pb::Overlayable& pb_overlayable = pb_entry.overlayable(); + for (const int policy : pb_overlayable.policy()) { + switch (policy) { + case pb::Overlayable::PUBLIC: + overlayable.policies |= Overlayable::Policy::kPublic; + break; + case pb::Overlayable::SYSTEM: + overlayable.policies |= Overlayable::Policy::kSystem; + break; + case pb::Overlayable::VENDOR: + overlayable.policies |= Overlayable::Policy::kVendor; + break; + case pb::Overlayable::PRODUCT: + overlayable.policies |= Overlayable::Policy::kProduct; + break; + case pb::Overlayable::PRODUCT_SERVICES: + overlayable.policies |= Overlayable::Policy::kProductServices; + break; + default: + *out_error = "unknown overlayable policy"; + return false; + } } if (pb_overlayable.has_source()) { DeserializeSourceFromPb(pb_overlayable.source(), src_pool, &overlayable.source); } + overlayable.comment = pb_overlayable.comment(); - entry->overlayable_declarations.push_back(overlayable); + entry->overlayable = overlayable; } ResourceId resid(pb_package.package_id().id(), pb_type.type_id().id(), diff --git a/tools/aapt2/format/proto/ProtoSerialize.cpp b/tools/aapt2/format/proto/ProtoSerialize.cpp index ecf34d13e1b3..70bf8684f8a8 100644 --- a/tools/aapt2/format/proto/ProtoSerialize.cpp +++ b/tools/aapt2/format/proto/ProtoSerialize.cpp @@ -310,26 +310,24 @@ void SerializeTableToPb(const ResourceTable& table, pb::ResourceTable* out_table pb_allow_new->set_comment(entry->allow_new.value().comment); } - for (const Overlayable& overlayable : entry->overlayable_declarations) { - pb::Overlayable* pb_overlayable = pb_entry->add_overlayable(); - if (overlayable.policy) { - switch (overlayable.policy.value()) { - case Overlayable::Policy::kPublic: - pb_overlayable->set_policy(pb::Overlayable::PUBLIC); - break; - case Overlayable::Policy::kProduct: - pb_overlayable->set_policy(pb::Overlayable::PRODUCT); - break; - case Overlayable::Policy::kProductServices: - pb_overlayable->set_policy(pb::Overlayable::PRODUCT_SERVICES); - break; - case Overlayable::Policy::kSystem: - pb_overlayable->set_policy(pb::Overlayable::SYSTEM); - break; - case Overlayable::Policy::kVendor: - pb_overlayable->set_policy(pb::Overlayable::VENDOR); - break; - } + if (entry->overlayable) { + pb::Overlayable* pb_overlayable = pb_entry->mutable_overlayable(); + + Overlayable overlayable = entry->overlayable.value(); + if (overlayable.policies & Overlayable::Policy::kPublic) { + pb_overlayable->add_policy(pb::Overlayable::PUBLIC); + } + if (overlayable.policies & Overlayable::Policy::kProduct) { + pb_overlayable->add_policy(pb::Overlayable::PRODUCT); + } + if (overlayable.policies & Overlayable::Policy::kProductServices) { + pb_overlayable->add_policy(pb::Overlayable::PRODUCT_SERVICES); + } + if (overlayable.policies & Overlayable::Policy::kSystem) { + pb_overlayable->add_policy(pb::Overlayable::SYSTEM); + } + if (overlayable.policies & Overlayable::Policy::kVendor) { + pb_overlayable->add_policy(pb::Overlayable::VENDOR); } SerializeSourceToPb(overlayable.source, &source_pool, diff --git a/tools/aapt2/format/proto/ProtoSerialize_test.cpp b/tools/aapt2/format/proto/ProtoSerialize_test.cpp index 1cd2f0b9a961..fb913f409f52 100644 --- a/tools/aapt2/format/proto/ProtoSerialize_test.cpp +++ b/tools/aapt2/format/proto/ProtoSerialize_test.cpp @@ -93,7 +93,7 @@ TEST(ProtoSerializeTest, SerializeSinglePackage) { util::make_unique<Reference>(expected_ref), context->GetDiagnostics())); // Make an overlayable resource. - ASSERT_TRUE(table->AddOverlayable(test::ParseNameOrDie("com.app.a:integer/overlayable"), + ASSERT_TRUE(table->SetOverlayable(test::ParseNameOrDie("com.app.a:integer/overlayable"), Overlayable{}, test::GetDiagnostics())); pb::ResourceTable pb_table; @@ -160,8 +160,9 @@ TEST(ProtoSerializeTest, SerializeSinglePackage) { new_table.FindResource(test::ParseNameOrDie("com.app.a:integer/overlayable")); ASSERT_TRUE(search_result); ASSERT_THAT(search_result.value().entry, NotNull()); - EXPECT_THAT(search_result.value().entry->overlayable_declarations.size(), Eq(1)); - EXPECT_FALSE(search_result.value().entry->overlayable_declarations[0].policy); + ASSERT_TRUE(search_result.value().entry->overlayable); + EXPECT_THAT(search_result.value().entry->overlayable.value().policies, + Eq(Overlayable::Policy::kNone)); } TEST(ProtoSerializeTest, SerializeAndDeserializeXml) { @@ -502,15 +503,26 @@ TEST(ProtoSerializeTest, SerializeDeserializeConfiguration) { } TEST(ProtoSerializeTest, SerializeAndDeserializeOverlayable) { + Overlayable overlayable_foo{}; + overlayable_foo.policies |= Overlayable::Policy::kSystem; + overlayable_foo.policies |= Overlayable::Policy::kProduct; + + Overlayable overlayable_bar{}; + overlayable_bar.policies |= Overlayable::Policy::kProductServices; + overlayable_bar.policies |= Overlayable::Policy::kVendor; + + Overlayable overlayable_baz{}; + overlayable_baz.policies |= Overlayable::Policy::kPublic; + + Overlayable overlayable_biz{}; + std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build(); std::unique_ptr<ResourceTable> table = test::ResourceTableBuilder() - .AddOverlayable("com.app.a:bool/foo", Overlayable::Policy::kSystem) - .AddOverlayable("com.app.a:bool/foo", Overlayable::Policy::kProduct) - .AddOverlayable("com.app.a:bool/bar", Overlayable::Policy::kProductServices) - .AddOverlayable("com.app.a:bool/bar", Overlayable::Policy::kVendor) - .AddOverlayable("com.app.a:bool/baz", Overlayable::Policy::kPublic) - .AddOverlayable("com.app.a:bool/biz", {}) + .SetOverlayable("com.app.a:bool/foo", overlayable_foo) + .SetOverlayable("com.app.a:bool/bar", overlayable_bar) + .SetOverlayable("com.app.a:bool/baz", overlayable_baz) + .SetOverlayable("com.app.a:bool/biz", overlayable_biz) .AddValue("com.app.a:bool/fiz", ResourceUtils::TryParseBool("true")) .Build(); @@ -523,37 +535,36 @@ TEST(ProtoSerializeTest, SerializeAndDeserializeOverlayable) { ASSERT_TRUE(DeserializeTableFromPb(pb_table, &files, &new_table, &error)); EXPECT_THAT(error, IsEmpty()); - Maybe<ResourceTable::SearchResult> result = + Maybe<ResourceTable::SearchResult> search_result = new_table.FindResource(test::ParseNameOrDie("com.app.a:bool/foo")); - ASSERT_TRUE(result); - ASSERT_THAT(result.value().entry->overlayable_declarations.size(), Eq(2)); - EXPECT_THAT(result.value().entry->overlayable_declarations[0].policy, - Eq(Overlayable::Policy::kSystem)); - EXPECT_THAT(result.value().entry->overlayable_declarations[1].policy, - Eq(Overlayable::Policy::kProduct)); + ASSERT_TRUE(search_result); + ASSERT_TRUE(search_result.value().entry->overlayable); + Overlayable result_overlayable = search_result.value().entry->overlayable.value(); + EXPECT_THAT(result_overlayable.policies, Eq(Overlayable::Policy::kSystem + | Overlayable::Policy::kProduct)); - result = new_table.FindResource(test::ParseNameOrDie("com.app.a:bool/bar")); - ASSERT_TRUE(result); - ASSERT_THAT(result.value().entry->overlayable_declarations.size(), Eq(2)); - EXPECT_THAT(result.value().entry->overlayable_declarations[0].policy, - Eq(Overlayable::Policy::kProductServices)); - EXPECT_THAT(result.value().entry->overlayable_declarations[1].policy, - Eq(Overlayable::Policy::kVendor)); + search_result = new_table.FindResource(test::ParseNameOrDie("com.app.a:bool/bar")); + ASSERT_TRUE(search_result); + ASSERT_TRUE(search_result.value().entry->overlayable); + result_overlayable = search_result.value().entry->overlayable.value(); + EXPECT_THAT(result_overlayable.policies, Eq(Overlayable::Policy::kProductServices + | Overlayable::Policy::kVendor)); - result = new_table.FindResource(test::ParseNameOrDie("com.app.a:bool/baz")); - ASSERT_TRUE(result); - ASSERT_THAT(result.value().entry->overlayable_declarations.size(), Eq(1)); - EXPECT_THAT(result.value().entry->overlayable_declarations[0].policy, - Eq(Overlayable::Policy::kPublic)); + search_result = new_table.FindResource(test::ParseNameOrDie("com.app.a:bool/baz")); + ASSERT_TRUE(search_result); + ASSERT_TRUE(search_result.value().entry->overlayable); + result_overlayable = search_result.value().entry->overlayable.value(); + EXPECT_THAT(result_overlayable.policies, Overlayable::Policy::kPublic); - result = new_table.FindResource(test::ParseNameOrDie("com.app.a:bool/biz")); - ASSERT_TRUE(result); - ASSERT_THAT(result.value().entry->overlayable_declarations.size(), Eq(1)); - EXPECT_FALSE(result.value().entry->overlayable_declarations[0].policy); + search_result = new_table.FindResource(test::ParseNameOrDie("com.app.a:bool/biz")); + ASSERT_TRUE(search_result); + ASSERT_TRUE(search_result.value().entry->overlayable); + result_overlayable = search_result.value().entry->overlayable.value(); + EXPECT_THAT(result_overlayable.policies, Overlayable::Policy::kNone); - result = new_table.FindResource(test::ParseNameOrDie("com.app.a:bool/fiz")); - ASSERT_TRUE(result); - EXPECT_THAT(result.value().entry->overlayable_declarations.size(), Eq(0)); + search_result = new_table.FindResource(test::ParseNameOrDie("com.app.a:bool/fiz")); + ASSERT_TRUE(search_result); + ASSERT_FALSE(search_result.value().entry->overlayable); } } // namespace aapt diff --git a/tools/aapt2/link/ReferenceLinker.cpp b/tools/aapt2/link/ReferenceLinker.cpp index 1b6626a8dfe9..8cbc03738677 100644 --- a/tools/aapt2/link/ReferenceLinker.cpp +++ b/tools/aapt2/link/ReferenceLinker.cpp @@ -374,8 +374,8 @@ bool ReferenceLinker::Consume(IAaptContext* context, ResourceTable* table) { } // Ensure that definitions for values declared as overlayable exist - if (!entry->overlayable_declarations.empty() && entry->values.empty()) { - context->GetDiagnostics()->Error(DiagMessage(entry->overlayable_declarations[0].source) + if (entry->overlayable && entry->values.empty()) { + context->GetDiagnostics()->Error(DiagMessage(entry->overlayable.value().source) << "no definition for overlayable symbol '" << name << "'"); error = true; diff --git a/tools/aapt2/link/TableMerger.cpp b/tools/aapt2/link/TableMerger.cpp index d777e22fa4b7..22e1723591a8 100644 --- a/tools/aapt2/link/TableMerger.cpp +++ b/tools/aapt2/link/TableMerger.cpp @@ -134,35 +134,21 @@ static bool MergeEntry(IAaptContext* context, const Source& src, dst_entry->allow_new = std::move(src_entry->allow_new); } - for (auto& src_overlayable : src_entry->overlayable_declarations) { - for (auto& dst_overlayable : dst_entry->overlayable_declarations) { - // An overlayable resource cannot be declared twice with the same policy - if (src_overlayable.policy == dst_overlayable.policy) { - context->GetDiagnostics()->Error(DiagMessage(src_overlayable.source) - << "duplicate overlayable declaration for resource '" - << src_entry->name << "'"); - context->GetDiagnostics()->Error(DiagMessage(dst_overlayable.source) - << "previous declaration here"); - return false; - } - - // An overlayable resource cannot be declared once with a policy and without a policy because - // the policy becomes unused - if (!src_overlayable.policy || !dst_overlayable.policy) { - context->GetDiagnostics()->Error(DiagMessage(src_overlayable.source) - << "overlayable resource '" << src_entry->name - << "' declared once with a policy and once with no " - << "policy"); - context->GetDiagnostics()->Error(DiagMessage(dst_overlayable.source) - << "previous declaration here"); - return false; - } + if (src_entry->overlayable) { + if (dst_entry->overlayable) { + // Do not allow a resource with an overlayable declaration to have that overlayable + // declaration redefined + context->GetDiagnostics()->Error(DiagMessage(src_entry->overlayable.value().source) + << "duplicate overlayable declaration for resource '" + << src_entry->name << "'"); + context->GetDiagnostics()->Error(DiagMessage(dst_entry->overlayable.value().source) + << "previous declaration here"); + return false; + } else { + dst_entry->overlayable = std::move(src_entry->overlayable); } } - dst_entry->overlayable_declarations.insert(dst_entry->overlayable_declarations.end(), - src_entry->overlayable_declarations.begin(), - src_entry->overlayable_declarations.end()); return true; } diff --git a/tools/aapt2/link/TableMerger_test.cpp b/tools/aapt2/link/TableMerger_test.cpp index d6579d37b452..17b2a83bad04 100644 --- a/tools/aapt2/link/TableMerger_test.cpp +++ b/tools/aapt2/link/TableMerger_test.cpp @@ -436,17 +436,21 @@ TEST_F(TableMergerTest, OverlaidStyleablesAndStylesShouldBeMerged) { Eq(make_value(Reference(test::ParseNameOrDie("com.app.a:style/OverlayParent"))))); } -TEST_F(TableMergerTest, AddOverlayable) { +TEST_F(TableMergerTest, SetOverlayable) { + Overlayable overlayable{}; + overlayable.policies |= Overlayable::Policy::kProduct; + overlayable.policies |= Overlayable::Policy::kVendor; + std::unique_ptr<ResourceTable> table_a = test::ResourceTableBuilder() .SetPackageId("com.app.a", 0x7f) - .AddOverlayable("bool/foo", Overlayable::Policy::kProduct) + .SetOverlayable("bool/foo", overlayable) .Build(); std::unique_ptr<ResourceTable> table_b = test::ResourceTableBuilder() .SetPackageId("com.app.a", 0x7f) - .AddOverlayable("bool/foo", Overlayable::Policy::kProductServices) + .AddSimple("bool/foo") .Build(); ResourceTable final_table; @@ -457,26 +461,28 @@ TEST_F(TableMergerTest, AddOverlayable) { ASSERT_TRUE(merger.Merge({}, table_b.get(), false /*overlay*/)); const ResourceName name = test::ParseNameOrDie("com.app.a:bool/foo"); - Maybe<ResourceTable::SearchResult> result = final_table.FindResource(name); - ASSERT_TRUE(result); - ASSERT_THAT(result.value().entry->overlayable_declarations.size(), Eq(2)); - ASSERT_THAT(result.value().entry->overlayable_declarations[0].policy, - Eq(Overlayable::Policy::kProduct)); - ASSERT_THAT(result.value().entry->overlayable_declarations[1].policy, - Eq(Overlayable::Policy::kProductServices)); + Maybe<ResourceTable::SearchResult> search_result = final_table.FindResource(name); + ASSERT_TRUE(search_result); + ASSERT_TRUE(search_result.value().entry->overlayable); + Overlayable& result_overlayable = search_result.value().entry->overlayable.value(); + EXPECT_THAT(result_overlayable.policies, Eq(Overlayable::Policy::kProduct + | Overlayable::Policy::kVendor)); } -TEST_F(TableMergerTest, AddDuplicateOverlayableFail) { +TEST_F(TableMergerTest, SetOverlayableLater) { std::unique_ptr<ResourceTable> table_a = test::ResourceTableBuilder() .SetPackageId("com.app.a", 0x7f) - .AddOverlayable("bool/foo", Overlayable::Policy::kProduct) + .AddSimple("bool/foo") .Build(); + Overlayable overlayable{}; + overlayable.policies |= Overlayable::Policy::kPublic; + overlayable.policies |= Overlayable::Policy::kProductServices; std::unique_ptr<ResourceTable> table_b = test::ResourceTableBuilder() .SetPackageId("com.app.a", 0x7f) - .AddOverlayable("bool/foo", Overlayable::Policy::kProduct) + .SetOverlayable("bool/foo", overlayable) .Build(); ResourceTable final_table; @@ -484,20 +490,32 @@ TEST_F(TableMergerTest, AddDuplicateOverlayableFail) { options.auto_add_overlay = true; TableMerger merger(context_.get(), &final_table, options); ASSERT_TRUE(merger.Merge({}, table_a.get(), false /*overlay*/)); - ASSERT_FALSE(merger.Merge({}, table_b.get(), false /*overlay*/)); + ASSERT_TRUE(merger.Merge({}, table_b.get(), false /*overlay*/)); + + const ResourceName name = test::ParseNameOrDie("com.app.a:bool/foo"); + Maybe<ResourceTable::SearchResult> search_result = final_table.FindResource(name); + ASSERT_TRUE(search_result); + ASSERT_TRUE(search_result.value().entry->overlayable); + Overlayable& result_overlayable = search_result.value().entry->overlayable.value(); + EXPECT_THAT(result_overlayable.policies, Eq(Overlayable::Policy::kPublic + | Overlayable::Policy::kProductServices)); } -TEST_F(TableMergerTest, AddOverlayablePolicyAndNoneFirstFail) { +TEST_F(TableMergerTest, SetOverlayableSamePolicesFail) { + Overlayable overlayable_first{}; + overlayable_first.policies |= Overlayable::Policy::kProduct; std::unique_ptr<ResourceTable> table_a = test::ResourceTableBuilder() .SetPackageId("com.app.a", 0x7f) - .AddOverlayable("bool/foo", {}) + .SetOverlayable("bool/foo", overlayable_first) .Build(); + Overlayable overlayable_second{}; + overlayable_second.policies |= Overlayable::Policy::kProduct; std::unique_ptr<ResourceTable> table_b = test::ResourceTableBuilder() .SetPackageId("com.app.a", 0x7f) - .AddOverlayable("bool/foo", Overlayable::Policy::kProduct) + .SetOverlayable("bool/foo", overlayable_second) .Build(); ResourceTable final_table; @@ -508,17 +526,21 @@ TEST_F(TableMergerTest, AddOverlayablePolicyAndNoneFirstFail) { ASSERT_FALSE(merger.Merge({}, table_b.get(), false /*overlay*/)); } -TEST_F(TableMergerTest, AddOverlayablePolicyAndNoneLastFail) { +TEST_F(TableMergerTest, SetOverlayableDifferentPolicesFail) { + Overlayable overlayable_first{}; + overlayable_first.policies |= Overlayable::Policy::kVendor; std::unique_ptr<ResourceTable> table_a = test::ResourceTableBuilder() .SetPackageId("com.app.a", 0x7f) - .AddOverlayable("bool/foo", Overlayable::Policy::kProduct) + .SetOverlayable("bool/foo",overlayable_first) .Build(); + Overlayable overlayable_second{}; + overlayable_second.policies |= Overlayable::Policy::kProduct; std::unique_ptr<ResourceTable> table_b = test::ResourceTableBuilder() .SetPackageId("com.app.a", 0x7f) - .AddOverlayable("bool/foo", {}) + .SetOverlayable("bool/foo", overlayable_second) .Build(); ResourceTable final_table; diff --git a/tools/aapt2/split/TableSplitter.cpp b/tools/aapt2/split/TableSplitter.cpp index 2e717ff2bc3b..9c5b5d36b798 100644 --- a/tools/aapt2/split/TableSplitter.cpp +++ b/tools/aapt2/split/TableSplitter.cpp @@ -248,7 +248,7 @@ void TableSplitter::SplitTable(ResourceTable* original_table) { if (!split_entry->id) { split_entry->id = entry->id; split_entry->visibility = entry->visibility; - split_entry->overlayable_declarations = entry->overlayable_declarations; + split_entry->overlayable = entry->overlayable; } // Copy the selected values into the new Split Entry. diff --git a/tools/aapt2/test/Builders.cpp b/tools/aapt2/test/Builders.cpp index 03b59e033402..884ec38290f8 100644 --- a/tools/aapt2/test/Builders.cpp +++ b/tools/aapt2/test/Builders.cpp @@ -135,12 +135,11 @@ ResourceTableBuilder& ResourceTableBuilder::SetSymbolState(const StringPiece& na return *this; } -ResourceTableBuilder& ResourceTableBuilder::AddOverlayable(const StringPiece& name, - const Maybe<Overlayable::Policy> p) { +ResourceTableBuilder& ResourceTableBuilder::SetOverlayable(const StringPiece& name, + const Overlayable& overlayable) { + ResourceName res_name = ParseNameOrDie(name); - Overlayable overlayable; - overlayable.policy = p; - CHECK(table_->AddOverlayable(res_name, overlayable, GetDiagnostics())); + CHECK(table_->SetOverlayable(res_name, overlayable, GetDiagnostics())); return *this; } diff --git a/tools/aapt2/test/Builders.h b/tools/aapt2/test/Builders.h index d68c24ddc665..a12048436e38 100644 --- a/tools/aapt2/test/Builders.h +++ b/tools/aapt2/test/Builders.h @@ -73,8 +73,8 @@ class ResourceTableBuilder { const ResourceId& id, std::unique_ptr<Value> value); ResourceTableBuilder& SetSymbolState(const android::StringPiece& name, const ResourceId& id, Visibility::Level level, bool allow_new = false); - ResourceTableBuilder& AddOverlayable(const android::StringPiece& name, - Maybe<Overlayable::Policy> policy); + ResourceTableBuilder& SetOverlayable(const android::StringPiece& name, + const Overlayable& overlayable); StringPool* string_pool(); std::unique_ptr<ResourceTable> Build(); |