From 7c7573040ade65f70560bfdb508cdb29d41b1254 Mon Sep 17 00:00:00 2001 From: Mohamed Heikal Date: Thu, 25 Apr 2019 17:39:43 -0400 Subject: Fix bug where path shortening breaks ColorStateLists Android resource loader uses the file path to check if a resource is a ColorStateList. Path shortening removed that part of the path and thus broke the resource loader of APKs optimized with path shortening. This cl skips path shortening for resources under "res/color/" Test: make aapt2_tests Bug: b/75965637 Change-Id: If94dfa398efd81522d4faed157afd35f6dabe856 --- tools/aapt2/optimize/ResourcePathShortener.cpp | 4 ++ .../aapt2/optimize/ResourcePathShortener_test.cpp | 46 ++++++++++++++++++++++ 2 files changed, 50 insertions(+) (limited to 'tools') diff --git a/tools/aapt2/optimize/ResourcePathShortener.cpp b/tools/aapt2/optimize/ResourcePathShortener.cpp index c5df3dd00db9..845262bcb0d9 100644 --- a/tools/aapt2/optimize/ResourcePathShortener.cpp +++ b/tools/aapt2/optimize/ResourcePathShortener.cpp @@ -95,6 +95,10 @@ bool ResourcePathShortener::Consume(IAaptContext* context, ResourceTable* table) android::StringPiece res_subdir, actual_filename, extension; util::ExtractResFilePathParts(*file_ref->path, &res_subdir, &actual_filename, &extension); + // Android detects ColorStateLists via pathname, skip res/color/* + if (res_subdir == android::StringPiece("res/color/")) + continue; + std::string shortened_filename = ShortenFileName(*file_ref->path, num_chars); int collision_count = 0; std::string shortened_path = GetShortenedPath(shortened_filename, extension, collision_count); diff --git a/tools/aapt2/optimize/ResourcePathShortener_test.cpp b/tools/aapt2/optimize/ResourcePathShortener_test.cpp index 88cadc76c336..efbef8f40722 100644 --- a/tools/aapt2/optimize/ResourcePathShortener_test.cpp +++ b/tools/aapt2/optimize/ResourcePathShortener_test.cpp @@ -24,6 +24,11 @@ using ::testing::Not; using ::testing::NotNull; using ::testing::Eq; +android::StringPiece GetExtension(android::StringPiece path) { + auto iter = std::find(path.begin(), path.end(), '.'); + return android::StringPiece(iter, path.end() - iter); +} + namespace aapt { TEST(ResourcePathShortenerTest, FileRefPathsChangedInResourceTable) { @@ -64,4 +69,45 @@ TEST(ResourcePathShortenerTest, FileRefPathsChangedInResourceTable) { EXPECT_THAT(path_map.find("res/should/still/be/the/same.png"), Eq(path_map.end())); } +TEST(ResourcePathShortenerTest, SkipColorFileRefPaths) { + std::unique_ptr context = test::ContextBuilder().Build(); + + std::unique_ptr table = + test::ResourceTableBuilder() + .AddFileReference("android:color/colorlist", "res/color/colorlist.xml") + .Build(); + + std::map path_map; + ASSERT_TRUE(ResourcePathShortener(path_map).Consume(context.get(), table.get())); + + // Expect that the path map to not contain the ColorStateList + ASSERT_THAT(path_map.find("res/color/colorlist.xml"), Eq(path_map.end())); +} + +TEST(ResourcePathShortenerTest, KeepExtensions) { + std::unique_ptr context = test::ContextBuilder().Build(); + + std::string original_xml_path = "res/drawable/xmlfile.xml"; + std::string original_png_path = "res/drawable/pngfile.png"; + + std::unique_ptr table = + test::ResourceTableBuilder() + .AddFileReference("android:color/xmlfile", original_xml_path) + .AddFileReference("android:color/pngfile", original_png_path) + .Build(); + + std::map path_map; + ASSERT_TRUE(ResourcePathShortener(path_map).Consume(context.get(), table.get())); + + // Expect that the path map is populated + ASSERT_THAT(path_map.find("res/drawable/xmlfile.xml"), Not(Eq(path_map.end()))); + ASSERT_THAT(path_map.find("res/drawable/pngfile.png"), Not(Eq(path_map.end()))); + + auto shortend_xml_path = path_map[original_xml_path]; + auto shortend_png_path = path_map[original_png_path]; + + EXPECT_THAT(GetExtension(path_map[original_xml_path]), Eq(android::StringPiece(".xml"))); + EXPECT_THAT(GetExtension(path_map[original_png_path]), Eq(android::StringPiece(".png"))); +} + } // namespace aapt -- cgit v1.2.3 From 5e7370d1d02ef452ff685e669702c0caa8a49051 Mon Sep 17 00:00:00 2001 From: Winson Date: Tue, 30 Apr 2019 13:55:27 -0700 Subject: AAPT2: Fix --output-to-dir Without a FinishEntry call, the file_ wasn't being released, which prevents another StartEntry from succeeding. Bug: 68033366 Test: manual aapt2 link --output-to-dir -o outputDir Test: Archive_test.cpp Change-Id: I3b66266327163307a2e4c7f891f6ee76baf7fc10 --- tools/aapt2/format/Archive.cpp | 9 +- tools/aapt2/format/Archive_test.cpp | 209 ++++++++++++++++++++++++++++++++++++ 2 files changed, 217 insertions(+), 1 deletion(-) create mode 100644 tools/aapt2/format/Archive_test.cpp (limited to 'tools') diff --git a/tools/aapt2/format/Archive.cpp b/tools/aapt2/format/Archive.cpp index d152a9cc7e62..41f01a01ed7c 100644 --- a/tools/aapt2/format/Archive.cpp +++ b/tools/aapt2/format/Archive.cpp @@ -103,7 +103,13 @@ class DirectoryWriter : public IArchiveWriter { return false; } } - return !in->HadError(); + + if (in->HadError()) { + error_ = in->GetError(); + return false; + } + + return FinishEntry(); } bool HadError() const override { @@ -191,6 +197,7 @@ class ZipFileWriter : public IArchiveWriter { } if (in->HadError()) { + error_ = in->GetError(); return false; } diff --git a/tools/aapt2/format/Archive_test.cpp b/tools/aapt2/format/Archive_test.cpp new file mode 100644 index 000000000000..ceed3740f37a --- /dev/null +++ b/tools/aapt2/format/Archive_test.cpp @@ -0,0 +1,209 @@ +/* + * Copyright (C) 2017 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "test/Test.h" + +namespace aapt { + +using ArchiveTest = TestDirectoryFixture; + +constexpr size_t kTestDataLength = 100; + +class TestData : public io::MallocData { + public: + TestData(std::unique_ptr& data, size_t size) + : MallocData(std::move(data), size) {} + + bool HadError() const override { return !error_.empty(); } + + std::string GetError() const override { return error_; } + + std::string error_; +}; + +std::unique_ptr MakeTestArray() { + auto array = std::make_unique(kTestDataLength); + for (int index = 0; index < kTestDataLength; ++index) { + array[index] = static_cast(rand()); + } + return array; +} + +std::unique_ptr MakeDirectoryWriter(const std::string& output_path) { + file::mkdirs(output_path); + + StdErrDiagnostics diag; + return CreateDirectoryArchiveWriter(&diag, output_path); +} + +std::unique_ptr MakeZipFileWriter(const std::string& output_path) { + file::mkdirs(file::GetStem(output_path).to_string()); + std::remove(output_path.c_str()); + + StdErrDiagnostics diag; + return CreateZipFileArchiveWriter(&diag, output_path); +} + +void VerifyDirectory(const std::string& path, const std::string& file, const uint8_t array[]) { + std::string file_path = file::BuildPath({path, file}); + auto buffer = std::make_unique(kTestDataLength); + std::ifstream stream(file_path); + stream.read(buffer.get(), kTestDataLength); + + for (int index = 0; index < kTestDataLength; ++index) { + ASSERT_EQ(array[index], static_cast(buffer[index])); + } +} + +void VerifyZipFile(const std::string& output_path, const std::string& file, const uint8_t array[]) { + std::unique_ptr zip = io::ZipFileCollection::Create(output_path, nullptr); + std::unique_ptr stream = zip->FindFile(file)->OpenInputStream(); + + std::vector buffer; + const void* data; + size_t size; + + while (stream->Next(&data, &size)) { + auto pointer = static_cast(data); + buffer.insert(buffer.end(), pointer, pointer + size); + } + + for (int index = 0; index < kTestDataLength; ++index) { + ASSERT_EQ(array[index], buffer[index]); + } +} + +TEST_F(ArchiveTest, DirectoryWriteEntrySuccess) { + std::string output_path = GetTestPath("output"); + std::unique_ptr writer = MakeDirectoryWriter(output_path); + std::unique_ptr data1 = MakeTestArray(); + std::unique_ptr data2 = MakeTestArray(); + + ASSERT_TRUE(writer->StartEntry("test1", 0)); + ASSERT_TRUE(writer->Write(static_cast(data1.get()), kTestDataLength)); + ASSERT_TRUE(writer->FinishEntry()); + ASSERT_FALSE(writer->HadError()); + + ASSERT_TRUE(writer->StartEntry("test2", 0)); + ASSERT_TRUE(writer->Write(static_cast(data2.get()), kTestDataLength)); + ASSERT_TRUE(writer->FinishEntry()); + ASSERT_FALSE(writer->HadError()); + + writer.reset(); + + VerifyDirectory(output_path, "test1", data1.get()); + VerifyDirectory(output_path, "test2", data2.get()); +} + +TEST_F(ArchiveTest, DirectoryWriteFileSuccess) { + std::string output_path = GetTestPath("output"); + std::unique_ptr writer = MakeDirectoryWriter(output_path); + + std::unique_ptr data1 = MakeTestArray(); + auto data1_copy = std::make_unique(kTestDataLength); + std::copy(data1.get(), data1.get() + kTestDataLength, data1_copy.get()); + + std::unique_ptr data2 = MakeTestArray(); + auto data2_copy = std::make_unique(kTestDataLength); + std::copy(data2.get(), data2.get() + kTestDataLength, data2_copy.get()); + + auto input1 = std::make_unique(data1_copy, kTestDataLength); + auto input2 = std::make_unique(data2_copy, kTestDataLength); + + ASSERT_TRUE(writer->WriteFile("test1", 0, input1.get())); + ASSERT_FALSE(writer->HadError()); + ASSERT_TRUE(writer->WriteFile("test2", 0, input2.get())); + ASSERT_FALSE(writer->HadError()); + + writer.reset(); + + VerifyDirectory(output_path, "test1", data1.get()); + VerifyDirectory(output_path, "test2", data2.get()); +} + +TEST_F(ArchiveTest, DirectoryWriteFileError) { + std::string output_path = GetTestPath("output"); + std::unique_ptr writer = MakeDirectoryWriter(output_path); + std::unique_ptr data = MakeTestArray(); + auto input = std::make_unique(data, kTestDataLength); + input->error_ = "DirectoryWriteFileError"; + + ASSERT_FALSE(writer->WriteFile("test", 0, input.get())); + ASSERT_TRUE(writer->HadError()); + ASSERT_EQ("DirectoryWriteFileError", writer->GetError()); +} + +TEST_F(ArchiveTest, ZipFileWriteEntrySuccess) { + std::string output_path = GetTestPath("output.apk"); + std::unique_ptr writer = MakeZipFileWriter(output_path); + std::unique_ptr data1 = MakeTestArray(); + std::unique_ptr data2 = MakeTestArray(); + + ASSERT_TRUE(writer->StartEntry("test1", 0)); + ASSERT_TRUE(writer->Write(static_cast(data1.get()), kTestDataLength)); + ASSERT_TRUE(writer->FinishEntry()); + ASSERT_FALSE(writer->HadError()); + + ASSERT_TRUE(writer->StartEntry("test2", 0)); + ASSERT_TRUE(writer->Write(static_cast(data2.get()), kTestDataLength)); + ASSERT_TRUE(writer->FinishEntry()); + ASSERT_FALSE(writer->HadError()); + + writer.reset(); + + VerifyZipFile(output_path, "test1", data1.get()); + VerifyZipFile(output_path, "test2", data2.get()); +} + +TEST_F(ArchiveTest, ZipFileWriteFileSuccess) { + std::string output_path = GetTestPath("output.apk"); + std::unique_ptr writer = MakeZipFileWriter(output_path); + + std::unique_ptr data1 = MakeTestArray(); + auto data1_copy = std::make_unique(kTestDataLength); + std::copy(data1.get(), data1.get() + kTestDataLength, data1_copy.get()); + + std::unique_ptr data2 = MakeTestArray(); + auto data2_copy = std::make_unique(kTestDataLength); + std::copy(data2.get(), data2.get() + kTestDataLength, data2_copy.get()); + + auto input1 = std::make_unique(data1_copy, kTestDataLength); + auto input2 = std::make_unique(data2_copy, kTestDataLength); + + ASSERT_TRUE(writer->WriteFile("test1", 0, input1.get())); + ASSERT_FALSE(writer->HadError()); + ASSERT_TRUE(writer->WriteFile("test2", 0, input2.get())); + ASSERT_FALSE(writer->HadError()); + + writer.reset(); + + VerifyZipFile(output_path, "test1", data1.get()); + VerifyZipFile(output_path, "test2", data2.get()); +} + +TEST_F(ArchiveTest, ZipFileWriteFileError) { + std::string output_path = GetTestPath("output.apk"); + std::unique_ptr writer = MakeZipFileWriter(output_path); + std::unique_ptr data = MakeTestArray(); + auto input = std::make_unique(data, kTestDataLength); + input->error_ = "ZipFileWriteFileError"; + + ASSERT_FALSE(writer->WriteFile("test", 0, input.get())); + ASSERT_TRUE(writer->HadError()); + ASSERT_EQ("ZipFileWriteFileError", writer->GetError()); +} + +} // namespace aapt -- cgit v1.2.3 From 9477d560ac56018c47133105760c1ddd7be63a2b Mon Sep 17 00:00:00 2001 From: Ian Pedowitz Date: Thu, 2 May 2019 00:55:51 +0000 Subject: Revert "RESTRICT AUTOMERGE Android Q is API 29" This reverts commit 8a3d1f96e1736c7e0493ebb71ed2904dffdebbfd. Reason for revert: QT SDK Finalization. Will be merged again on/after May 13th Bug: 131429032 Bug: 129975435 Change-Id: I7a48ef6a057a97ebd9903b7e934a7d95ec97f00e (cherry picked from commit 60c71cee6ec064a0d2f4c6608fdb54bf95545d45) --- tools/aapt/SdkConstants.h | 1 - tools/aapt2/SdkConstants.h | 1 - 2 files changed, 2 deletions(-) (limited to 'tools') diff --git a/tools/aapt/SdkConstants.h b/tools/aapt/SdkConstants.h index 27ffcdf52168..c1fcf5cad240 100644 --- a/tools/aapt/SdkConstants.h +++ b/tools/aapt/SdkConstants.h @@ -44,7 +44,6 @@ enum { SDK_O = 26, SDK_O_MR1 = 27, SDK_P = 28, - SDK_Q = 29, }; #endif // H_AAPT_SDK_CONSTANTS diff --git a/tools/aapt2/SdkConstants.h b/tools/aapt2/SdkConstants.h index a00d978565ad..adb034a95328 100644 --- a/tools/aapt2/SdkConstants.h +++ b/tools/aapt2/SdkConstants.h @@ -54,7 +54,6 @@ enum : ApiVersion { SDK_O = 26, SDK_O_MR1 = 27, SDK_P = 28, - SDK_Q = 29, }; ApiVersion FindAttributeSdkLevel(const ResourceId& id); -- cgit v1.2.3 From 42fe2b2ac7debe61bfa8b63ef3bb41b774f12cdf Mon Sep 17 00:00:00 2001 From: Donald Chai Date: Tue, 7 May 2019 20:03:27 -0700 Subject: [aapt2] Close empty lists in "dump resources". Bug: 129134933 Test: manual (b/129134933#comment4) since there's no Debug_test.cpp Change-Id: Ib31481457089eade6f1d380cff4eeba0ff0c499d --- tools/aapt2/Debug.cpp | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) (limited to 'tools') diff --git a/tools/aapt2/Debug.cpp b/tools/aapt2/Debug.cpp index 98324850f3f5..fbe147206d9c 100644 --- a/tools/aapt2/Debug.cpp +++ b/tools/aapt2/Debug.cpp @@ -170,19 +170,17 @@ class ValueBodyPrinter : public ConstValueVisitor { void Visit(const Array* array) override { const size_t count = array->elements.size(); printer_->Print("["); - if (count > 0) { - for (size_t i = 0u; i < count; i++) { - if (i != 0u && i % 4u == 0u) { - printer_->Println(); - printer_->Print(" "); - } - PrintItem(*array->elements[i]); - if (i != count - 1) { - printer_->Print(", "); - } + for (size_t i = 0u; i < count; i++) { + if (i != 0u && i % 4u == 0u) { + printer_->Println(); + printer_->Print(" "); + } + PrintItem(*array->elements[i]); + if (i != count - 1) { + printer_->Print(", "); } - printer_->Println("]"); } + printer_->Println("]"); } void Visit(const Plural* plural) override { -- cgit v1.2.3 From 2ee7cc6ee9ad8343962379573d4dbf928cb1c9a9 Mon Sep 17 00:00:00 2001 From: Donald Chai Date: Sat, 11 May 2019 02:37:23 -0700 Subject: [aapt2] Set use_version_lib. This is needed by the build system to update soong_build_number. On a local build, "aapt2 version" used to print: Android Asset Packaging Tool (aapt) 2.19-SOONG BUILD NUMBER PLACEHOLDER Now it prints: Android Asset Packaging Tool (aapt) 2.19-eng.dchai.20190511.023434 Bug: 132476779 Bug: 123663089 Change-Id: I5bc5adc9f5b2a7a30bdc649e3c073868b55f17df Tested: manual, see above --- tools/aapt2/Android.bp | 1 + 1 file changed, 1 insertion(+) (limited to 'tools') diff --git a/tools/aapt2/Android.bp b/tools/aapt2/Android.bp index 53372bff3e67..b13731f75629 100644 --- a/tools/aapt2/Android.bp +++ b/tools/aapt2/Android.bp @@ -197,6 +197,7 @@ cc_test_host { cc_binary_host { name: "aapt2", srcs: ["Main.cpp"] + toolSources, + use_version_lib: true, static_libs: ["libaapt2"], defaults: ["aapt2_defaults"], } -- cgit v1.2.3 From 22ead1c3b02e6f4995915ad02d6612fa9468ba63 Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Mon, 20 May 2019 16:54:48 -0700 Subject: Remove lite optimization from protos aapt2 does not require the lite runtime optimization to reduce binary size. Removing the allows for the protos to be optimized for speed instead of binary size and grants access to refelctive operations. Test: aapt2_tests on linux and windows Change-Id: Ie7ed9df4061e2eb60adbd300eb358d4c3f32c80c --- tools/aapt2/Android.bp | 2 +- tools/aapt2/Configuration.proto | 1 - tools/aapt2/Resources.proto | 1 - tools/aapt2/ResourcesInternal.proto | 1 - tools/aapt2/io/Util.cpp | 2 +- tools/aapt2/io/Util.h | 10 +++++----- 6 files changed, 7 insertions(+), 10 deletions(-) (limited to 'tools') diff --git a/tools/aapt2/Android.bp b/tools/aapt2/Android.bp index b13731f75629..1be4ea8eccda 100644 --- a/tools/aapt2/Android.bp +++ b/tools/aapt2/Android.bp @@ -56,7 +56,7 @@ cc_defaults { "libziparchive", "libpng", "libbase", - "libprotobuf-cpp-lite", + "libprotobuf-cpp-full", "libz", "libbuildversion", ], diff --git a/tools/aapt2/Configuration.proto b/tools/aapt2/Configuration.proto index fc636a43ec40..8a4644c9a219 100644 --- a/tools/aapt2/Configuration.proto +++ b/tools/aapt2/Configuration.proto @@ -19,7 +19,6 @@ syntax = "proto3"; package aapt.pb; option java_package = "com.android.aapt"; -option optimize_for = LITE_RUNTIME; // A description of the requirements a device must have in order for a // resource to be matched and selected. diff --git a/tools/aapt2/Resources.proto b/tools/aapt2/Resources.proto index b2fc08423d34..da0dc790e682 100644 --- a/tools/aapt2/Resources.proto +++ b/tools/aapt2/Resources.proto @@ -21,7 +21,6 @@ import "frameworks/base/tools/aapt2/Configuration.proto"; package aapt.pb; option java_package = "com.android.aapt"; -option optimize_for = LITE_RUNTIME; // A string pool that wraps the binary form of the C++ class android::ResStringPool. message StringPool { diff --git a/tools/aapt2/ResourcesInternal.proto b/tools/aapt2/ResourcesInternal.proto index 520b242ee509..b0ed3da33368 100644 --- a/tools/aapt2/ResourcesInternal.proto +++ b/tools/aapt2/ResourcesInternal.proto @@ -22,7 +22,6 @@ import "frameworks/base/tools/aapt2/Resources.proto"; package aapt.pb.internal; option java_package = "android.aapt.pb.internal"; -option optimize_for = LITE_RUNTIME; // The top level message representing an external resource file (layout XML, PNG, etc). // This is used to represent a compiled file before it is linked. Only useful to aapt2. diff --git a/tools/aapt2/io/Util.cpp b/tools/aapt2/io/Util.cpp index ce6d9352180d..bb925c9b3f8e 100644 --- a/tools/aapt2/io/Util.cpp +++ b/tools/aapt2/io/Util.cpp @@ -58,7 +58,7 @@ bool CopyFileToArchivePreserveCompression(IAaptContext* context, io::IFile* file return CopyFileToArchive(context, file, out_path, compression_flags, writer); } -bool CopyProtoToArchive(IAaptContext* context, ::google::protobuf::MessageLite* proto_msg, +bool CopyProtoToArchive(IAaptContext* context, ::google::protobuf::Message* proto_msg, const std::string& out_path, uint32_t compression_flags, IArchiveWriter* writer) { TRACE_CALL(); diff --git a/tools/aapt2/io/Util.h b/tools/aapt2/io/Util.h index 5f978a8e2c35..5cb8206db23c 100644 --- a/tools/aapt2/io/Util.h +++ b/tools/aapt2/io/Util.h @@ -19,7 +19,7 @@ #include -#include "google/protobuf/message_lite.h" +#include "google/protobuf/message.h" #include "google/protobuf/io/coded_stream.h" #include "format/Archive.h" @@ -39,7 +39,7 @@ bool CopyFileToArchive(IAaptContext* context, IFile* file, const std::string& ou bool CopyFileToArchivePreserveCompression(IAaptContext* context, IFile* file, const std::string& out_path, IArchiveWriter* writer); -bool CopyProtoToArchive(IAaptContext* context, ::google::protobuf::MessageLite* proto_msg, +bool CopyProtoToArchive(IAaptContext* context, ::google::protobuf::Message* proto_msg, const std::string& out_path, uint32_t compression_flags, IArchiveWriter* writer); @@ -127,13 +127,13 @@ class ProtoInputStreamReader { public: explicit ProtoInputStreamReader(io::InputStream* in) : in_(in) { } - /** Deserializes a MessageLite proto from the current position in the input stream.*/ - template bool ReadMessage(T *message_lite) { + /** Deserializes a Message proto from the current position in the input stream.*/ + template bool ReadMessage(T *message) { ZeroCopyInputAdaptor adapter(in_); google::protobuf::io::CodedInputStream coded_stream(&adapter); coded_stream.SetTotalBytesLimit(std::numeric_limits::max(), coded_stream.BytesUntilTotalBytesLimit()); - return message_lite->ParseFromCodedStream(&coded_stream); + return message->ParseFromCodedStream(&coded_stream); } private: -- cgit v1.2.3 From fc3874ac821b63b07fef089667e4afe3c4528aa5 Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Tue, 28 May 2019 16:30:17 -0700 Subject: Empty attribute values in styles should be strings In aapt1, if you specified the value of an attribute in a style as an empty string (eg. ), the encoded value would be an empty string. In aapt2 currently, @null is encoded instead. This change restores aapt1 behavior. Use @null explicitly if the desired value is to be @null. Bug: 133450400 Test: manual comparison of APK created by aapt1 and aapt2 $ aapt p -M AndroidManifest.xml -F out1.apk -S res -f $ aapt2 compile --dir res-o compiled.flata $ aapt2 link --manifest AndroidManifest.xml -o out2.apk compiled.flata $ aapt2 dump out2.apk Change-Id: I8aa0ba30379dac0b1229da525abbc5482f40114b --- tools/aapt2/ResourceParser.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) (limited to 'tools') diff --git a/tools/aapt2/ResourceParser.cpp b/tools/aapt2/ResourceParser.cpp index 45cea8190844..859fe80c5ce7 100644 --- a/tools/aapt2/ResourceParser.cpp +++ b/tools/aapt2/ResourceParser.cpp @@ -769,16 +769,14 @@ std::unique_ptr ResourceParser::ParseXml(xml::XmlPullParser* parser, return std::move(string); } - // If the text is empty, and the value is not allowed to be a string, encode it as a @null. - if (util::TrimWhitespace(raw_value).empty()) { - return ResourceUtils::MakeNull(); - } - if (allow_raw_value) { // We can't parse this so return a RawString if we are allowed. return util::make_unique( table_->string_pool.MakeRef(util::TrimWhitespace(raw_value), StringPool::Context(config_))); + } else if (util::TrimWhitespace(raw_value).empty()) { + // If the text is empty, and the value is not allowed to be a string, encode it as a @null. + return ResourceUtils::MakeNull(); } return {}; } -- cgit v1.2.3 From 4c3da0f8a4e3db7ae6a0dff3888d7faab3198bb1 Mon Sep 17 00:00:00 2001 From: Donald Chai Date: Fri, 31 May 2019 23:52:21 -0700 Subject: [aapt2] Pseudolocalize values. (.won slaer roF) Bug: 134190774 Test: aapt2_tests Change-Id: If307af4adfb1d556c41d7cb08590d1c25287f992 --- tools/aapt2/compile/PseudolocaleGenerator.cpp | 2 +- tools/aapt2/compile/PseudolocaleGenerator_test.cpp | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) (limited to 'tools') diff --git a/tools/aapt2/compile/PseudolocaleGenerator.cpp b/tools/aapt2/compile/PseudolocaleGenerator.cpp index c5de9e058907..5e0300b3071b 100644 --- a/tools/aapt2/compile/PseudolocaleGenerator.cpp +++ b/tools/aapt2/compile/PseudolocaleGenerator.cpp @@ -231,7 +231,7 @@ class Visitor : public ValueVisitor { Visitor sub_visitor(pool_, method_); if (plural->values[i]) { plural->values[i]->Accept(&sub_visitor); - if (sub_visitor.value) { + if (sub_visitor.item) { localized->values[i] = std::move(sub_visitor.item); } else { localized->values[i] = std::unique_ptr(plural->values[i]->Clone(pool_)); diff --git a/tools/aapt2/compile/PseudolocaleGenerator_test.cpp b/tools/aapt2/compile/PseudolocaleGenerator_test.cpp index 31358020ab60..e816c86e20a8 100644 --- a/tools/aapt2/compile/PseudolocaleGenerator_test.cpp +++ b/tools/aapt2/compile/PseudolocaleGenerator_test.cpp @@ -234,6 +234,27 @@ TEST(PseudolocaleGeneratorTest, PseudolocalizeOnlyDefaultConfigs) { test::ParseConfigOrDie("ar-rXB"))); } +TEST(PseudolocaleGeneratorTest, PluralsArePseudolocalized) { + std::unique_ptr context = test::ContextBuilder().Build(); + std::unique_ptr table = + test::ResourceTableBuilder().SetPackageId("com.pkg", 0x7F).Build(); + std::unique_ptr plural = util::make_unique(); + plural->values = {util::make_unique(table->string_pool.MakeRef("zero")), + util::make_unique(table->string_pool.MakeRef("one"))}; + ASSERT_TRUE(table->AddResource(test::ParseNameOrDie("com.pkg:plurals/foo"), ConfigDescription{}, + {}, std::move(plural), context->GetDiagnostics())); + std::unique_ptr expected = util::make_unique(); + expected->values = {util::make_unique(table->string_pool.MakeRef("[žéŕö one]")), + util::make_unique(table->string_pool.MakeRef("[öñé one]"))}; + + PseudolocaleGenerator generator; + ASSERT_TRUE(generator.Consume(context.get(), table.get())); + + const auto* actual = test::GetValueForConfig(table.get(), "com.pkg:plurals/foo", + test::ParseConfigOrDie("en-rXA")); + EXPECT_TRUE(actual->Equals(expected.get())); +} + TEST(PseudolocaleGeneratorTest, RespectUntranslateableSections) { std::unique_ptr context = test::ContextBuilder().SetCompilationPackage("android").Build(); -- cgit v1.2.3 From 44fa342eb91b90df7998fa2808e21af75aafaf39 Mon Sep 17 00:00:00 2001 From: Donald Chai Date: Fri, 31 May 2019 22:13:00 -0700 Subject: [aapt2] Fix infinite loop in proguard::CollectLocations std::set only works correctly when the < comparator is a strict weak ordering, while UsageLocation::operator< was actually implementing !=. Bug: 134190468 Change-Id: Icb9407e9c8451f9fcb4eb9b2cea310e3bcaf159e Tested: aapt2_tests, and b/134190468#comment1 --- tools/aapt2/java/ProguardRules.h | 6 ++++-- tools/aapt2/java/ProguardRules_test.cpp | 8 ++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) (limited to 'tools') diff --git a/tools/aapt2/java/ProguardRules.h b/tools/aapt2/java/ProguardRules.h index f9656d112b7b..b15df59f56a6 100644 --- a/tools/aapt2/java/ProguardRules.h +++ b/tools/aapt2/java/ProguardRules.h @@ -99,11 +99,13 @@ bool CollectLocations(const UsageLocation& location, const KeepSet& keep_set, // inline bool operator==(const UsageLocation& lhs, const UsageLocation& rhs) { + // The "source" member is ignored because we only need "name" for outputting + // keep rules; "source" is used for comments. return lhs.name == rhs.name; } -inline int operator<(const UsageLocation& lhs, const UsageLocation& rhs) { - return lhs.name.compare(rhs.name); +inline bool operator<(const UsageLocation& lhs, const UsageLocation& rhs) { + return lhs.name.compare(rhs.name) < 0; } // diff --git a/tools/aapt2/java/ProguardRules_test.cpp b/tools/aapt2/java/ProguardRules_test.cpp index 559b07af3e80..25b55ab003b0 100644 --- a/tools/aapt2/java/ProguardRules_test.cpp +++ b/tools/aapt2/java/ProguardRules_test.cpp @@ -364,4 +364,12 @@ TEST(ProguardRulesTest, TransitionRulesAreEmitted) { "-keep class com.foo.Bar { (android.content.Context, android.util.AttributeSet); }")); } +TEST(ProguardRulesTest, UsageLocationComparator) { + proguard::UsageLocation location1 = {{"pkg", ResourceType::kAttr, "x"}}; + proguard::UsageLocation location2 = {{"pkg", ResourceType::kAttr, "y"}}; + + EXPECT_EQ(location1 < location2, true); + EXPECT_EQ(location2 < location1, false); +} + } // namespace aapt -- cgit v1.2.3 From a36cc98af509943d32288293c2c238c138edaba7 Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Wed, 5 Jun 2019 10:13:41 -0700 Subject: Badging should print compiled platform version aapt2 saves space by compiling all raw xml strings to the Res_value structure. This causes the platformBuildVersionCode and platformBuildVersionCodename attributes on the tag to be compilled as integers. aapt2 and aapt dump badging assumed these values would be strings and is failing to print the value for these attributes. With this change, the value will be printed regardless if it is encoded as an integer or string. Bug: 130830770 Test: manualy build APK and run aapt2 dump badging Change-Id: I89487db36c6bc4d0fde3a410b7a64debfec5021e --- tools/aapt2/dump/DumpManifest.cpp | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) (limited to 'tools') diff --git a/tools/aapt2/dump/DumpManifest.cpp b/tools/aapt2/dump/DumpManifest.cpp index 92f1ddb292e1..54f0816f0398 100644 --- a/tools/aapt2/dump/DumpManifest.cpp +++ b/tools/aapt2/dump/DumpManifest.cpp @@ -291,7 +291,10 @@ class ManifestExtractor { } } } - return &attr->value; + + if (!attr->value.empty()) { + return &attr->value; + } } return nullptr; } @@ -425,6 +428,8 @@ class Manifest : public ManifestExtractor::Element { const std::string* split = nullptr; const std::string* platformVersionName = nullptr; const std::string* platformVersionCode = nullptr; + const int32_t* platformVersionNameInt = nullptr; + const int32_t* platformVersionCodeInt = nullptr; const int32_t* compilesdkVersion = nullptr; const std::string* compilesdkVersionCodename = nullptr; const int32_t* installLocation = nullptr; @@ -440,6 +445,10 @@ class Manifest : public ManifestExtractor::Element { "platformBuildVersionName")); platformVersionCode = GetAttributeString(FindAttribute(manifest, {}, "platformBuildVersionCode")); + platformVersionNameInt = GetAttributeInteger(FindAttribute(manifest, {}, + "platformBuildVersionName")); + platformVersionCodeInt = GetAttributeInteger(FindAttribute(manifest, {}, + "platformBuildVersionCode")); // Extract the compile sdk info compilesdkVersion = GetAttributeInteger(FindAttribute(manifest, COMPILE_SDK_VERSION_ATTR)); @@ -460,9 +469,15 @@ class Manifest : public ManifestExtractor::Element { if (platformVersionName) { printer->Print(StringPrintf(" platformBuildVersionName='%s'", platformVersionName->data())); } + if (platformVersionNameInt) { + printer->Print(StringPrintf(" platformBuildVersionName='%d'", *platformVersionNameInt)); + } if (platformVersionCode) { printer->Print(StringPrintf(" platformBuildVersionCode='%s'", platformVersionCode->data())); } + if (platformVersionCodeInt) { + printer->Print(StringPrintf(" platformBuildVersionCode='%d'", *platformVersionCodeInt)); + } if (compilesdkVersion) { printer->Print(StringPrintf(" compileSdkVersion='%d'", *compilesdkVersion)); } -- cgit v1.2.3 From 09a847902fa428f97841c3689b9f11243cc60460 Mon Sep 17 00:00:00 2001 From: Ryan Savitski Date: Mon, 3 Jun 2019 23:57:09 +0100 Subject: userdebug: support perfetto traces as a section in incident reports This set of patches adds a way for the perfetto command line client to save a trace to a hardcoded location, /data/misc/perfetto-traces/incident-trace, and call into incidentd to start a report, which will include said trace in a new section. This is not a long-term solution, and is structured to minimize changes to perfetto and incidentd. The latter is currently architected in a way where it can only pull pre-defined information out of the system, so we're resorting to persisting the intermediate results in a hardcoded location. This will introduce at most two more linked files at the same time. Bug: 130543265 Tested: manually on blueline-userdebug Change-Id: Iaaa312d2d9da73ca329807211227a8c7a049102c --- tools/incident_section_gen/main.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'tools') diff --git a/tools/incident_section_gen/main.cpp b/tools/incident_section_gen/main.cpp index c9c0edc59585..91f875ed9918 100644 --- a/tools/incident_section_gen/main.cpp +++ b/tools/incident_section_gen/main.cpp @@ -408,9 +408,10 @@ static bool generateSectionListCpp(Descriptor const* descriptor) { for (int i=0; ifield_count(); i++) { const FieldDescriptor* field = descriptor->field(i); - if (field->type() != FieldDescriptor::TYPE_MESSAGE - && field->type() != FieldDescriptor::TYPE_STRING) { - continue; + if (field->type() != FieldDescriptor::TYPE_MESSAGE && + field->type() != FieldDescriptor::TYPE_STRING && + field->type() != FieldDescriptor::TYPE_BYTES) { + continue; } const SectionFlags s = getSectionFlags(field); -- cgit v1.2.3 From 6e49735aa8787e872c0530110be2c24715e805f5 Mon Sep 17 00:00:00 2001 From: Donald Chai Date: Sun, 19 May 2019 21:07:50 -0700 Subject: [aapt2] Ensure that attributes have declared namespaces when fixing manifest aapt::ManifestFixer might add attributes from http://schemas.android.com/apk/res/android when none existed previously. When that happens, something needs to ensure that the schema has a declared xmlns prefix, otherwise this: https://github.com/bazelbuild/bazel/blob/bdb3e97c47188ed4e5b472a3b7393e9588c93011/src/tools/android/java/com/google/devtools/build/android/aapt2/ResourceLinker.java#L560 will crash. It's a bit odd that Bazel converts back and forth between protobuf and XML, but as long as the fields exist in Resources.proto, they should be set. Bug: 133004752 Change-Id: I73fb3f713be34c9f81753fde2fd24c4524656b7d Tested: aapt2_tests --- tools/aapt2/link/ManifestFixer.cpp | 29 +++++++++++++++++++++++++++++ tools/aapt2/link/ManifestFixer_test.cpp | 30 ++++++++++++++++++++++++++++-- 2 files changed, 57 insertions(+), 2 deletions(-) (limited to 'tools') diff --git a/tools/aapt2/link/ManifestFixer.cpp b/tools/aapt2/link/ManifestFixer.cpp index 49909f6e2b8e..11150dd8e882 100644 --- a/tools/aapt2/link/ManifestFixer.cpp +++ b/tools/aapt2/link/ManifestFixer.cpp @@ -214,6 +214,33 @@ static bool VerifyUsesFeature(xml::Element* el, SourcePathDiagnostics* diag) { return true; } +// Ensure that 'ns_decls' contains a declaration for 'uri', using 'prefix' as +// the xmlns prefix if possible. +static void EnsureNamespaceIsDeclared(const std::string& prefix, const std::string& uri, + std::vector* ns_decls) { + if (std::find_if(ns_decls->begin(), ns_decls->end(), [&](const xml::NamespaceDecl& ns_decl) { + return ns_decl.uri == uri; + }) != ns_decls->end()) { + return; + } + + std::set used_prefixes; + for (const auto& ns_decl : *ns_decls) { + used_prefixes.insert(ns_decl.prefix); + } + + // Make multiple attempts in the unlikely event that 'prefix' is already taken. + std::string disambiguator; + for (int i = 0; i < used_prefixes.size() + 1; i++) { + std::string attempted_prefix = prefix + disambiguator; + if (used_prefixes.find(attempted_prefix) == used_prefixes.end()) { + ns_decls->push_back(xml::NamespaceDecl{attempted_prefix, uri}); + return; + } + disambiguator = std::to_string(i); + } +} + bool ManifestFixer::BuildRules(xml::XmlActionExecutor* executor, IDiagnostics* diag) { // First verify some options. @@ -262,6 +289,8 @@ bool ManifestFixer::BuildRules(xml::XmlActionExecutor* executor, manifest_action.Action(VerifyManifest); manifest_action.Action(FixCoreAppAttribute); manifest_action.Action([&](xml::Element* el) -> bool { + EnsureNamespaceIsDeclared("android", xml::kSchemaAndroid, &el->namespace_decls); + if (options_.version_name_default) { if (options_.replace_version) { el->RemoveAttribute(xml::kSchemaAndroid, "versionName"); diff --git a/tools/aapt2/link/ManifestFixer_test.cpp b/tools/aapt2/link/ManifestFixer_test.cpp index 3f1ee36dea4a..3af06f53d4f3 100644 --- a/tools/aapt2/link/ManifestFixer_test.cpp +++ b/tools/aapt2/link/ManifestFixer_test.cpp @@ -727,8 +727,7 @@ TEST_F(ManifestFixerTest, SupportKeySets) { } TEST_F(ManifestFixerTest, InsertCompileSdkVersions) { - std::string input = R"( - )"; + std::string input = R"()"; ManifestFixerOptions options; options.compile_sdk_version = {"28"}; options.compile_sdk_version_codename = {"P"}; @@ -736,6 +735,12 @@ TEST_F(ManifestFixerTest, InsertCompileSdkVersions) { std::unique_ptr manifest = VerifyWithOptions(input, options); ASSERT_THAT(manifest, NotNull()); + // There should be a declaration of kSchemaAndroid, even when the input + // didn't have one. + EXPECT_EQ(manifest->root->namespace_decls.size(), 1); + EXPECT_EQ(manifest->root->namespace_decls[0].prefix, "android"); + EXPECT_EQ(manifest->root->namespace_decls[0].uri, xml::kSchemaAndroid); + xml::Attribute* attr = manifest->root->FindAttribute(xml::kSchemaAndroid, "compileSdkVersion"); ASSERT_THAT(attr, NotNull()); EXPECT_THAT(attr->value, StrEq("28")); @@ -782,6 +787,27 @@ TEST_F(ManifestFixerTest, OverrideCompileSdkVersions) { EXPECT_THAT(attr->value, StrEq("P")); } +TEST_F(ManifestFixerTest, AndroidPrefixAlreadyUsed) { + std::string input = + R"()"; + ManifestFixerOptions options; + options.compile_sdk_version = {"28"}; + options.compile_sdk_version_codename = {"P"}; + + std::unique_ptr manifest = VerifyWithOptions(input, options); + ASSERT_THAT(manifest, NotNull()); + + // Make sure that we don't redefine "android". + EXPECT_EQ(manifest->root->namespace_decls.size(), 2); + EXPECT_EQ(manifest->root->namespace_decls[0].prefix, "android"); + EXPECT_EQ(manifest->root->namespace_decls[0].uri, + "http://schemas.android.com/apk/prv/res/android"); + EXPECT_EQ(manifest->root->namespace_decls[1].prefix, "android0"); + EXPECT_EQ(manifest->root->namespace_decls[1].uri, xml::kSchemaAndroid); +} + TEST_F(ManifestFixerTest, UnexpectedElementsInManifest) { std::string input = R"( Date: Mon, 20 May 2019 16:47:08 -0700 Subject: Retain parsed attribute type If the value of an attribute enum is defined as a hexadecimal integer, flatten uses of the attribute as with the android::Res_value::TYPE_INT_HEX type. This change adds a "type" field to pb::Attribute::Symbol, which if left unset, will have a default value of android::Res_value::TYPE_INT_DEC when deserialized by aapt2. Bug: 124474141 Test: aapt2_tests and manual compilation of files and inspection using `aapt2 dump chunks` Change-Id: Ibf12394284fdbe3a8047f7ecf4fe68517dfc3abb --- tools/aapt2/ResourceParser.cpp | 2 +- tools/aapt2/ResourceParser_test.cpp | 5 ++++- tools/aapt2/ResourceUtils.cpp | 2 +- tools/aapt2/ResourceValues.h | 1 + tools/aapt2/Resources.proto | 3 +++ tools/aapt2/format/binary/BinaryResourceParser.cpp | 1 + tools/aapt2/format/binary/TableFlattener.cpp | 2 +- tools/aapt2/format/proto/ProtoDeserialize.cpp | 2 ++ tools/aapt2/format/proto/ProtoSerialize.cpp | 1 + tools/aapt2/format/proto/ProtoSerialize_test.cpp | 4 +++- tools/aapt2/process/SymbolTable.cpp | 1 + 11 files changed, 19 insertions(+), 5 deletions(-) (limited to 'tools') diff --git a/tools/aapt2/ResourceParser.cpp b/tools/aapt2/ResourceParser.cpp index 859fe80c5ce7..c55765663204 100644 --- a/tools/aapt2/ResourceParser.cpp +++ b/tools/aapt2/ResourceParser.cpp @@ -1387,7 +1387,7 @@ Maybe ResourceParser::ParseEnumOrFlagItem( return Attribute::Symbol{ Reference(ResourceNameRef({}, ResourceType::kId, maybe_name.value())), - val.data}; + val.data, val.dataType}; } bool ResourceParser::ParseStyleItem(xml::XmlPullParser* parser, Style* style) { diff --git a/tools/aapt2/ResourceParser_test.cpp b/tools/aapt2/ResourceParser_test.cpp index 464225fefb85..42374690d135 100644 --- a/tools/aapt2/ResourceParser_test.cpp +++ b/tools/aapt2/ResourceParser_test.cpp @@ -401,7 +401,7 @@ TEST_F(ResourceParserTest, ParseEnumAttr) { std::string input = R"( - + )"; ASSERT_TRUE(TestParse(input)); @@ -414,14 +414,17 @@ TEST_F(ResourceParserTest, ParseEnumAttr) { ASSERT_TRUE(enum_attr->symbols[0].symbol.name); EXPECT_THAT(enum_attr->symbols[0].symbol.name.value().entry, Eq("bar")); EXPECT_THAT(enum_attr->symbols[0].value, Eq(0u)); + EXPECT_THAT(enum_attr->symbols[0].type, Eq(Res_value::TYPE_INT_DEC)); ASSERT_TRUE(enum_attr->symbols[1].symbol.name); EXPECT_THAT(enum_attr->symbols[1].symbol.name.value().entry, Eq("bat")); EXPECT_THAT(enum_attr->symbols[1].value, Eq(1u)); + EXPECT_THAT(enum_attr->symbols[1].type, Eq(Res_value::TYPE_INT_HEX)); ASSERT_TRUE(enum_attr->symbols[2].symbol.name); EXPECT_THAT(enum_attr->symbols[2].symbol.name.value().entry, Eq("baz")); EXPECT_THAT(enum_attr->symbols[2].value, Eq(2u)); + EXPECT_THAT(enum_attr->symbols[2].type, Eq(Res_value::TYPE_INT_DEC)); } TEST_F(ResourceParserTest, ParseFlagAttr) { diff --git a/tools/aapt2/ResourceUtils.cpp b/tools/aapt2/ResourceUtils.cpp index e0040e486a23..bd2ab5377311 100644 --- a/tools/aapt2/ResourceUtils.cpp +++ b/tools/aapt2/ResourceUtils.cpp @@ -378,7 +378,7 @@ std::unique_ptr TryParseEnumSymbol(const Attribute* enum_attr, const ResourceName& enum_symbol_resource_name = symbol.symbol.name.value(); if (trimmed_str == enum_symbol_resource_name.entry) { android::Res_value value = {}; - value.dataType = android::Res_value::TYPE_INT_DEC; + value.dataType = symbol.type; value.data = symbol.value; return util::make_unique(value); } diff --git a/tools/aapt2/ResourceValues.h b/tools/aapt2/ResourceValues.h index 168ad61784e7..fe0883be50aa 100644 --- a/tools/aapt2/ResourceValues.h +++ b/tools/aapt2/ResourceValues.h @@ -292,6 +292,7 @@ struct Attribute : public BaseValue { struct Symbol { Reference symbol; uint32_t value; + uint8_t type; friend std::ostream& operator<<(std::ostream& out, const Symbol& symbol); }; diff --git a/tools/aapt2/Resources.proto b/tools/aapt2/Resources.proto index b2fc08423d34..65f465240d7d 100644 --- a/tools/aapt2/Resources.proto +++ b/tools/aapt2/Resources.proto @@ -388,6 +388,9 @@ message Attribute { // The value of the enum/flag. uint32 value = 4; + + // The data type of the enum/flag as defined in android::Res_value. + uint32 type = 5; } // Bitmask of formats allowed for an attribute. diff --git a/tools/aapt2/format/binary/BinaryResourceParser.cpp b/tools/aapt2/format/binary/BinaryResourceParser.cpp index fd8e36ebf823..fcd6aaafba7a 100644 --- a/tools/aapt2/format/binary/BinaryResourceParser.cpp +++ b/tools/aapt2/format/binary/BinaryResourceParser.cpp @@ -614,6 +614,7 @@ std::unique_ptr BinaryResourceParser::ParseAttr(const ResourceNameRef if (attr->type_mask & (ResTable_map::TYPE_ENUM | ResTable_map::TYPE_FLAGS)) { Attribute::Symbol symbol; symbol.value = util::DeviceToHost32(map_entry.value.data); + symbol.type = map_entry.value.dataType; symbol.symbol = Reference(util::DeviceToHost32(map_entry.name.ident)); attr->symbols.push_back(std::move(symbol)); } diff --git a/tools/aapt2/format/binary/TableFlattener.cpp b/tools/aapt2/format/binary/TableFlattener.cpp index f2e72da4056a..b9321174100b 100644 --- a/tools/aapt2/format/binary/TableFlattener.cpp +++ b/tools/aapt2/format/binary/TableFlattener.cpp @@ -107,7 +107,7 @@ class MapFlattenVisitor : public ValueVisitor { } for (Attribute::Symbol& s : attr->symbols) { - BinaryPrimitive val(Res_value::TYPE_INT_DEC, s.value); + BinaryPrimitive val(s.type, s.value); FlattenEntry(&s.symbol, &val); } } diff --git a/tools/aapt2/format/proto/ProtoDeserialize.cpp b/tools/aapt2/format/proto/ProtoDeserialize.cpp index bb21c1c539fb..efbf636878f2 100644 --- a/tools/aapt2/format/proto/ProtoDeserialize.cpp +++ b/tools/aapt2/format/proto/ProtoDeserialize.cpp @@ -708,6 +708,8 @@ std::unique_ptr DeserializeValueFromPb(const pb::Value& pb_value, return {}; } symbol.value = pb_symbol.value(); + symbol.type = pb_symbol.type() != 0U ? pb_symbol.type() + : android::Res_value::TYPE_INT_DEC; attr->symbols.push_back(std::move(symbol)); } value = std::move(attr); diff --git a/tools/aapt2/format/proto/ProtoSerialize.cpp b/tools/aapt2/format/proto/ProtoSerialize.cpp index a54822b20302..aa6547e81624 100644 --- a/tools/aapt2/format/proto/ProtoSerialize.cpp +++ b/tools/aapt2/format/proto/ProtoSerialize.cpp @@ -552,6 +552,7 @@ class ValueSerializer : public ConstValueVisitor { SerializeItemMetaDataToPb(symbol.symbol, pb_symbol, src_pool_); SerializeReferenceToPb(symbol.symbol, pb_symbol->mutable_name()); pb_symbol->set_value(symbol.value); + pb_symbol->set_type(symbol.type); } } diff --git a/tools/aapt2/format/proto/ProtoSerialize_test.cpp b/tools/aapt2/format/proto/ProtoSerialize_test.cpp index f252f33f44fb..e7f23302652c 100644 --- a/tools/aapt2/format/proto/ProtoSerialize_test.cpp +++ b/tools/aapt2/format/proto/ProtoSerialize_test.cpp @@ -80,7 +80,7 @@ TEST(ProtoSerializeTest, SerializeSinglePackage) { test::BuildPrimitive(android::Res_value::TYPE_INT_DEC, 123u), context->GetDiagnostics())); ASSERT_TRUE(table->AddResource( test::ParseNameOrDie("com.app.a:integer/one"), test::ParseConfigOrDie("land"), "tablet", - test::BuildPrimitive(android::Res_value::TYPE_INT_DEC, 321u), context->GetDiagnostics())); + test::BuildPrimitive(android::Res_value::TYPE_INT_HEX, 321u), context->GetDiagnostics())); // Make a reference with both resource name and resource ID. // The reference should point to a resource outside of this table to test that both name and id @@ -133,11 +133,13 @@ TEST(ProtoSerializeTest, SerializeSinglePackage) { &new_table, "com.app.a:integer/one", test::ParseConfigOrDie("land"), ""); ASSERT_THAT(prim, NotNull()); EXPECT_THAT(prim->value.data, Eq(123u)); + EXPECT_THAT(prim->value.dataType, Eq(0x10)); prim = test::GetValueForConfigAndProduct( &new_table, "com.app.a:integer/one", test::ParseConfigOrDie("land"), "tablet"); ASSERT_THAT(prim, NotNull()); EXPECT_THAT(prim->value.data, Eq(321u)); + EXPECT_THAT(prim->value.dataType, Eq(0x11)); Reference* actual_ref = test::GetValue(&new_table, "com.app.a:layout/abc"); ASSERT_THAT(actual_ref, NotNull()); diff --git a/tools/aapt2/process/SymbolTable.cpp b/tools/aapt2/process/SymbolTable.cpp index 61a8fbbb7f52..bc09f19b94d5 100644 --- a/tools/aapt2/process/SymbolTable.cpp +++ b/tools/aapt2/process/SymbolTable.cpp @@ -313,6 +313,7 @@ static std::unique_ptr LookupAttributeInTable( symbol.symbol.name = parsed_name.value(); symbol.symbol.id = ResourceId(map_entry.key); symbol.value = map_entry.value.data; + symbol.type = map_entry.value.dataType; s->attribute->symbols.push_back(std::move(symbol)); } } -- cgit v1.2.3 From 81dfca0e057f8888f715fdc560d29ad95c20335e Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Fri, 7 Jun 2019 10:20:27 -0700 Subject: Fix asset compression to check name ends with arg There exists a discrepancy between how aapt1 and aapt2 prevent the compression of assets using the -0 flag. aapt1 checked if the file name ended with an argument, but aapt2 is checking if the file extension exactly matches the argument. This change makes aapt2 behave the same as aapt1 for asset compression using the -0 flag. Bug: 132823799 Test: aapt2_tests Change-Id: I641b3ebce29e4407b543faea373a5ce516b70cda --- tools/aapt2/cmd/Compile_test.cpp | 2 +- tools/aapt2/cmd/Link.cpp | 55 +++++++++-------------- tools/aapt2/cmd/Link.h | 2 +- tools/aapt2/cmd/Link_test.cpp | 96 ++++++++++++++++++++++++++++++++++++++-- tools/aapt2/test/Fixture.cpp | 25 ++++++++--- tools/aapt2/test/Fixture.h | 5 ++- 6 files changed, 140 insertions(+), 45 deletions(-) (limited to 'tools') diff --git a/tools/aapt2/cmd/Compile_test.cpp b/tools/aapt2/cmd/Compile_test.cpp index 5f637bd8d582..fb786a31360e 100644 --- a/tools/aapt2/cmd/Compile_test.cpp +++ b/tools/aapt2/cmd/Compile_test.cpp @@ -200,7 +200,7 @@ static void AssertTranslations(CommandTestFixture *ctf, std::string file_name, const std::string compiled_files_dir = ctf->GetTestPath("/compiled_" + file_name); const std::string out_apk = ctf->GetTestPath("/" + file_name + ".apk"); - CHECK(ctf->WriteFile(source_file, sTranslatableXmlContent)); + ctf->WriteFile(source_file, sTranslatableXmlContent); CHECK(file::mkdirs(compiled_files_dir.data())); ASSERT_EQ(CompileCommand(&diag).Execute({ diff --git a/tools/aapt2/cmd/Link.cpp b/tools/aapt2/cmd/Link.cpp index f354bb610224..4b977225fd01 100644 --- a/tools/aapt2/cmd/Link.cpp +++ b/tools/aapt2/cmd/Link.cpp @@ -297,6 +297,25 @@ struct R { }; }; +template +uint32_t GetCompressionFlags(const StringPiece& str, T options) { + if (options.do_not_compress_anything) { + return 0; + } + + if (options.regex_to_not_compress + && std::regex_search(str.to_string(), options.regex_to_not_compress.value())) { + return 0; + } + + for (const std::string& extension : options.extensions_to_not_compress) { + if (util::EndsWith(str, extension)) { + return 0; + } + } + return ArchiveEntry::kCompress; +} + class ResourceFileFlattener { public: ResourceFileFlattener(const ResourceFileFlattenerOptions& options, IAaptContext* context, @@ -321,8 +340,6 @@ class ResourceFileFlattener { std::string dst_path; }; - uint32_t GetCompressionFlags(const StringPiece& str); - std::vector> LinkAndVersionXmlFile(ResourceTable* table, FileOperation* file_op); @@ -381,26 +398,6 @@ ResourceFileFlattener::ResourceFileFlattener(const ResourceFileFlattenerOptions& } } -// TODO(rtmitchell): turn this function into a variable that points to a method that retrieves the -// compression flag -uint32_t ResourceFileFlattener::GetCompressionFlags(const StringPiece& str) { - if (options_.do_not_compress_anything) { - return 0; - } - - if (options_.regex_to_not_compress - && std::regex_search(str.to_string(), options_.regex_to_not_compress.value())) { - return 0; - } - - for (const std::string& extension : options_.extensions_to_not_compress) { - if (util::EndsWith(str, extension)) { - return 0; - } - } - return ArchiveEntry::kCompress; -} - static bool IsTransitionElement(const std::string& name) { return name == "fade" || name == "changeBounds" || name == "slide" || name == "explode" || name == "changeImageTransform" || name == "changeTransform" || @@ -640,7 +637,8 @@ bool ResourceFileFlattener::Flatten(ResourceTable* table, IArchiveWriter* archiv } } else { error |= !io::CopyFileToArchive(context_, file_op.file_to_copy, file_op.dst_path, - GetCompressionFlags(file_op.dst_path), archive_writer); + GetCompressionFlags(file_op.dst_path, options_), + archive_writer); } } } @@ -1547,16 +1545,7 @@ class Linker { } for (auto& entry : merged_assets) { - uint32_t compression_flags = ArchiveEntry::kCompress; - std::string extension = file::GetExtension(entry.first).to_string(); - - if (options_.do_not_compress_anything - || options_.extensions_to_not_compress.count(extension) > 0 - || (options_.regex_to_not_compress - && std::regex_search(extension, options_.regex_to_not_compress.value()))) { - compression_flags = 0u; - } - + uint32_t compression_flags = GetCompressionFlags(entry.first, options_); if (!io::CopyFileToArchive(context_, entry.second.get(), entry.first, compression_flags, writer)) { return false; diff --git a/tools/aapt2/cmd/Link.h b/tools/aapt2/cmd/Link.h index 7c583858ee1d..5b0653ed53bd 100644 --- a/tools/aapt2/cmd/Link.h +++ b/tools/aapt2/cmd/Link.h @@ -248,7 +248,7 @@ class LinkCommand : public Command { "Changes the name of the target package for instrumentation. Most useful\n" "when used in conjunction with --rename-manifest-package.", &options_.manifest_fixer_options.rename_instrumentation_target_package); - AddOptionalFlagList("-0", "File extensions not to compress.", + AddOptionalFlagList("-0", "File suffix not to compress.", &options_.extensions_to_not_compress); AddOptionalSwitch("--no-compress", "Do not compress any resources.", &options_.do_not_compress_anything); diff --git a/tools/aapt2/cmd/Link_test.cpp b/tools/aapt2/cmd/Link_test.cpp index 9ea93f638aff..32ed1dd81b3f 100644 --- a/tools/aapt2/cmd/Link_test.cpp +++ b/tools/aapt2/cmd/Link_test.cpp @@ -43,10 +43,8 @@ TEST_F(LinkTest, RemoveRawXmlStrings) { // Load the binary xml tree android::ResXMLTree tree; std::unique_ptr apk = LoadedApk::LoadApkFromPath(out_apk, &diag); - std::unique_ptr data = OpenFileAsData(apk.get(), "res/xml/test.xml"); ASSERT_THAT(data, Ne(nullptr)); - AssertLoadXml(apk.get(), data.get(), &tree); // Check that the raw string index has not been assigned @@ -71,10 +69,8 @@ TEST_F(LinkTest, KeepRawXmlStrings) { // Load the binary xml tree android::ResXMLTree tree; std::unique_ptr apk = LoadedApk::LoadApkFromPath(out_apk, &diag); - std::unique_ptr data = OpenFileAsData(apk.get(), "res/xml/test.xml"); ASSERT_THAT(data, Ne(nullptr)); - AssertLoadXml(apk.get(), data.get(), &tree); // Check that the raw string index has been set to the correct string pool entry @@ -83,4 +79,96 @@ TEST_F(LinkTest, KeepRawXmlStrings) { EXPECT_THAT(util::GetString(tree.getStrings(), static_cast(raw_index)), Eq("007")); } +TEST_F(LinkTest, NoCompressAssets) { + StdErrDiagnostics diag; + std::string content(500, 'a'); + WriteFile(GetTestPath("assets/testtxt"), content); + WriteFile(GetTestPath("assets/testtxt2"), content); + WriteFile(GetTestPath("assets/test.txt"), content); + WriteFile(GetTestPath("assets/test.hello.txt"), content); + WriteFile(GetTestPath("assets/test.hello.xml"), content); + + const std::string out_apk = GetTestPath("out.apk"); + std::vector link_args = { + "--manifest", GetDefaultManifest(), + "-o", out_apk, + "-0", ".txt", + "-0", "txt2", + "-0", ".hello.txt", + "-0", "hello.xml", + "-A", GetTestPath("assets") + }; + + ASSERT_TRUE(Link(link_args, &diag)); + + std::unique_ptr apk = LoadedApk::LoadApkFromPath(out_apk, &diag); + ASSERT_THAT(apk, Ne(nullptr)); + io::IFileCollection* zip = apk->GetFileCollection(); + ASSERT_THAT(zip, Ne(nullptr)); + + auto file = zip->FindFile("assets/testtxt"); + ASSERT_THAT(file, Ne(nullptr)); + EXPECT_TRUE(file->WasCompressed()); + + file = zip->FindFile("assets/testtxt2"); + ASSERT_THAT(file, Ne(nullptr)); + EXPECT_FALSE(file->WasCompressed()); + + file = zip->FindFile("assets/test.txt"); + ASSERT_THAT(file, Ne(nullptr)); + EXPECT_FALSE(file->WasCompressed()); + + file = zip->FindFile("assets/test.hello.txt"); + ASSERT_THAT(file, Ne(nullptr)); + EXPECT_FALSE(file->WasCompressed()); + + file = zip->FindFile("assets/test.hello.xml"); + ASSERT_THAT(file, Ne(nullptr)); + EXPECT_FALSE(file->WasCompressed()); +} + +TEST_F(LinkTest, NoCompressResources) { + StdErrDiagnostics diag; + std::string content(500, 'a'); + const std::string compiled_files_dir = GetTestPath("compiled"); + ASSERT_TRUE(CompileFile(GetTestPath("res/raw/testtxt"), content, compiled_files_dir, &diag)); + ASSERT_TRUE(CompileFile(GetTestPath("res/raw/test.txt"), content, compiled_files_dir, &diag)); + ASSERT_TRUE(CompileFile(GetTestPath("res/raw/test1.hello.txt"), content, compiled_files_dir, + &diag)); + ASSERT_TRUE(CompileFile(GetTestPath("res/raw/test2.goodbye.xml"), content, compiled_files_dir, + &diag)); + + const std::string out_apk = GetTestPath("out.apk"); + std::vector link_args = { + "--manifest", GetDefaultManifest(), + "-o", out_apk, + "-0", ".txt", + "-0", ".hello.txt", + "-0", "goodbye.xml", + }; + + ASSERT_TRUE(Link(link_args, compiled_files_dir, &diag)); + + std::unique_ptr apk = LoadedApk::LoadApkFromPath(out_apk, &diag); + ASSERT_THAT(apk, Ne(nullptr)); + io::IFileCollection* zip = apk->GetFileCollection(); + ASSERT_THAT(zip, Ne(nullptr)); + + auto file = zip->FindFile("res/raw/testtxt"); + ASSERT_THAT(file, Ne(nullptr)); + EXPECT_TRUE(file->WasCompressed()); + + file = zip->FindFile("res/raw/test.txt"); + ASSERT_THAT(file, Ne(nullptr)); + EXPECT_FALSE(file->WasCompressed()); + + file = zip->FindFile("res/raw/test1.hello.hello.txt"); + ASSERT_THAT(file, Ne(nullptr)); + EXPECT_FALSE(file->WasCompressed()); + + file = zip->FindFile("res/raw/test2.goodbye.goodbye.xml"); + ASSERT_THAT(file, Ne(nullptr)); + EXPECT_FALSE(file->WasCompressed()); +} + } // namespace aapt \ No newline at end of file diff --git a/tools/aapt2/test/Fixture.cpp b/tools/aapt2/test/Fixture.cpp index a51b4a4649f1..5386802dbc8e 100644 --- a/tools/aapt2/test/Fixture.cpp +++ b/tools/aapt2/test/Fixture.cpp @@ -80,7 +80,7 @@ void TestDirectoryFixture::TearDown() { ClearDirectory(temp_dir_); } -bool TestDirectoryFixture::WriteFile(const std::string& path, const std::string& contents) { +void TestDirectoryFixture::WriteFile(const std::string& path, const std::string& contents) { CHECK(util::StartsWith(path, temp_dir_)) << "Attempting to create a file outside of test temporary directory."; @@ -91,16 +91,31 @@ bool TestDirectoryFixture::WriteFile(const std::string& path, const std::string& file::mkdirs(dirs); } - return android::base::WriteStringToFile(contents, path); + CHECK(android::base::WriteStringToFile(contents, path)); } bool CommandTestFixture::CompileFile(const std::string& path, const std::string& contents, const android::StringPiece& out_dir, IDiagnostics* diag) { - CHECK(WriteFile(path, contents)); + WriteFile(path, contents); CHECK(file::mkdirs(out_dir.data())); return CompileCommand(diag).Execute({path, "-o", out_dir, "-v"}, &std::cerr) == 0; } +bool CommandTestFixture::Link(const std::vector& args, IDiagnostics* diag) { + std::vector link_args; + for(const std::string& arg : args) { + link_args.emplace_back(arg); + } + + // Link against the android SDK + std::string android_sdk = file::BuildPath({android::base::GetExecutableDirectory(), + "integration-tests", "CommandTests", + "android-28.jar"}); + link_args.insert(link_args.end(), {"-I", android_sdk}); + + return LinkCommand(diag).Execute(link_args, &std::cerr) == 0; +} + bool CommandTestFixture::Link(const std::vector& args, const android::StringPiece& flat_dir, IDiagnostics* diag) { std::vector link_args; @@ -128,10 +143,10 @@ bool CommandTestFixture::Link(const std::vector& args, std::string CommandTestFixture::GetDefaultManifest(const char* package_name) { const std::string manifest_file = GetTestPath("AndroidManifest.xml"); - CHECK(WriteFile(manifest_file, android::base::StringPrintf(R"( + WriteFile(manifest_file, android::base::StringPrintf(R"( - )", package_name))); + )", package_name)); return manifest_file; } diff --git a/tools/aapt2/test/Fixture.h b/tools/aapt2/test/Fixture.h index fce2aebfecaa..457d65e30b65 100644 --- a/tools/aapt2/test/Fixture.h +++ b/tools/aapt2/test/Fixture.h @@ -58,7 +58,7 @@ class TestDirectoryFixture : public ::testing::Test { // Creates a file with the specified contents, creates any intermediate directories in the // process. The file path must be an absolute path within the test directory. - bool WriteFile(const std::string& path, const std::string& contents); + void WriteFile(const std::string& path, const std::string& contents); private: std::string temp_dir_; @@ -75,6 +75,9 @@ class CommandTestFixture : public TestDirectoryFixture { bool CompileFile(const std::string& path, const std::string& contents, const android::StringPiece& flat_out_dir, IDiagnostics* diag); + // Executes the link command with the specified arguments. + bool Link(const std::vector& args, IDiagnostics* diag); + // Executes the link command with the specified arguments. The flattened files residing in the // flat directory will be added to the link command as file arguments. bool Link(const std::vector& args, const android::StringPiece& flat_dir, -- cgit v1.2.3 From 121c6e8aa037efcbd047bdd5f53e6686a07fa002 Mon Sep 17 00:00:00 2001 From: Donald Chai Date: Wed, 12 Jun 2019 12:51:57 -0700 Subject: [aapt2] Add "link" option to override styles instead of overlaying. For normal app development, the desired linking semantics are: * styleables - take union of all definitions * all other resources - take last non-weak definition This differs from the semantics needed in other scenarios, where merging/overlaying styles is desired. Bug: 134525082 Change-Id: Iac0c43ca2ecf1f3fddc9c3367f8914c12c9258e1 Tested: aapt2_tests --- tools/aapt2/cmd/Link.cpp | 2 + tools/aapt2/cmd/Link.h | 5 +++ tools/aapt2/cmd/Link_test.cpp | 84 ++++++++++++++++++++++++++++++++++- tools/aapt2/link/TableMerger.cpp | 27 ++++++----- tools/aapt2/link/TableMerger.h | 2 + tools/aapt2/link/TableMerger_test.cpp | 47 ++++++++++++++++++++ 6 files changed, 155 insertions(+), 12 deletions(-) (limited to 'tools') diff --git a/tools/aapt2/cmd/Link.cpp b/tools/aapt2/cmd/Link.cpp index 4b977225fd01..04d12f829e3c 100644 --- a/tools/aapt2/cmd/Link.cpp +++ b/tools/aapt2/cmd/Link.cpp @@ -1691,6 +1691,8 @@ class Linker { TableMergerOptions table_merger_options; table_merger_options.auto_add_overlay = options_.auto_add_overlay; + table_merger_options.override_styles_instead_of_overlaying = + options_.override_styles_instead_of_overlaying; table_merger_options.strict_visibility = options_.strict_visibility; table_merger_ = util::make_unique(context_, &final_table_, table_merger_options); diff --git a/tools/aapt2/cmd/Link.h b/tools/aapt2/cmd/Link.h index 5b0653ed53bd..37765f6b5d08 100644 --- a/tools/aapt2/cmd/Link.h +++ b/tools/aapt2/cmd/Link.h @@ -42,6 +42,7 @@ struct LinkOptions { std::vector assets_dirs; bool output_to_directory = false; bool auto_add_overlay = false; + bool override_styles_instead_of_overlaying = false; OutputFormat output_format = OutputFormat::kApk; // Java/Proguard options. @@ -242,6 +243,10 @@ class LinkCommand : public Command { "Allows the addition of new resources in overlays without\n" " tags.", &options_.auto_add_overlay); + AddOptionalSwitch("--override-styles-instead-of-overlaying", + "Causes styles defined in -R resources to replace previous definitions\n" + "instead of merging into them\n", + &options_.override_styles_instead_of_overlaying); AddOptionalFlag("--rename-manifest-package", "Renames the package in AndroidManifest.xml.", &options_.manifest_fixer_options.rename_manifest_package); AddOptionalFlag("--rename-instrumentation-target-package", diff --git a/tools/aapt2/cmd/Link_test.cpp b/tools/aapt2/cmd/Link_test.cpp index 32ed1dd81b3f..bf8f043f6b68 100644 --- a/tools/aapt2/cmd/Link_test.cpp +++ b/tools/aapt2/cmd/Link_test.cpp @@ -171,4 +171,86 @@ TEST_F(LinkTest, NoCompressResources) { EXPECT_FALSE(file->WasCompressed()); } -} // namespace aapt \ No newline at end of file +TEST_F(LinkTest, OverlayStyles) { + StdErrDiagnostics diag; + const std::string compiled_files_dir = GetTestPath("compiled"); + const std::string override_files_dir = GetTestPath("compiled-override"); + ASSERT_TRUE(CompileFile(GetTestPath("res/values/values.xml"), + R"( + + )", + compiled_files_dir, &diag)); + ASSERT_TRUE(CompileFile(GetTestPath("res/values/values-override.xml"), + R"( + + )", + override_files_dir, &diag)); + + + const std::string out_apk = GetTestPath("out.apk"); + std::vector link_args = { + "--manifest", GetDefaultManifest(kDefaultPackageName), + "-o", out_apk, + }; + const auto override_files = file::FindFiles(override_files_dir, &diag); + for (const auto &override_file : override_files.value()) { + link_args.push_back("-R"); + link_args.push_back(file::BuildPath({override_files_dir, override_file})); + } + ASSERT_TRUE(Link(link_args, compiled_files_dir, &diag)); + + std::unique_ptr apk = LoadedApk::LoadApkFromPath(out_apk, &diag); + const Style* actual_style = test::GetValue + )", + compiled_files_dir, &diag)); + ASSERT_TRUE(CompileFile(GetTestPath("res/values/values-override.xml"), + R"( + + )", + override_files_dir, &diag)); + + + const std::string out_apk = GetTestPath("out.apk"); + std::vector link_args = { + "--manifest", GetDefaultManifest(kDefaultPackageName), + "--override-styles-instead-of-overlaying", + "-o", out_apk, + }; + const auto override_files = file::FindFiles(override_files_dir, &diag); + for (const auto &override_file : override_files.value()) { + link_args.push_back("-R"); + link_args.push_back(file::BuildPath({override_files_dir, override_file})); + } + ASSERT_TRUE(Link(link_args, compiled_files_dir, &diag)); + + std::unique_ptr apk = LoadedApk::LoadApkFromPath(out_apk, &diag); + const Style* actual_style = test::GetValue + + + + + +