From 5d2755129dae6ff6f0061da0abe634f44054b23e Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Thu, 19 Jul 2018 14:29:00 -0700 Subject: AAPT2: Reformatted dump command invocations Use with: aapt2 dump apc [apc] aapt2 dump configurations [apk] aapt2 dump strings [apk] aapt2 dump resources [apk] aapt2 dump xmlstrings [apk] --file [file] aapt2 dump xmltree [apk] --file [file] Will add permissions and badging in a later commit. Bug: 73351292 Test: Manual tests of the commands Change-Id: I97eec01222af14053a98bd70255f1bfecd16b1c4 --- tools/aapt2/Debug.cpp | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) (limited to 'tools/aapt2/Debug.cpp') diff --git a/tools/aapt2/Debug.cpp b/tools/aapt2/Debug.cpp index f064cb14248f..0a517ab8a2de 100644 --- a/tools/aapt2/Debug.cpp +++ b/tools/aapt2/Debug.cpp @@ -408,6 +408,41 @@ void Debug::DumpHex(const void* data, size_t len) { } } +void Debug::DumpResStringPool(const android::ResStringPool* pool, text::Printer* printer) { + using namespace android; + + if (pool->getError() == NO_INIT) { + printer->Print("String pool is unitialized.\n"); + return; + } else if (pool->getError() != NO_ERROR) { + printer->Print("String pool is corrupt/invalid.\n"); + return; + } + + SortedVector uniqueStrings; + const size_t N = pool->size(); + for (size_t i=0; iisUTF8()) { + uniqueStrings.add(pool->string8At(i, &len)); + } else { + uniqueStrings.add(pool->stringAt(i, &len)); + } + } + + printer->Print(StringPrintf("String pool of %zd unique %s %s strings, %zd entries and %zd styles " + "using %zd bytes:\n", uniqueStrings.size(), + pool->isUTF8() ? "UTF-8" : "UTF-16", + pool->isSorted() ? "sorted" : "non-sorted", N, pool->styleCount(), + pool->bytes())); + + const size_t NS = pool->size(); + for (size_t s=0; sstring8ObjectAt(s); + printer->Print(StringPrintf("String #%zd : %s\n", s, str.string())); + } +} + namespace { class XmlPrinter : public xml::ConstVisitor { -- cgit v1.2.3-59-g8ed1b From e4e989ccba19f9bfad44d070873d67e7a3fd29c4 Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Mon, 29 Oct 2018 02:21:50 -0700 Subject: RRO: Added partition policies for overlays tags can now have policy elements that indicate which partition the overlay apk must reside on in order to be allowed to overlay a resource. This change only adds parsing of and encoding of policy in the proto ResourceTable. A later change will add the encoding of policy and overlayable in the binary APK. Bug: 110869880 Test: make aapt2_tests Change-Id: I8d4ed7b0e01f981149c6e3190af1681073b79b03 --- tools/aapt2/Debug.cpp | 25 ++- tools/aapt2/ResourceParser.cpp | 164 +++++++++----- tools/aapt2/ResourceParser.h | 4 + tools/aapt2/ResourceParser_test.cpp | 239 ++++++++++++++++++--- tools/aapt2/ResourceTable.cpp | 39 ++-- tools/aapt2/ResourceTable.h | 30 ++- tools/aapt2/ResourceTable_test.cpp | 62 +++++- tools/aapt2/Resources.proto | 16 +- tools/aapt2/format/binary/BinaryResourceParser.cpp | 2 +- tools/aapt2/format/binary/TableFlattener.cpp | 2 +- tools/aapt2/format/binary/TableFlattener_test.cpp | 2 +- tools/aapt2/format/proto/ProtoDeserialize.cpp | 30 ++- tools/aapt2/format/proto/ProtoSerialize.cpp | 28 ++- tools/aapt2/format/proto/ProtoSerialize_test.cpp | 62 +++++- tools/aapt2/link/TableMerger.cpp | 40 +++- tools/aapt2/link/TableMerger_test.cpp | 93 ++++++++ tools/aapt2/test/Builders.cpp | 9 + tools/aapt2/test/Builders.h | 2 + 18 files changed, 717 insertions(+), 132 deletions(-) (limited to 'tools/aapt2/Debug.cpp') diff --git a/tools/aapt2/Debug.cpp b/tools/aapt2/Debug.cpp index 0a517ab8a2de..0bc5221245ca 100644 --- a/tools/aapt2/Debug.cpp +++ b/tools/aapt2/Debug.cpp @@ -306,8 +306,29 @@ void Debug::PrintTable(const ResourceTable& table, const DebugPrintTableOptions& break; } - if (entry->overlayable) { - printer->Print(" OVERLAYABLE"); + 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(); diff --git a/tools/aapt2/ResourceParser.cpp b/tools/aapt2/ResourceParser.cpp index 9a3f14c8e08e..4f25e0968c0e 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; - bool overlayable = false; + std::vector overlayable_declarations; std::string comment; std::unique_ptr value; @@ -133,11 +133,8 @@ static bool AddResourcesToTable(ResourceTable* table, IDiagnostics* diag, Parsed } } - if (res->overlayable) { - Overlayable overlayable; - overlayable.source = res->source; - overlayable.comment = res->comment; - if (!table->SetOverlayable(res->name, overlayable, diag)) { + for (auto& overlayable : res->overlayable_declarations) { + if (!table->AddOverlayable(res->name, overlayable, diag)) { return false; } } @@ -673,7 +670,7 @@ bool ResourceParser::ParseResource(xml::XmlPullParser* parser, if (can_be_bag) { const auto bag_iter = elToBagMap.find(resource_type); if (bag_iter != elToBagMap.end()) { - // Ensure we have a name (unless this is a ). + // Ensure we have a name (unless this is a or ). if (resource_type != "public-group" && resource_type != "overlayable") { if (!maybe_name) { diag_->Error(DiagMessage(out_resource->source) @@ -1062,72 +1059,135 @@ bool ResourceParser::ParseSymbol(xml::XmlPullParser* parser, ParsedResource* out bool ResourceParser::ParseOverlayable(xml::XmlPullParser* parser, ParsedResource* out_resource) { if (out_resource->config != ConfigDescription::DefaultConfig()) { diag_->Warn(DiagMessage(out_resource->source) - << "ignoring configuration '" << out_resource->config << "' for tag"); + << "ignoring configuration '" << out_resource->config + << "' for tag"); } - if (Maybe maybe_policy = xml::FindNonEmptyAttribute(parser, "policy")) { - const StringPiece& policy = maybe_policy.value(); - if (policy != "system") { - diag_->Error(DiagMessage(out_resource->source) - << " has invalid policy '" << policy << "'"); - return false; - } - } + std::string comment; + std::vector policies; bool error = false; - const size_t depth = parser->depth(); - while (xml::XmlPullParser::NextChildNode(parser, depth)) { - if (parser->event() != xml::XmlPullParser::Event::kStartElement) { - // Skip text/comments. + 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; + } else if (event == xml::XmlPullParser::Event::kEndElement + && parser->depth() == start_depth + 1) { + // Clear the current policies when exiting the policy element + policies.clear(); + continue; + } else if (event == xml::XmlPullParser::Event::kComment) { + // Get the comment of individual item elements + comment = parser->comment(); + continue; + } else if (event != xml::XmlPullParser::Event::kStartElement) { + // Skip to the next element continue; } const Source item_source = source_.WithLine(parser->line_number()); - const std::string& element_namespace = parser->element_namespace(); const std::string& element_name = parser->element_name(); + const std::string& element_namespace = parser->element_namespace(); + if (element_namespace.empty() && element_name == "item") { - Maybe maybe_name = xml::FindNonEmptyAttribute(parser, "name"); - if (!maybe_name) { - diag_->Error(DiagMessage(item_source) - << " within an tag must have a 'name' attribute"); + if (!ParseOverlayableItem(parser, policies, comment, out_resource)) { error = true; - continue; } - - Maybe maybe_type = xml::FindNonEmptyAttribute(parser, "type"); - if (!maybe_type) { - diag_->Error(DiagMessage(item_source) - << " within an tag must have a 'type' attribute"); + } else if (element_namespace.empty() && element_name == "policy") { + if (!policies.empty()) { + // If the policy list is not empty, then we are currently inside a policy element + diag_->Error(DiagMessage(item_source) << " blocks cannot be recursively nested"); error = true; - continue; + break; + } else if (Maybe maybe_type = xml::FindNonEmptyAttribute(parser, "type")) { + // Parse the polices separated by vertical bar characters to allow for specifying multiple + // policies at once + for (StringPiece part : util::Tokenize(maybe_type.value(), '|')) { + StringPiece trimmed_part = util::TrimWhitespace(part); + if (trimmed_part == "public") { + policies.push_back(Overlayable::Policy::kPublic); + } else if (trimmed_part == "product") { + policies.push_back(Overlayable::Policy::kProduct); + } else if (trimmed_part == "product_services") { + policies.push_back(Overlayable::Policy::kProductServices); + } else if (trimmed_part == "system") { + policies.push_back(Overlayable::Policy::kSystem); + } else if (trimmed_part == "vendor") { + policies.push_back(Overlayable::Policy::kVendor); + } else { + diag_->Error(DiagMessage(out_resource->source) + << " 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 " + << " "); + error = true; + break; + } + } - const ResourceType* type = ParseResourceType(maybe_type.value()); - if (type == nullptr) { - diag_->Error(DiagMessage(out_resource->source) + return !error; +} + +bool ResourceParser::ParseOverlayableItem(xml::XmlPullParser* parser, + const std::vector& policies, + const std::string& comment, + ParsedResource* out_resource) { + const Source item_source = source_.WithLine(parser->line_number()); + + Maybe maybe_name = xml::FindNonEmptyAttribute(parser, "name"); + if (!maybe_name) { + diag_->Error(DiagMessage(item_source) + << " within an tag must have a 'name' attribute"); + return false; + } + + Maybe maybe_type = xml::FindNonEmptyAttribute(parser, "type"); + if (!maybe_type) { + diag_->Error(DiagMessage(item_source) + << " within an 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 within an "); - error = true; - continue; - } + return false; + } - ParsedResource child_resource; - child_resource.name.type = *type; - child_resource.name.entry = maybe_name.value().to_string(); - child_resource.source = item_source; - child_resource.overlayable = true; - if (options_.visibility) { - child_resource.visibility_level = options_.visibility.value(); - } - out_resource->child_resources.push_back(std::move(child_resource)); + ParsedResource child_resource; + child_resource.name.type = *type; + child_resource.name.entry = maybe_name.value().to_string(); + child_resource.source = item_source; - xml::XmlPullParser::SkipCurrentElement(parser); - } else if (!ShouldIgnoreElement(element_namespace, element_name)) { - diag_->Error(DiagMessage(item_source) << ":" << element_name << ">"); - error = true; + 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); } } - return !error; + + 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) { diff --git a/tools/aapt2/ResourceParser.h b/tools/aapt2/ResourceParser.h index 06bb0c9cf264..ebacd6f1280e 100644 --- a/tools/aapt2/ResourceParser.h +++ b/tools/aapt2/ResourceParser.h @@ -96,6 +96,10 @@ 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& 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 0dff66430532..c6f29ac53ca6 100644 --- a/tools/aapt2/ResourceParser_test.cpp +++ b/tools/aapt2/ResourceParser_test.cpp @@ -891,56 +891,162 @@ TEST_F(ResourceParserTest, ParsePlatformIndependentNewline) { ASSERT_TRUE(TestParse(R"(%1$s %n %2$s)")); } -TEST_F(ResourceParserTest, ParseOverlayableTagWithSystemPolicy) { - std::string input = R"( - +TEST_F(ResourceParserTest, ParseOverlayable) { + std::string input = R"()"; + EXPECT_TRUE(TestParse(input)); + + input = R"( + + )"; - EXPECT_FALSE(TestParse(input)); + ASSERT_TRUE(TestParse(input)); + + 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); + + 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); +} + +TEST_F(ResourceParserTest, ParseOverlayablePolicy) { + std::string input = R"()"; + EXPECT_TRUE(TestParse(input)); input = R"( - - + + + + + + + + + + + + + + + + + )"; - EXPECT_FALSE(TestParse(input)); + ASSERT_TRUE(TestParse(input)); - input = R"( - - + 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); + + 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)); + + 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)); + + 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)); + + 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)); + + 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)); +} + +TEST_F(ResourceParserTest, ParseOverlayableBadPolicyError) { + std::string input = R"( + + + + )"; EXPECT_FALSE(TestParse(input)); input = R"( - - + + + + )"; EXPECT_FALSE(TestParse(input)); - input = R"()"; - EXPECT_TRUE(TestParse(input)); - - input = R"()"; - EXPECT_TRUE(TestParse(input)); - input = R"( - - - + + + + )"; - ASSERT_TRUE(TestParse(input)); + EXPECT_FALSE(TestParse(input)); +} - input = R"( +TEST_F(ResourceParserTest, ParseOverlayableMultiplePolicy) { + std::string input = R"( - + + + + + + )"; ASSERT_TRUE(TestParse(input)); - Maybe search_result = - table_.FindResource(test::ParseNameOrDie("string/bar")); + 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)); + + 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_TRUE(search_result.value().entry->overlayable); + 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)); } TEST_F(ResourceParserTest, DuplicateOverlayableIsError) { @@ -950,6 +1056,85 @@ TEST_F(ResourceParserTest, DuplicateOverlayableIsError) { )"; EXPECT_FALSE(TestParse(input)); + + input = R"( + + + + + + )"; + EXPECT_FALSE(TestParse(input)); + + input = R"( + + + + + + )"; + EXPECT_FALSE(TestParse(input)); + + input = R"( + + + + + + + + + + + )"; + EXPECT_FALSE(TestParse(input)); +} + +TEST_F(ResourceParserTest, PolicyAndNonPolicyOverlayableError) { + std::string input = R"( + + + + + + )"; + EXPECT_FALSE(TestParse(input)); + + input = R"( + + + + + + )"; + EXPECT_FALSE(TestParse(input)); +} + +TEST_F(ResourceParserTest, DuplicateOverlayableMultiplePolicyError) { + std::string input = R"( + + + + + + + + + + )"; + EXPECT_FALSE(TestParse(input)); +} + +TEST_F(ResourceParserTest, NestPolicyInOverlayableError) { + std::string input = R"( + + + + + + + )"; + EXPECT_FALSE(TestParse(input)); } TEST_F(ResourceParserTest, ParseIdItem) { diff --git a/tools/aapt2/ResourceTable.cpp b/tools/aapt2/ResourceTable.cpp index 056a27bf011d..bc8a4d1f85b8 100644 --- a/tools/aapt2/ResourceTable.cpp +++ b/tools/aapt2/ResourceTable.cpp @@ -625,17 +625,17 @@ bool ResourceTable::SetAllowNewImpl(const ResourceNameRef& name, const AllowNew& return true; } -bool ResourceTable::SetOverlayable(const ResourceNameRef& name, const Overlayable& overlayable, +bool ResourceTable::AddOverlayable(const ResourceNameRef& name, const Overlayable& overlayable, IDiagnostics* diag) { - return SetOverlayableImpl(name, overlayable, ResourceNameValidator, diag); + return AddOverlayableImpl(name, overlayable, ResourceNameValidator, diag); } -bool ResourceTable::SetOverlayableMangled(const ResourceNameRef& name, +bool ResourceTable::AddOverlayableMangled(const ResourceNameRef& name, const Overlayable& overlayable, IDiagnostics* diag) { - return SetOverlayableImpl(name, overlayable, SkipNameValidator, diag); + return AddOverlayableImpl(name, overlayable, SkipNameValidator, diag); } -bool ResourceTable::SetOverlayableImpl(const ResourceNameRef& name, const Overlayable& overlayable, +bool ResourceTable::AddOverlayableImpl(const ResourceNameRef& name, const Overlayable& overlayable, NameValidator name_validator, IDiagnostics* diag) { CHECK(diag != nullptr); @@ -646,13 +646,28 @@ bool ResourceTable::SetOverlayableImpl(const ResourceNameRef& name, const Overla ResourceTablePackage* package = FindOrCreatePackage(name.package); ResourceTableType* type = package->FindOrCreateType(name.type); ResourceEntry* entry = type->FindOrCreateEntry(name.entry); - if (entry->overlayable) { - diag->Error(DiagMessage(overlayable.source) - << "duplicate overlayable declaration for resource '" << name << "'"); - diag->Error(DiagMessage(entry->overlayable.value().source) << "previous declaration here"); - return false; + + 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) + << "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; + } } - entry->overlayable = overlayable; + + entry->overlayable_declarations.push_back(overlayable); return true; } @@ -688,7 +703,7 @@ std::unique_ptr ResourceTable::Clone() const { new_entry->id = entry->id; new_entry->visibility = entry->visibility; new_entry->allow_new = entry->allow_new; - new_entry->overlayable = entry->overlayable; + new_entry->overlayable_declarations = entry->overlayable_declarations; for (const auto& config_value : entry->values) { ResourceConfigValue* new_value = diff --git a/tools/aapt2/ResourceTable.h b/tools/aapt2/ResourceTable.h index 1917d7e78c4f..3dd0a769d944 100644 --- a/tools/aapt2/ResourceTable.h +++ b/tools/aapt2/ResourceTable.h @@ -57,8 +57,27 @@ struct AllowNew { std::string comment; }; -// The policy dictating whether an entry is overlayable at runtime by RROs. +// Represents a declaration that a resource is overayable at runtime. struct Overlayable { + // Represents the types overlays that are allowed to overlay the resource. + enum class Policy { + // The resource can be overlaid by any overlay. + kPublic, + + // The resource can be overlaid by any overlay on the system partition. + kSystem, + + // The resource can be overlaid by any overlay on the vendor partition. + kVendor, + + // The resource can be overlaid by any overlay on the product partition. + kProduct, + + // The resource can be overlaid by any overlay on the product services partition. + kProductServices, + }; + + Maybe policy; Source source; std::string comment; }; @@ -96,7 +115,8 @@ class ResourceEntry { Maybe allow_new; - Maybe overlayable; + // The declarations of this resource as overlayable for RROs + std::vector overlayable_declarations; // The resource's values for each configuration. std::vector> values; @@ -226,9 +246,9 @@ class ResourceTable { bool SetVisibilityWithIdMangled(const ResourceNameRef& name, const Visibility& visibility, const ResourceId& res_id, IDiagnostics* diag); - bool SetOverlayable(const ResourceNameRef& name, const Overlayable& overlayable, + bool AddOverlayable(const ResourceNameRef& name, const Overlayable& overlayable, IDiagnostics* diag); - bool SetOverlayableMangled(const ResourceNameRef& name, const Overlayable& overlayable, + bool AddOverlayableMangled(const ResourceNameRef& name, const Overlayable& overlayable, IDiagnostics* diag); bool SetAllowNew(const ResourceNameRef& name, const AllowNew& allow_new, IDiagnostics* diag); @@ -303,7 +323,7 @@ class ResourceTable { bool SetAllowNewImpl(const ResourceNameRef& name, const AllowNew& allow_new, NameValidator name_validator, IDiagnostics* diag); - bool SetOverlayableImpl(const ResourceNameRef& name, const Overlayable& overlayable, + bool AddOverlayableImpl(const ResourceNameRef& name, const Overlayable& overlayable, NameValidator name_validator, IDiagnostics* diag); bool SetSymbolStateImpl(const ResourceNameRef& name, const ResourceId& res_id, diff --git a/tools/aapt2/ResourceTable_test.cpp b/tools/aapt2/ResourceTable_test.cpp index 05c6f1531d34..7c28f07d0f66 100644 --- a/tools/aapt2/ResourceTable_test.cpp +++ b/tools/aapt2/ResourceTable_test.cpp @@ -242,21 +242,69 @@ TEST(ResourceTableTest, SetAllowNew) { ASSERT_THAT(result.value().entry->allow_new.value().comment, StrEq("second")); } -TEST(ResourceTableTest, SetOverlayable) { +TEST(ResourceTableTest, AddOverlayable) { ResourceTable table; const ResourceName name = test::ParseNameOrDie("android:string/foo"); Overlayable overlayable; - + overlayable.policy = Overlayable::Policy::kProduct; overlayable.comment = "first"; - ASSERT_TRUE(table.SetOverlayable(name, overlayable, test::GetDiagnostics())); + ASSERT_TRUE(table.AddOverlayable(name, overlayable, test::GetDiagnostics())); Maybe result = table.FindResource(name); ASSERT_TRUE(result); - ASSERT_TRUE(result.value().entry->overlayable); - ASSERT_THAT(result.value().entry->overlayable.value().comment, StrEq("first")); + 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)); +} + +TEST(ResourceTableTest, AddDuplicateOverlayableFail) { + 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 overlayable2; + overlayable2.policy = Overlayable::Policy::kProduct; + ASSERT_FALSE(table.AddOverlayable(name, overlayable2, test::GetDiagnostics())); +} + +TEST(ResourceTableTest, AddOverlayablePolicyAndNoneFirstFail) { + ResourceTable table; + const ResourceName name = test::ParseNameOrDie("android:string/foo"); + + ASSERT_TRUE(table.AddOverlayable(name, {}, test::GetDiagnostics())); + + Overlayable overlayable2; + overlayable2.policy = Overlayable::Policy::kProduct; + ASSERT_FALSE(table.AddOverlayable(name, overlayable2, test::GetDiagnostics())); +} + +TEST(ResourceTableTest, AddOverlayablePolicyAndNoneLastFail) { + 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.comment = "second"; - ASSERT_FALSE(table.SetOverlayable(name, overlayable, test::GetDiagnostics())); + ASSERT_FALSE(table.AddOverlayable(name, {}, test::GetDiagnostics())); } TEST(ResourceTableTest, AllowDuplictaeResourcesNames) { diff --git a/tools/aapt2/Resources.proto b/tools/aapt2/Resources.proto index d7a377176fc5..bf9fe49da2d6 100644 --- a/tools/aapt2/Resources.proto +++ b/tools/aapt2/Resources.proto @@ -133,13 +133,25 @@ message AllowNew { string comment = 2; } -// Whether a resource is overlayable by runtime resource overlays (RRO). +// 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; + } + // Where this declaration was defined in source. Source source = 1; // Any comment associated with the declaration. string comment = 2; + + // The policy of the overlayable declaration + Policy policy = 3; } // An entry ID in the range [0x0000, 0xffff]. @@ -169,7 +181,7 @@ message Entry { AllowNew allow_new = 4; // Whether this resource can be overlaid by a runtime resource overlay (RRO). - Overlayable overlayable = 5; + repeated Overlayable overlayable = 5; // The set of values defined for this entry, each corresponding to a different // configuration/variant. diff --git a/tools/aapt2/format/binary/BinaryResourceParser.cpp b/tools/aapt2/format/binary/BinaryResourceParser.cpp index 3a39a6baeeeb..ed70fb3c57d6 100644 --- a/tools/aapt2/format/binary/BinaryResourceParser.cpp +++ b/tools/aapt2/format/binary/BinaryResourceParser.cpp @@ -398,7 +398,7 @@ bool BinaryResourceParser::ParseType(const ResourceTablePackage* package, if (type_spec_flags & ResTable_typeSpec::SPEC_OVERLAYABLE) { Overlayable overlayable; overlayable.source = source_.WithLine(0); - if (!table_->SetOverlayableMangled(name, overlayable, diag_)) { + if (!table_->AddOverlayableMangled(name, overlayable, diag_)) { return false; } } diff --git a/tools/aapt2/format/binary/TableFlattener.cpp b/tools/aapt2/format/binary/TableFlattener.cpp index 8641a7c93d19..8a86f63a30c1 100644 --- a/tools/aapt2/format/binary/TableFlattener.cpp +++ b/tools/aapt2/format/binary/TableFlattener.cpp @@ -446,7 +446,7 @@ class PackageFlattener { config_masks[entry->id.value()] |= util::HostToDevice32(ResTable_typeSpec::SPEC_PUBLIC); } - if (entry->overlayable) { + if (!entry->overlayable_declarations.empty()) { config_masks[entry->id.value()] |= util::HostToDevice32(ResTable_typeSpec::SPEC_OVERLAYABLE); } diff --git a/tools/aapt2/format/binary/TableFlattener_test.cpp b/tools/aapt2/format/binary/TableFlattener_test.cpp index af19b98e528b..cd1414c7e628 100644 --- a/tools/aapt2/format/binary/TableFlattener_test.cpp +++ b/tools/aapt2/format/binary/TableFlattener_test.cpp @@ -634,7 +634,7 @@ TEST_F(TableFlattenerTest, FlattenOverlayable) { .AddSimple("com.app.test:integer/overlayable", ResourceId(0x7f020000)) .Build(); - ASSERT_TRUE(table->SetOverlayable(test::ParseNameOrDie("com.app.test:integer/overlayable"), + ASSERT_TRUE(table->AddOverlayable(test::ParseNameOrDie("com.app.test:integer/overlayable"), Overlayable{}, test::GetDiagnostics())); ResTable res_table; diff --git a/tools/aapt2/format/proto/ProtoDeserialize.cpp b/tools/aapt2/format/proto/ProtoDeserialize.cpp index d1b2fdb84afc..f612914269de 100644 --- a/tools/aapt2/format/proto/ProtoDeserialize.cpp +++ b/tools/aapt2/format/proto/ProtoDeserialize.cpp @@ -437,15 +437,37 @@ static bool DeserializePackageFromPb(const pb::Package& pb_package, const ResStr entry->allow_new = std::move(allow_new); } - if (pb_entry.has_overlayable()) { - const pb::Overlayable& pb_overlayable = pb_entry.overlayable(); - + 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_overlayable.has_source()) { DeserializeSourceFromPb(pb_overlayable.source(), src_pool, &overlayable.source); } overlayable.comment = pb_overlayable.comment(); - entry->overlayable = std::move(overlayable); + entry->overlayable_declarations.push_back(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 7e35ea7bb7a3..f1e96d61191b 100644 --- a/tools/aapt2/format/proto/ProtoSerialize.cpp +++ b/tools/aapt2/format/proto/ProtoSerialize.cpp @@ -310,11 +310,31 @@ void SerializeTableToPb(const ResourceTable& table, pb::ResourceTable* out_table pb_allow_new->set_comment(entry->allow_new.value().comment); } - if (entry->overlayable) { - pb::Overlayable* pb_overlayable = pb_entry->mutable_overlayable(); - SerializeSourceToPb(entry->overlayable.value().source, &source_pool, + 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; + } + } + + SerializeSourceToPb(overlayable.source, &source_pool, pb_overlayable->mutable_source()); - pb_overlayable->set_comment(entry->overlayable.value().comment); + pb_overlayable->set_comment(overlayable.comment); } for (const std::unique_ptr& config_value : entry->values) { diff --git a/tools/aapt2/format/proto/ProtoSerialize_test.cpp b/tools/aapt2/format/proto/ProtoSerialize_test.cpp index 3c4d41ae5d1a..95dbbeb58a5d 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(expected_ref), context->GetDiagnostics())); // Make an overlayable resource. - ASSERT_TRUE(table->SetOverlayable(test::ParseNameOrDie("com.app.a:integer/overlayable"), + ASSERT_TRUE(table->AddOverlayable(test::ParseNameOrDie("com.app.a:integer/overlayable"), Overlayable{}, test::GetDiagnostics())); pb::ResourceTable pb_table; @@ -106,7 +106,7 @@ TEST(ProtoSerializeTest, SerializeSinglePackage) { ResourceTable new_table; std::string error; - ASSERT_TRUE(DeserializeTableFromPb(pb_table, &files, &new_table, &error)); + ASSERT_TRUE(DeserializeTableFromPb(pb_table, &files, &new_table, &error)) << error; EXPECT_THAT(error, IsEmpty()); Id* new_id = test::GetValue(&new_table, "com.app.a:id/foo"); @@ -160,7 +160,8 @@ 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_TRUE(search_result.value().entry->overlayable); + EXPECT_THAT(search_result.value().entry->overlayable_declarations.size(), Eq(1)); + EXPECT_FALSE(search_result.value().entry->overlayable_declarations[0].policy); } TEST(ProtoSerializeTest, SerializeAndDeserializeXml) { @@ -464,4 +465,59 @@ TEST(ProtoSerializeTest, SerializeDeserializeConfiguration) { "night-xhdpi-stylus-keysexposed-qwerty-navhidden-dpad-300x200-v23"); } +TEST(ProtoSerializeTest, SerializeAndDeserializeOverlayable) { + std::unique_ptr context = test::ContextBuilder().Build(); + std::unique_ptr 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", {}) + .AddValue("com.app.a:bool/fiz", ResourceUtils::TryParseBool("true")) + .Build(); + + pb::ResourceTable pb_table; + SerializeTableToPb(*table, &pb_table, context->GetDiagnostics()); + + MockFileCollection files; + ResourceTable new_table; + std::string error; + ASSERT_TRUE(DeserializeTableFromPb(pb_table, &files, &new_table, &error)); + EXPECT_THAT(error, IsEmpty()); + + Maybe 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)); + + 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)); + + 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)); + + 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); + + result = new_table.FindResource(test::ParseNameOrDie("com.app.a:bool/fiz")); + ASSERT_TRUE(result); + EXPECT_THAT(result.value().entry->overlayable_declarations.size(), Eq(0)); +} + } // namespace aapt diff --git a/tools/aapt2/link/TableMerger.cpp b/tools/aapt2/link/TableMerger.cpp index afb8ae097449..d777e22fa4b7 100644 --- a/tools/aapt2/link/TableMerger.cpp +++ b/tools/aapt2/link/TableMerger.cpp @@ -101,7 +101,7 @@ static bool MergeType(IAaptContext* context, const Source& src, ResourceTableTyp return true; } -static bool MergeEntry(IAaptContext* context, const Source& src, bool overlay, +static bool MergeEntry(IAaptContext* context, const Source& src, ResourceEntry* dst_entry, ResourceEntry* src_entry, bool strict_visibility) { if (strict_visibility @@ -134,17 +134,35 @@ static bool MergeEntry(IAaptContext* context, const Source& src, bool overlay, dst_entry->allow_new = std::move(src_entry->allow_new); } - if (src_entry->overlayable) { - if (dst_entry->overlayable && !overlay) { - 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; + 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; + } } - 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; } @@ -244,7 +262,7 @@ bool TableMerger::DoMerge(const Source& src, ResourceTable* src_table, continue; } - if (!MergeEntry(context_, src, overlay, dst_entry, src_entry.get(), options_.strict_visibility)) { + if (!MergeEntry(context_, src, dst_entry, src_entry.get(), options_.strict_visibility)) { error = true; continue; } diff --git a/tools/aapt2/link/TableMerger_test.cpp b/tools/aapt2/link/TableMerger_test.cpp index 79a734bffabd..d6579d37b452 100644 --- a/tools/aapt2/link/TableMerger_test.cpp +++ b/tools/aapt2/link/TableMerger_test.cpp @@ -436,4 +436,97 @@ TEST_F(TableMergerTest, OverlaidStyleablesAndStylesShouldBeMerged) { Eq(make_value(Reference(test::ParseNameOrDie("com.app.a:style/OverlayParent"))))); } +TEST_F(TableMergerTest, AddOverlayable) { + std::unique_ptr table_a = + test::ResourceTableBuilder() + .SetPackageId("com.app.a", 0x7f) + .AddOverlayable("bool/foo", Overlayable::Policy::kProduct) + .Build(); + + std::unique_ptr table_b = + test::ResourceTableBuilder() + .SetPackageId("com.app.a", 0x7f) + .AddOverlayable("bool/foo", Overlayable::Policy::kProductServices) + .Build(); + + ResourceTable final_table; + TableMergerOptions options; + options.auto_add_overlay = true; + TableMerger merger(context_.get(), &final_table, options); + ASSERT_TRUE(merger.Merge({}, table_a.get(), false /*overlay*/)); + ASSERT_TRUE(merger.Merge({}, table_b.get(), false /*overlay*/)); + + const ResourceName name = test::ParseNameOrDie("com.app.a:bool/foo"); + Maybe 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)); +} + +TEST_F(TableMergerTest, AddDuplicateOverlayableFail) { + std::unique_ptr table_a = + test::ResourceTableBuilder() + .SetPackageId("com.app.a", 0x7f) + .AddOverlayable("bool/foo", Overlayable::Policy::kProduct) + .Build(); + + std::unique_ptr table_b = + test::ResourceTableBuilder() + .SetPackageId("com.app.a", 0x7f) + .AddOverlayable("bool/foo", Overlayable::Policy::kProduct) + .Build(); + + ResourceTable final_table; + TableMergerOptions options; + 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*/)); +} + +TEST_F(TableMergerTest, AddOverlayablePolicyAndNoneFirstFail) { + std::unique_ptr table_a = + test::ResourceTableBuilder() + .SetPackageId("com.app.a", 0x7f) + .AddOverlayable("bool/foo", {}) + .Build(); + + std::unique_ptr table_b = + test::ResourceTableBuilder() + .SetPackageId("com.app.a", 0x7f) + .AddOverlayable("bool/foo", Overlayable::Policy::kProduct) + .Build(); + + ResourceTable final_table; + TableMergerOptions options; + 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*/)); +} + +TEST_F(TableMergerTest, AddOverlayablePolicyAndNoneLastFail) { + std::unique_ptr table_a = + test::ResourceTableBuilder() + .SetPackageId("com.app.a", 0x7f) + .AddOverlayable("bool/foo", Overlayable::Policy::kProduct) + .Build(); + + std::unique_ptr table_b = + test::ResourceTableBuilder() + .SetPackageId("com.app.a", 0x7f) + .AddOverlayable("bool/foo", {}) + .Build(); + + ResourceTable final_table; + TableMergerOptions options; + 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*/)); +} + } // namespace aapt diff --git a/tools/aapt2/test/Builders.cpp b/tools/aapt2/test/Builders.cpp index f33ae3155192..03b59e033402 100644 --- a/tools/aapt2/test/Builders.cpp +++ b/tools/aapt2/test/Builders.cpp @@ -135,6 +135,15 @@ ResourceTableBuilder& ResourceTableBuilder::SetSymbolState(const StringPiece& na return *this; } +ResourceTableBuilder& ResourceTableBuilder::AddOverlayable(const StringPiece& name, + const Maybe p) { + ResourceName res_name = ParseNameOrDie(name); + Overlayable overlayable; + overlayable.policy = p; + CHECK(table_->AddOverlayable(res_name, overlayable, GetDiagnostics())); + return *this; +} + StringPool* ResourceTableBuilder::string_pool() { return &table_->string_pool; } diff --git a/tools/aapt2/test/Builders.h b/tools/aapt2/test/Builders.h index 915959972bed..d68c24ddc665 100644 --- a/tools/aapt2/test/Builders.h +++ b/tools/aapt2/test/Builders.h @@ -73,6 +73,8 @@ class ResourceTableBuilder { const ResourceId& id, std::unique_ptr value); ResourceTableBuilder& SetSymbolState(const android::StringPiece& name, const ResourceId& id, Visibility::Level level, bool allow_new = false); + ResourceTableBuilder& AddOverlayable(const android::StringPiece& name, + Maybe policy); StringPool* string_pool(); std::unique_ptr Build(); -- cgit v1.2.3-59-g8ed1b From 4e9a922ede24f7f7bfe793321f7328623ee2a061 Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Tue, 13 Nov 2018 10:40:07 -0800 Subject: Fix loaded apk string pool order Loading in an APk changed the ordering of strings in the string pool. When loading an apk, assign the strings to the same index as they are in the ResStringPool. Bug: 118831219 Test: "aapt2 dump strings left.apk" prints in the correct order, "aapt2 convert left.apk --output-format binary -o left_binary.apk" has entries in the correct order, and aapt2_tests Change-Id: I00014c02195f39c1152a110e90083d9b14e9216e --- tools/aapt2/Debug.cpp | 4 +- tools/aapt2/ResourceUtils.cpp | 4 +- tools/aapt2/StringPool.cpp | 22 +++- tools/aapt2/StringPool.h | 6 +- tools/aapt2/StringPool_test.cpp | 18 +++ tools/aapt2/cmd/Convert.cpp | 182 ++++++++++++++------------- tools/aapt2/cmd/Convert.h | 6 +- tools/aapt2/format/binary/TableFlattener.cpp | 20 +-- tools/aapt2/format/binary/TableFlattener.h | 3 + 9 files changed, 158 insertions(+), 107 deletions(-) (limited to 'tools/aapt2/Debug.cpp') diff --git a/tools/aapt2/Debug.cpp b/tools/aapt2/Debug.cpp index 0bc5221245ca..583f14ac0cbd 100644 --- a/tools/aapt2/Debug.cpp +++ b/tools/aapt2/Debug.cpp @@ -431,7 +431,7 @@ void Debug::DumpHex(const void* data, size_t len) { void Debug::DumpResStringPool(const android::ResStringPool* pool, text::Printer* printer) { using namespace android; - + if (pool->getError() == NO_INIT) { printer->Print("String pool is unitialized.\n"); return; @@ -460,7 +460,7 @@ void Debug::DumpResStringPool(const android::ResStringPool* pool, text::Printer* const size_t NS = pool->size(); for (size_t s=0; sstring8ObjectAt(s); - printer->Print(StringPrintf("String #%zd : %s\n", s, str.string())); + printer->Print(StringPrintf("String #%zd: %s\n", s, str.string())); } } diff --git a/tools/aapt2/ResourceUtils.cpp b/tools/aapt2/ResourceUtils.cpp index dbe5ac5adb98..da22e885b917 100644 --- a/tools/aapt2/ResourceUtils.cpp +++ b/tools/aapt2/ResourceUtils.cpp @@ -718,7 +718,7 @@ std::unique_ptr ParseBinaryResValue(const ResourceType& type, const Config // This must be a FileReference. std::unique_ptr file_ref = util::make_unique(dst_pool->MakeRef( - str, StringPool::Context(StringPool::Context::kHighPriority, config))); + str, StringPool::Context(StringPool::Context::kHighPriority, config), data)); if (type == ResourceType::kRaw) { file_ref->type = ResourceFile::Type::kUnknown; } else if (util::EndsWith(*file_ref->path, ".xml")) { @@ -730,7 +730,7 @@ std::unique_ptr ParseBinaryResValue(const ResourceType& type, const Config } // There are no styles associated with this string, so treat it as a simple string. - return util::make_unique(dst_pool->MakeRef(str, StringPool::Context(config))); + return util::make_unique(dst_pool->MakeRef(str, StringPool::Context(config), data)); } } break; diff --git a/tools/aapt2/StringPool.cpp b/tools/aapt2/StringPool.cpp index 8eabd3225d87..a8c26668b3a5 100644 --- a/tools/aapt2/StringPool.cpp +++ b/tools/aapt2/StringPool.cpp @@ -165,12 +165,13 @@ StringPool::Ref StringPool::MakeRef(const StringPiece& str) { return MakeRefImpl(str, Context{}, true); } -StringPool::Ref StringPool::MakeRef(const StringPiece& str, const Context& context) { - return MakeRefImpl(str, context, true); +StringPool::Ref StringPool::MakeRef(const StringPiece& str, const Context& context, + Maybe index) { + return MakeRefImpl(str, context, true, index); } StringPool::Ref StringPool::MakeRefImpl(const StringPiece& str, const Context& context, - bool unique) { + bool unique, Maybe index) { if (unique) { auto range = indexed_strings_.equal_range(str); for (auto iter = range.first; iter != range.second; ++iter) { @@ -180,15 +181,26 @@ StringPool::Ref StringPool::MakeRefImpl(const StringPiece& str, const Context& c } } + const size_t size = strings_.size(); + // Insert the string at the end of the string vector if no index is specified + const size_t insertion_index = index ? index.value() : size; + std::unique_ptr entry(new Entry()); entry->value = str.to_string(); entry->context = context; - entry->index_ = strings_.size(); + entry->index_ = insertion_index; entry->ref_ = 0; entry->pool_ = this; Entry* borrow = entry.get(); - strings_.emplace_back(std::move(entry)); + if (insertion_index == size) { + strings_.emplace_back(std::move(entry)); + } else { + // Allocate enough space for the string at the index + strings_.resize(std::max(insertion_index + 1, size)); + strings_[insertion_index] = std::move(entry); + } + indexed_strings_.insert(std::make_pair(StringPiece(borrow->value), borrow)); return Ref(borrow); } diff --git a/tools/aapt2/StringPool.h b/tools/aapt2/StringPool.h index 1006ca970dc5..115d5d315b8f 100644 --- a/tools/aapt2/StringPool.h +++ b/tools/aapt2/StringPool.h @@ -166,7 +166,8 @@ class StringPool { // Adds a string to the pool, unless it already exists, with a context object that can be used // when sorting the string pool. Returns a reference to the string in the pool. - Ref MakeRef(const android::StringPiece& str, const Context& context); + Ref MakeRef(const android::StringPiece& str, const Context& context, + Maybe index = {}); // Adds a string from another string pool. Returns a reference to the string in the string pool. Ref MakeRef(const Ref& ref); @@ -210,7 +211,8 @@ class StringPool { static bool Flatten(BigBuffer* out, const StringPool& pool, bool utf8, IDiagnostics* diag); - Ref MakeRefImpl(const android::StringPiece& str, const Context& context, bool unique); + Ref MakeRefImpl(const android::StringPiece& str, const Context& context, bool unique, + Maybe index = {}); void ReAssignIndices(); std::vector> strings_; diff --git a/tools/aapt2/StringPool_test.cpp b/tools/aapt2/StringPool_test.cpp index 9a7238b584ba..648be7d33754 100644 --- a/tools/aapt2/StringPool_test.cpp +++ b/tools/aapt2/StringPool_test.cpp @@ -84,6 +84,24 @@ TEST(StringPoolTest, MaintainInsertionOrderIndex) { EXPECT_THAT(ref_c.index(), Eq(2u)); } +TEST(StringPoolTest, AssignStringIndex) { + StringPool pool; + + StringPool::Ref ref_a = pool.MakeRef("0", StringPool::Context{}, 0u); + StringPool::Ref ref_b = pool.MakeRef("1", StringPool::Context{}, 1u); + StringPool::Ref ref_c = pool.MakeRef("5", StringPool::Context{}, 5u); + StringPool::Ref ref_d = pool.MakeRef("2", StringPool::Context{}, 2u); + StringPool::Ref ref_e = pool.MakeRef("4", StringPool::Context{}, 4u); + StringPool::Ref ref_f = pool.MakeRef("3", StringPool::Context{}, 3u); + + EXPECT_THAT(ref_a.index(), Eq(0u)); + EXPECT_THAT(ref_b.index(), Eq(1u)); + EXPECT_THAT(ref_d.index(), Eq(2u)); + EXPECT_THAT(ref_f.index(), Eq(3u)); + EXPECT_THAT(ref_e.index(), Eq(4u)); + EXPECT_THAT(ref_c.index(), Eq(5u)); +} + TEST(StringPoolTest, PruneStringsWithNoReferences) { StringPool pool; diff --git a/tools/aapt2/cmd/Convert.cpp b/tools/aapt2/cmd/Convert.cpp index 3ea17552ea7c..4492f6b49cf0 100644 --- a/tools/aapt2/cmd/Convert.cpp +++ b/tools/aapt2/cmd/Convert.cpp @@ -57,78 +57,6 @@ class IApkSerializer { Source source_; }; -bool ConvertApk(IAaptContext* context, unique_ptr apk, IApkSerializer* serializer, - IArchiveWriter* writer) { - io::IFile* manifest = apk->GetFileCollection()->FindFile(kAndroidManifestPath); - if (!serializer->SerializeXml(apk->GetManifest(), kAndroidManifestPath, true /*utf16*/, writer, - (manifest != nullptr && manifest->WasCompressed()) - ? ArchiveEntry::kCompress : 0u)) { - context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) - << "failed to serialize AndroidManifest.xml"); - return false; - } - - if (apk->GetResourceTable() != nullptr) { - // The table might be modified by below code. - auto converted_table = apk->GetResourceTable(); - - // Resources - for (const auto& package : converted_table->packages) { - for (const auto& type : package->types) { - for (const auto& entry : type->entries) { - for (const auto& config_value : entry->values) { - FileReference* file = ValueCast(config_value->value.get()); - if (file != nullptr) { - if (file->file == nullptr) { - context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) - << "no file associated with " << *file); - return false; - } - - if (!serializer->SerializeFile(file, writer)) { - context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) - << "failed to serialize file " << *file->path); - return false; - } - } // file - } // config_value - } // entry - } // type - } // package - - // Converted resource table - if (!serializer->SerializeTable(converted_table, writer)) { - context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) - << "failed to serialize the resource table"); - return false; - } - } - - // Other files - std::unique_ptr iterator = apk->GetFileCollection()->Iterator(); - while (iterator->HasNext()) { - io::IFile* file = iterator->Next(); - std::string path = file->GetSource().path; - - // Manifest, resource table and resources have already been taken care of. - if (path == kAndroidManifestPath || - path == kApkResourceTablePath || - path == kProtoResourceTablePath || - path.find("res/") == 0) { - continue; - } - - if (!io::CopyFileToArchivePreserveCompression(context, file, path, writer)) { - context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) - << "failed to copy file " << path); - return false; - } - } - - return true; -} - - class BinaryApkSerializer : public IApkSerializer { public: BinaryApkSerializer(IAaptContext* context, const Source& source, @@ -323,12 +251,97 @@ class Context : public IAaptContext { StdErrDiagnostics diag_; }; +int Convert(IAaptContext* context, LoadedApk* apk, IArchiveWriter* output_writer, + ApkFormat output_format, TableFlattenerOptions& options) { + // Do not change the ordering of strings in the values string pool + options.sort_stringpool_entries = false; + + unique_ptr serializer; + if (output_format == ApkFormat::kBinary) { + serializer.reset(new BinaryApkSerializer(context, apk->GetSource(), options)); + } else if (output_format == ApkFormat::kProto) { + serializer.reset(new ProtoApkSerializer(context, apk->GetSource())); + } else { + context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) + << "Cannot convert APK to unknown format"); + return 1; + } + + io::IFile* manifest = apk->GetFileCollection()->FindFile(kAndroidManifestPath); + if (!serializer->SerializeXml(apk->GetManifest(), kAndroidManifestPath, true /*utf16*/, + output_writer, (manifest != nullptr && manifest->WasCompressed()) + ? ArchiveEntry::kCompress : 0u)) { + context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) + << "failed to serialize AndroidManifest.xml"); + return 1; + } + + if (apk->GetResourceTable() != nullptr) { + // The table might be modified by below code. + auto converted_table = apk->GetResourceTable(); + + // Resources + for (const auto& package : converted_table->packages) { + for (const auto& type : package->types) { + for (const auto& entry : type->entries) { + for (const auto& config_value : entry->values) { + FileReference* file = ValueCast(config_value->value.get()); + if (file != nullptr) { + if (file->file == nullptr) { + context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) + << "no file associated with " << *file); + return 1; + } + + if (!serializer->SerializeFile(file, output_writer)) { + context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) + << "failed to serialize file " << *file->path); + return 1; + } + } // file + } // config_value + } // entry + } // type + } // package + + // Converted resource table + if (!serializer->SerializeTable(converted_table, output_writer)) { + context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) + << "failed to serialize the resource table"); + return 1; + } + } + + // Other files + std::unique_ptr iterator = apk->GetFileCollection()->Iterator(); + while (iterator->HasNext()) { + io::IFile* file = iterator->Next(); + std::string path = file->GetSource().path; + + // Manifest, resource table and resources have already been taken care of. + if (path == kAndroidManifestPath || + path == kApkResourceTablePath || + path == kProtoResourceTablePath || + path.find("res/") == 0) { + continue; + } + + if (!io::CopyFileToArchivePreserveCompression(context, file, path, output_writer)) { + context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) + << "failed to copy file " << path); + return 1; + } + } + + return 0; +} + const char* ConvertCommand::kOutputFormatProto = "proto"; const char* ConvertCommand::kOutputFormatBinary = "binary"; int ConvertCommand::Action(const std::vector& args) { if (args.size() != 1) { - std::cerr << "must supply a single proto APK\n"; + std::cerr << "must supply a single APK\n"; Usage(&std::cerr); return 1; } @@ -341,34 +354,31 @@ int ConvertCommand::Action(const std::vector& args) { return 1; } - Maybe app_info = - ExtractAppInfoFromBinaryManifest(*apk->GetManifest(), context.GetDiagnostics()); + Maybe app_info = ExtractAppInfoFromBinaryManifest(*apk->GetManifest(), + context.GetDiagnostics()); if (!app_info) { return 1; } context.package_ = app_info.value().package; - - unique_ptr writer = - CreateZipFileArchiveWriter(context.GetDiagnostics(), output_path_); + unique_ptr writer = CreateZipFileArchiveWriter(context.GetDiagnostics(), + output_path_); if (writer == nullptr) { return 1; } - unique_ptr serializer; + ApkFormat format; if (!output_format_ || output_format_.value() == ConvertCommand::kOutputFormatBinary) { - - serializer.reset(new BinaryApkSerializer(&context, apk->GetSource(), options_)); + format = ApkFormat::kBinary; } else if (output_format_.value() == ConvertCommand::kOutputFormatProto) { - serializer.reset(new ProtoApkSerializer(&context, apk->GetSource())); + format = ApkFormat::kProto; } else { - context.GetDiagnostics()->Error(DiagMessage(path) - << "Invalid value for flag --output-format: " - << output_format_.value()); + context.GetDiagnostics()->Error(DiagMessage(path) << "Invalid value for flag --output-format: " + << output_format_.value()); return 1; } - return ConvertApk(&context, std::move(apk), serializer.get(), writer.get()) ? 0 : 1; + return Convert(&context, apk.get(), writer.get(), format, options_); } } // namespace aapt diff --git a/tools/aapt2/cmd/Convert.h b/tools/aapt2/cmd/Convert.h index fcec23d16f78..6a6719c91b58 100644 --- a/tools/aapt2/cmd/Convert.h +++ b/tools/aapt2/cmd/Convert.h @@ -18,6 +18,7 @@ #define AAPT2_CONVERT_H #include "Command.h" +#include "LoadedApk.h" #include "format/binary/TableFlattener.h" namespace aapt { @@ -49,6 +50,9 @@ class ConvertCommand : public Command { bool verbose_ = false; }; -}// namespace aapt +int Convert(IAaptContext* context, LoadedApk* input, IArchiveWriter* output_writer, + ApkFormat output_format, TableFlattenerOptions& options); + +} // namespace aapt #endif //AAPT2_CONVERT_H diff --git a/tools/aapt2/format/binary/TableFlattener.cpp b/tools/aapt2/format/binary/TableFlattener.cpp index 8a86f63a30c1..6c1a9ba2cbad 100644 --- a/tools/aapt2/format/binary/TableFlattener.cpp +++ b/tools/aapt2/format/binary/TableFlattener.cpp @@ -573,15 +573,17 @@ class PackageFlattener { } // namespace bool TableFlattener::Consume(IAaptContext* context, ResourceTable* table) { - // We must do this before writing the resources, since the string pool IDs may change. - table->string_pool.Prune(); - table->string_pool.Sort([](const StringPool::Context& a, const StringPool::Context& b) -> int { - int diff = util::compare(a.priority, b.priority); - if (diff == 0) { - diff = a.config.compare(b.config); - } - return diff; - }); + if (options_.sort_stringpool_entries) { + // We must do this before writing the resources, since the string pool IDs may change. + table->string_pool.Prune(); + table->string_pool.Sort([](const StringPool::Context &a, const StringPool::Context &b) -> int { + int diff = util::compare(a.priority, b.priority); + if (diff == 0) { + diff = a.config.compare(b.config); + } + return diff; + }); + } // Write the ResTable header. ChunkWriter table_writer(buffer_); diff --git a/tools/aapt2/format/binary/TableFlattener.h b/tools/aapt2/format/binary/TableFlattener.h index c2e1d4b4ce8c..635cb21f514c 100644 --- a/tools/aapt2/format/binary/TableFlattener.h +++ b/tools/aapt2/format/binary/TableFlattener.h @@ -43,6 +43,9 @@ struct TableFlattenerOptions { // Set of whitelisted resource names to avoid altering in key stringpool std::set whitelisted_resources; + + // When true, sort the entries in the values string pool by priority and configuration. + bool sort_stringpool_entries = true; }; class TableFlattener : public IResourceTableConsumer { -- cgit v1.2.3-59-g8ed1b From 1bb1fe068a7e719711963c3cf3a50209e083a17f Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Fri, 16 Nov 2018 11:21:41 -0800 Subject: 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 --- libs/androidfw/include/androidfw/ResourceTypes.h | 2 +- tools/aapt2/Debug.cpp | 25 ----- tools/aapt2/ResourceParser.cpp | 124 ++++++++------------- tools/aapt2/ResourceParser.h | 4 - tools/aapt2/ResourceParser_test.cpp | 110 +++++++----------- tools/aapt2/ResourceTable.cpp | 37 ++---- tools/aapt2/ResourceTable.h | 33 +++--- tools/aapt2/ResourceTable_test.cpp | 73 +++++------- tools/aapt2/Resources.proto | 17 ++- tools/aapt2/cmd/Dump.h | 1 + tools/aapt2/format/binary/BinaryResourceParser.cpp | 24 ++-- tools/aapt2/format/binary/TableFlattener.cpp | 84 +++++++------- tools/aapt2/format/binary/TableFlattener_test.cpp | 103 ++++++++--------- tools/aapt2/format/proto/ProtoDeserialize.cpp | 52 ++++----- tools/aapt2/format/proto/ProtoSerialize.cpp | 38 +++---- tools/aapt2/format/proto/ProtoSerialize_test.cpp | 81 ++++++++------ tools/aapt2/link/ReferenceLinker.cpp | 4 +- tools/aapt2/link/TableMerger.cpp | 38 ++----- tools/aapt2/link/TableMerger_test.cpp | 62 +++++++---- tools/aapt2/split/TableSplitter.cpp | 2 +- tools/aapt2/test/Builders.cpp | 9 +- tools/aapt2/test/Builders.h | 4 +- 22 files changed, 412 insertions(+), 515 deletions(-) (limited to 'tools/aapt2/Debug.cpp') 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_declarations; + Maybe overlayable; std::string comment; std::unique_ptr 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 tag"); } - std::string comment; - std::vector 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 maybe_name = xml::FindNonEmptyAttribute(parser, "name"); + if (!maybe_name) { + diag_->Error(DiagMessage(item_source) + << " within an tag must have a 'name' attribute"); + error = true; + continue; + } + + Maybe maybe_type = xml::FindNonEmptyAttribute(parser, "type"); + if (!maybe_type) { + diag_->Error(DiagMessage(item_source) + << " within an 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 within an "); + 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) << " blocks cannot be recursively nested"); error = true; break; } else if (Maybe 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) - << " has unsupported type '" << trimmed_part << "'"); + diag_->Error(DiagMessage(item_source) + << " 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 " - << " "); + diag_->Error(DiagMessage(item_source) << "invalid element <" << element_name << "> " + << " in "); 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& policies, - const std::string& comment, - ParsedResource* out_resource) { - const Source item_source = source_.WithLine(parser->line_number()); - - Maybe maybe_name = xml::FindNonEmptyAttribute(parser, "name"); - if (!maybe_name) { - diag_->Error(DiagMessage(item_source) - << " within an tag must have a 'name' attribute"); - return false; - } - - Maybe maybe_type = xml::FindNonEmptyAttribute(parser, "type"); - if (!maybe_type) { - diag_->Error(DiagMessage(item_source) - << " within an 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 within an "); - 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& 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"( - + @@ -1080,45 +1071,30 @@ TEST_F(ResourceParserTest, DuplicateOverlayableIsError) { - + + )"; + EXPECT_FALSE(TestParse(input)); + input = R"( - )"; - EXPECT_FALSE(TestParse(input)); -} - -TEST_F(ResourceParserTest, PolicyAndNonPolicyOverlayableError) { - std::string input = R"( - - - - + - )"; + + )"; EXPECT_FALSE(TestParse(input)); input = R"( - - - - - - )"; - EXPECT_FALSE(TestParse(input)); -} - -TEST_F(ResourceParserTest, DuplicateOverlayableMultiplePolicyError) { - std::string input = R"( - + + - + )"; 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::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; + typedef uint32_t PolicyFlags; + PolicyFlags policies = Policy::kNone; + Source source; std::string comment; }; @@ -116,7 +121,7 @@ class ResourceEntry { Maybe allow_new; // The declarations of this resource as overlayable for RROs - std::vector overlayable_declarations; + Maybe overlayable; // The resource's values for each configuration. std::vector> 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 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 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(printer, diag_)); AddOptionalSubcommand(util::make_unique(printer, diag_)); AddOptionalSubcommand(util::make_unique(printer), /* hidden */ true); + // TODO(b/120609160): Add aapt2 overlayable dump command } int Action(const std::vector& 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(parser.chunk()); - std::vector 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( @@ -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 "; for (auto& entry : type->entries) { CHECK(bool(type->id)) << "entry must have an ID set when flattening "; + 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 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 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(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 context = test::ContextBuilder().Build(); std::unique_ptr 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 result = + Maybe 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 table_a = test::ResourceTableBuilder() .SetPackageId("com.app.a", 0x7f) - .AddOverlayable("bool/foo", Overlayable::Policy::kProduct) + .SetOverlayable("bool/foo", overlayable) .Build(); std::unique_ptr 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 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 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 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 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 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 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 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 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 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 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); ResourceTableBuilder& SetSymbolState(const android::StringPiece& name, const ResourceId& id, Visibility::Level level, bool allow_new = false); - ResourceTableBuilder& AddOverlayable(const android::StringPiece& name, - Maybe policy); + ResourceTableBuilder& SetOverlayable(const android::StringPiece& name, + const Overlayable& overlayable); StringPool* string_pool(); std::unique_ptr Build(); -- cgit v1.2.3-59-g8ed1b From 90b7a08aaf189824ec89c99063ae73c0de69fcdf Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Fri, 15 Feb 2019 17:39:58 +0000 Subject: Revert "Fix loaded apk string pool order" This reverts commit 4e9a922ede24f7f7bfe793321f7328623ee2a061. Reason for revert: Change-Id: I3650b2c6c9bdfa69a3034f9ca49e95a9698c3cdd --- tools/aapt2/Debug.cpp | 2 +- tools/aapt2/ResourceUtils.cpp | 4 ++-- tools/aapt2/StringPool.cpp | 22 +++++----------------- tools/aapt2/StringPool.h | 6 ++---- tools/aapt2/StringPool_test.cpp | 18 ------------------ tools/aapt2/cmd/Convert.cpp | 17 +++++++---------- tools/aapt2/format/binary/TableFlattener.cpp | 20 +++++++++----------- tools/aapt2/format/binary/TableFlattener.h | 3 --- 8 files changed, 26 insertions(+), 66 deletions(-) (limited to 'tools/aapt2/Debug.cpp') diff --git a/tools/aapt2/Debug.cpp b/tools/aapt2/Debug.cpp index 5f664f5fdd5c..98324850f3f5 100644 --- a/tools/aapt2/Debug.cpp +++ b/tools/aapt2/Debug.cpp @@ -435,7 +435,7 @@ void Debug::DumpResStringPool(const android::ResStringPool* pool, text::Printer* const size_t NS = pool->size(); for (size_t s=0; sstring8ObjectAt(s); - printer->Print(StringPrintf("String #%zd: %s\n", s, str.string())); + printer->Print(StringPrintf("String #%zd : %s\n", s, str.string())); } } diff --git a/tools/aapt2/ResourceUtils.cpp b/tools/aapt2/ResourceUtils.cpp index ab4805f626a5..0032960ff93e 100644 --- a/tools/aapt2/ResourceUtils.cpp +++ b/tools/aapt2/ResourceUtils.cpp @@ -727,7 +727,7 @@ std::unique_ptr ParseBinaryResValue(const ResourceType& type, const Config // This must be a FileReference. std::unique_ptr file_ref = util::make_unique(dst_pool->MakeRef( - str, StringPool::Context(StringPool::Context::kHighPriority, config), data)); + str, StringPool::Context(StringPool::Context::kHighPriority, config))); if (type == ResourceType::kRaw) { file_ref->type = ResourceFile::Type::kUnknown; } else if (util::EndsWith(*file_ref->path, ".xml")) { @@ -739,7 +739,7 @@ std::unique_ptr ParseBinaryResValue(const ResourceType& type, const Config } // There are no styles associated with this string, so treat it as a simple string. - return util::make_unique(dst_pool->MakeRef(str, StringPool::Context(config), data)); + return util::make_unique(dst_pool->MakeRef(str, StringPool::Context(config))); } } break; diff --git a/tools/aapt2/StringPool.cpp b/tools/aapt2/StringPool.cpp index a8c26668b3a5..8eabd3225d87 100644 --- a/tools/aapt2/StringPool.cpp +++ b/tools/aapt2/StringPool.cpp @@ -165,13 +165,12 @@ StringPool::Ref StringPool::MakeRef(const StringPiece& str) { return MakeRefImpl(str, Context{}, true); } -StringPool::Ref StringPool::MakeRef(const StringPiece& str, const Context& context, - Maybe index) { - return MakeRefImpl(str, context, true, index); +StringPool::Ref StringPool::MakeRef(const StringPiece& str, const Context& context) { + return MakeRefImpl(str, context, true); } StringPool::Ref StringPool::MakeRefImpl(const StringPiece& str, const Context& context, - bool unique, Maybe index) { + bool unique) { if (unique) { auto range = indexed_strings_.equal_range(str); for (auto iter = range.first; iter != range.second; ++iter) { @@ -181,26 +180,15 @@ StringPool::Ref StringPool::MakeRefImpl(const StringPiece& str, const Context& c } } - const size_t size = strings_.size(); - // Insert the string at the end of the string vector if no index is specified - const size_t insertion_index = index ? index.value() : size; - std::unique_ptr entry(new Entry()); entry->value = str.to_string(); entry->context = context; - entry->index_ = insertion_index; + entry->index_ = strings_.size(); entry->ref_ = 0; entry->pool_ = this; Entry* borrow = entry.get(); - if (insertion_index == size) { - strings_.emplace_back(std::move(entry)); - } else { - // Allocate enough space for the string at the index - strings_.resize(std::max(insertion_index + 1, size)); - strings_[insertion_index] = std::move(entry); - } - + strings_.emplace_back(std::move(entry)); indexed_strings_.insert(std::make_pair(StringPiece(borrow->value), borrow)); return Ref(borrow); } diff --git a/tools/aapt2/StringPool.h b/tools/aapt2/StringPool.h index 115d5d315b8f..1006ca970dc5 100644 --- a/tools/aapt2/StringPool.h +++ b/tools/aapt2/StringPool.h @@ -166,8 +166,7 @@ class StringPool { // Adds a string to the pool, unless it already exists, with a context object that can be used // when sorting the string pool. Returns a reference to the string in the pool. - Ref MakeRef(const android::StringPiece& str, const Context& context, - Maybe index = {}); + Ref MakeRef(const android::StringPiece& str, const Context& context); // Adds a string from another string pool. Returns a reference to the string in the string pool. Ref MakeRef(const Ref& ref); @@ -211,8 +210,7 @@ class StringPool { static bool Flatten(BigBuffer* out, const StringPool& pool, bool utf8, IDiagnostics* diag); - Ref MakeRefImpl(const android::StringPiece& str, const Context& context, bool unique, - Maybe index = {}); + Ref MakeRefImpl(const android::StringPiece& str, const Context& context, bool unique); void ReAssignIndices(); std::vector> strings_; diff --git a/tools/aapt2/StringPool_test.cpp b/tools/aapt2/StringPool_test.cpp index 648be7d33754..9a7238b584ba 100644 --- a/tools/aapt2/StringPool_test.cpp +++ b/tools/aapt2/StringPool_test.cpp @@ -84,24 +84,6 @@ TEST(StringPoolTest, MaintainInsertionOrderIndex) { EXPECT_THAT(ref_c.index(), Eq(2u)); } -TEST(StringPoolTest, AssignStringIndex) { - StringPool pool; - - StringPool::Ref ref_a = pool.MakeRef("0", StringPool::Context{}, 0u); - StringPool::Ref ref_b = pool.MakeRef("1", StringPool::Context{}, 1u); - StringPool::Ref ref_c = pool.MakeRef("5", StringPool::Context{}, 5u); - StringPool::Ref ref_d = pool.MakeRef("2", StringPool::Context{}, 2u); - StringPool::Ref ref_e = pool.MakeRef("4", StringPool::Context{}, 4u); - StringPool::Ref ref_f = pool.MakeRef("3", StringPool::Context{}, 3u); - - EXPECT_THAT(ref_a.index(), Eq(0u)); - EXPECT_THAT(ref_b.index(), Eq(1u)); - EXPECT_THAT(ref_d.index(), Eq(2u)); - EXPECT_THAT(ref_f.index(), Eq(3u)); - EXPECT_THAT(ref_e.index(), Eq(4u)); - EXPECT_THAT(ref_c.index(), Eq(5u)); -} - TEST(StringPoolTest, PruneStringsWithNoReferences) { StringPool pool; diff --git a/tools/aapt2/cmd/Convert.cpp b/tools/aapt2/cmd/Convert.cpp index 7a74ba925ba0..0cf86ccdd59f 100644 --- a/tools/aapt2/cmd/Convert.cpp +++ b/tools/aapt2/cmd/Convert.cpp @@ -43,7 +43,8 @@ namespace aapt { class IApkSerializer { public: - IApkSerializer(IAaptContext* context, const Source& source) : context_(context), source_(source) {} + IApkSerializer(IAaptContext* context, const Source& source) : context_(context), + source_(source) {} virtual bool SerializeXml(const xml::XmlResource* xml, const std::string& path, bool utf16, IArchiveWriter* writer, uint32_t compression_flags) = 0; @@ -167,7 +168,7 @@ class ProtoApkSerializer : public IApkSerializer { std::unique_ptr data = file->file->OpenAsData(); if (!data) { context_->GetDiagnostics()->Error(DiagMessage(source_) - << "failed to open file " << *file->path); + << "failed to open file " << *file->path); return false; } @@ -175,7 +176,7 @@ class ProtoApkSerializer : public IApkSerializer { std::unique_ptr xml = xml::Inflate(data->data(), data->size(), &error); if (xml == nullptr) { context_->GetDiagnostics()->Error(DiagMessage(source_) << "failed to parse binary XML: " - << error); + << error); return false; } @@ -256,9 +257,6 @@ class Context : public IAaptContext { int Convert(IAaptContext* context, LoadedApk* apk, IArchiveWriter* output_writer, ApkFormat output_format, TableFlattenerOptions table_flattener_options, XmlFlattenerOptions xml_flattener_options) { - // Do not change the ordering of strings in the values string pool - table_flattener_options.sort_stringpool_entries = false; - unique_ptr serializer; if (output_format == ApkFormat::kBinary) { serializer.reset(new BinaryApkSerializer(context, apk->GetSource(), table_flattener_options, @@ -274,7 +272,7 @@ int Convert(IAaptContext* context, LoadedApk* apk, IArchiveWriter* output_writer io::IFile* manifest = apk->GetFileCollection()->FindFile(kAndroidManifestPath); if (!serializer->SerializeXml(apk->GetManifest(), kAndroidManifestPath, true /*utf16*/, output_writer, (manifest != nullptr && manifest->WasCompressed()) - ? ArchiveEntry::kCompress : 0u)) { + ? ArchiveEntry::kCompress : 0u)) { context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) << "failed to serialize AndroidManifest.xml"); return 1; @@ -303,8 +301,7 @@ int Convert(IAaptContext* context, LoadedApk* apk, IArchiveWriter* output_writer if (files_written.insert(*file->path).second) { if (!serializer->SerializeFile(file, output_writer)) { context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) - << "failed to serialize file " - << *file->path); + << "failed to serialize file " << *file->path); return 1; } } @@ -338,7 +335,7 @@ int Convert(IAaptContext* context, LoadedApk* apk, IArchiveWriter* output_writer if (!io::CopyFileToArchivePreserveCompression(context, file, path, output_writer)) { context->GetDiagnostics()->Error(DiagMessage(apk->GetSource()) - << "failed to copy file " << path); + << "failed to copy file " << path); return 1; } } diff --git a/tools/aapt2/format/binary/TableFlattener.cpp b/tools/aapt2/format/binary/TableFlattener.cpp index 9d341cc1ca4a..c8bb4655265d 100644 --- a/tools/aapt2/format/binary/TableFlattener.cpp +++ b/tools/aapt2/format/binary/TableFlattener.cpp @@ -702,17 +702,15 @@ class PackageFlattener { } // namespace bool TableFlattener::Consume(IAaptContext* context, ResourceTable* table) { - if (options_.sort_stringpool_entries) { - // We must do this before writing the resources, since the string pool IDs may change. - table->string_pool.Prune(); - table->string_pool.Sort([](const StringPool::Context &a, const StringPool::Context &b) -> int { - int diff = util::compare(a.priority, b.priority); - if (diff == 0) { - diff = a.config.compare(b.config); - } - return diff; - }); - } + // We must do this before writing the resources, since the string pool IDs may change. + table->string_pool.Prune(); + table->string_pool.Sort([](const StringPool::Context& a, const StringPool::Context& b) -> int { + int diff = util::compare(a.priority, b.priority); + if (diff == 0) { + diff = a.config.compare(b.config); + } + return diff; + }); // Write the ResTable header. ChunkWriter table_writer(buffer_); diff --git a/tools/aapt2/format/binary/TableFlattener.h b/tools/aapt2/format/binary/TableFlattener.h index 71330e3fb74f..73c17295556b 100644 --- a/tools/aapt2/format/binary/TableFlattener.h +++ b/tools/aapt2/format/binary/TableFlattener.h @@ -44,9 +44,6 @@ struct TableFlattenerOptions { // Set of whitelisted resource names to avoid altering in key stringpool std::set whitelisted_resources; - // When true, sort the entries in the values string pool by priority and configuration. - bool sort_stringpool_entries = true; - // Map from original resource paths to shortened resource paths. std::map shortened_path_map; }; -- cgit v1.2.3-59-g8ed1b From 0c0cedff00534dd9b14306b7c1cdc20ee20efb5e Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Mon, 15 Apr 2019 16:47:58 -0700 Subject: Check value in dump before printing For applications that remove the names of resources from the string pool, check that the attribute has a name before attempting to print it. Test: manual Bug: 130553900 Change-Id: I05e5d59f01b2c02c8a024d06fd896074d6bf465b --- tools/aapt2/Debug.cpp | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) (limited to 'tools/aapt2/Debug.cpp') diff --git a/tools/aapt2/Debug.cpp b/tools/aapt2/Debug.cpp index 98324850f3f5..3da22b4fb9fa 100644 --- a/tools/aapt2/Debug.cpp +++ b/tools/aapt2/Debug.cpp @@ -129,12 +129,20 @@ class ValueBodyPrinter : public ConstValueVisitor { constexpr uint32_t kMask = android::ResTable_map::TYPE_ENUM | android::ResTable_map::TYPE_FLAGS; if (attr->type_mask & kMask) { for (const auto& symbol : attr->symbols) { - printer_->Print(symbol.symbol.name.value().entry); - if (symbol.symbol.id) { - printer_->Print("("); + if (symbol.symbol.name) { + printer_->Print(symbol.symbol.name.value().entry); + + if (symbol.symbol.id) { + printer_->Print("("); + printer_->Print(symbol.symbol.id.value().to_string()); + printer_->Print(")"); + } + } else if (symbol.symbol.id) { printer_->Print(symbol.symbol.id.value().to_string()); - printer_->Print(")"); + } else { + printer_->Print("???"); } + printer_->Println(StringPrintf("=0x%08x", symbol.value)); } } -- cgit v1.2.3-59-g8ed1b