AAPT2: Fix merging of styleables the right way
Styleables should only be merged when processing overlays.
This moves the styleable merging code out of ResourceTable
and into TableMerger.
Change-Id: I3aae05cf4dd875cd25ac2ac744b61194409b2fee
diff --git a/tools/aapt2/link/TableMerger.cpp b/tools/aapt2/link/TableMerger.cpp
index 379c991..889ac70 100644
--- a/tools/aapt2/link/TableMerger.cpp
+++ b/tools/aapt2/link/TableMerger.cpp
@@ -45,49 +45,56 @@
}
/**
+ * Ignore packages with an ID that is not our desired package ID or 0x0, or if the name
+ * is not equal to the package we are compiling.
+ */
+static bool shouldIgnorePackage(IAaptContext* context, ResourceTablePackage* package) {
+ const Maybe<ResourceId>& id = package->id;
+ const std::string& packageName = package->name;
+ return (id && id.value() != 0x0 && id.value() != context->getPackageId())
+ || (!packageName.empty() && packageName != context->getCompilationPackage());
+}
+
+/**
* This will merge packages with the same package name (or no package name).
*/
bool TableMerger::mergeImpl(const Source& src, ResourceTable* table,
io::IFileCollection* collection,
bool overlay, bool allowNew) {
- const uint8_t desiredPackageId = mContext->getPackageId();
-
bool error = false;
for (auto& package : table->packages) {
- // Warn of packages with an unrelated ID.
- if (package->id && package->id.value() != 0x0 && package->id.value() != desiredPackageId) {
+ // Warn of packages with an unrelated ID or name.
+ if (shouldIgnorePackage(mContext, package.get())) {
mContext->getDiagnostics()->warn(DiagMessage(src)
<< "ignoring package " << package->name);
continue;
}
- if (package->name.empty() || mContext->getCompilationPackage() == package->name) {
- FileMergeCallback callback;
- if (collection) {
- callback = [&](const ResourceNameRef& name, const ConfigDescription& config,
- FileReference* newFile, FileReference* oldFile) -> bool {
- // The old file's path points inside the APK, so we can use it as is.
- io::IFile* f = collection->findFile(*oldFile->path);
- if (!f) {
- mContext->getDiagnostics()->error(DiagMessage(src) << "file '"
- << *oldFile->path
- << "' not found");
- return false;
- }
+ FileMergeCallback callback;
+ if (collection) {
+ callback = [&](const ResourceNameRef& name, const ConfigDescription& config,
+ FileReference* newFile, FileReference* oldFile) -> bool {
+ // The old file's path points inside the APK, so we can use it as is.
+ io::IFile* f = collection->findFile(*oldFile->path);
+ if (!f) {
+ mContext->getDiagnostics()->error(DiagMessage(src) << "file '"
+ << *oldFile->path
+ << "' not found");
+ return false;
+ }
- newFile->file = f;
- return true;
- };
- }
-
- // Merge here. Once the entries are merged and mangled, any references to
- // them are still valid. This is because un-mangled references are
- // mangled, then looked up at resolution time.
- // Also, when linking, we convert references with no package name to use
- // the compilation package name.
- error |= !doMerge(src, table, package.get(),
- false /* mangle */, overlay, allowNew, callback);
+ newFile->file = f;
+ return true;
+ };
}
+
+ // Merge here. Once the entries are merged and mangled, any references to
+ // them are still valid. This is because un-mangled references are
+ // mangled, then looked up at resolution time.
+ // Also, when linking, we convert references with no package name to use
+ // the compilation package name.
+ error |= !doMerge(src, table, package.get(), false /* mangle */, overlay, allowNew,
+ callback);
}
return !error;
}
@@ -129,6 +136,106 @@
return !error;
}
+static bool mergeType(IAaptContext* context, const Source& src, ResourceTableType* dstType,
+ ResourceTableType* srcType) {
+ if (dstType->symbolStatus.state < srcType->symbolStatus.state) {
+ // The incoming type's visibility is stronger, so we should override
+ // the visibility.
+ if (srcType->symbolStatus.state == SymbolState::kPublic) {
+ // Only copy the ID if the source is public, or else the ID is meaningless.
+ dstType->id = srcType->id;
+ }
+ dstType->symbolStatus = std::move(srcType->symbolStatus);
+ } else if (dstType->symbolStatus.state == SymbolState::kPublic
+ && srcType->symbolStatus.state == SymbolState::kPublic
+ && dstType->id && srcType->id
+ && dstType->id.value() != srcType->id.value()) {
+ // Both types are public and have different IDs.
+ context->getDiagnostics()->error(DiagMessage(src)
+ << "cannot merge type '" << srcType->type
+ << "': conflicting public IDs");
+ return false;
+ }
+ return true;
+}
+
+static bool mergeEntry(IAaptContext* context, const Source& src, ResourceEntry* dstEntry,
+ ResourceEntry* srcEntry) {
+ if (dstEntry->symbolStatus.state < srcEntry->symbolStatus.state) {
+ // The incoming type's visibility is stronger, so we should override
+ // the visibility.
+ if (srcEntry->symbolStatus.state == SymbolState::kPublic) {
+ // Only copy the ID if the source is public, or else the ID is meaningless.
+ dstEntry->id = srcEntry->id;
+ }
+ dstEntry->symbolStatus = std::move(srcEntry->symbolStatus);
+ } else if (srcEntry->symbolStatus.state == SymbolState::kPublic
+ && dstEntry->symbolStatus.state == SymbolState::kPublic
+ && dstEntry->id && srcEntry->id
+ && dstEntry->id.value() != srcEntry->id.value()) {
+ // Both entries are public and have different IDs.
+ context->getDiagnostics()->error(DiagMessage(src)
+ << "cannot merge entry '" << srcEntry->name
+ << "': conflicting public IDs");
+ return false;
+ }
+ return true;
+}
+
+/**
+ * Modified CollisionResolver which will merge Styleables. Used with overlays.
+ *
+ * Styleables are not actual resources, but they are treated as such during the
+ * compilation phase. Styleables don't simply overlay each other, their definitions merge
+ * and accumulate. If both values are Styleables, we just merge them into the existing value.
+ */
+static ResourceTable::CollisionResult resolveMergeCollision(Value* existing, Value* incoming) {
+ if (Styleable* existingStyleable = valueCast<Styleable>(existing)) {
+ if (Styleable* incomingStyleable = valueCast<Styleable>(incoming)) {
+ // Styleables get merged.
+ existingStyleable->mergeWith(incomingStyleable);
+ return ResourceTable::CollisionResult::kKeepOriginal;
+ }
+ }
+ // Delegate to the default handler.
+ return ResourceTable::resolveValueCollision(existing, incoming);
+}
+
+static ResourceTable::CollisionResult mergeConfigValue(IAaptContext* context,
+ const ResourceNameRef& resName,
+ const bool overlay,
+ ResourceConfigValue* dstConfigValue,
+ ResourceConfigValue* srcConfigValue) {
+ using CollisionResult = ResourceTable::CollisionResult;
+
+ Value* dstValue = dstConfigValue->value.get();
+ Value* srcValue = srcConfigValue->value.get();
+
+ CollisionResult collisionResult;
+ if (overlay) {
+ collisionResult = resolveMergeCollision(dstValue, srcValue);
+ } else {
+ collisionResult = ResourceTable::resolveValueCollision(dstValue, srcValue);
+ }
+
+ if (collisionResult == CollisionResult::kConflict) {
+ if (overlay) {
+ return CollisionResult::kTakeNew;
+ }
+
+ // Error!
+ context->getDiagnostics()->error(DiagMessage(srcValue->getSource())
+ << "resource '" << resName
+ << "' has a conflicting value for "
+ << "configuration ("
+ << srcConfigValue->config << ")");
+ context->getDiagnostics()->note(DiagMessage(dstValue->getSource())
+ << "originally defined here");
+ return CollisionResult::kConflict;
+ }
+ return collisionResult;
+}
+
bool TableMerger::doMerge(const Source& src,
ResourceTable* srcTable,
ResourceTablePackage* srcPackage,
@@ -140,115 +247,64 @@
for (auto& srcType : srcPackage->types) {
ResourceTableType* dstType = mMasterPackage->findOrCreateType(srcType->type);
- if (srcType->symbolStatus.state == SymbolState::kPublic) {
- if (dstType->symbolStatus.state == SymbolState::kPublic && dstType->id && srcType->id
- && dstType->id.value() != srcType->id.value()) {
- // Both types are public and have different IDs.
- mContext->getDiagnostics()->error(DiagMessage(src)
- << "can not merge type '"
- << srcType->type
- << "': conflicting public IDs");
- error = true;
- continue;
- }
-
- dstType->symbolStatus = std::move(srcType->symbolStatus);
- dstType->id = srcType->id;
+ if (!mergeType(mContext, src, dstType, srcType.get())) {
+ error = true;
+ continue;
}
for (auto& srcEntry : srcType->entries) {
- ResourceEntry* dstEntry;
+ std::string entryName = srcEntry->name;
if (manglePackage) {
- std::string mangledName = NameMangler::mangleEntry(srcPackage->name,
- srcEntry->name);
- if (allowNewResources) {
- dstEntry = dstType->findOrCreateEntry(mangledName);
- } else {
- dstEntry = dstType->findEntry(mangledName);
- }
- } else {
- if (allowNewResources) {
- dstEntry = dstType->findOrCreateEntry(srcEntry->name);
- } else {
- dstEntry = dstType->findEntry(srcEntry->name);
- }
+ entryName = NameMangler::mangleEntry(srcPackage->name, srcEntry->name);
}
+ ResourceEntry* dstEntry;
+ if (allowNewResources) {
+ dstEntry = dstType->findOrCreateEntry(entryName);
+ } else {
+ dstEntry = dstType->findEntry(entryName);
+ }
+
+ const ResourceNameRef resName(srcPackage->name, srcType->type, srcEntry->name);
+
if (!dstEntry) {
mContext->getDiagnostics()->error(DiagMessage(src)
- << "resource "
- << ResourceNameRef(srcPackage->name,
- srcType->type,
- srcEntry->name)
+ << "resource " << resName
<< " does not override an existing resource");
mContext->getDiagnostics()->note(DiagMessage(src)
<< "define an <add-resource> tag or use "
- "--auto-add-overlay");
+ << "--auto-add-overlay");
error = true;
continue;
}
- if (srcEntry->symbolStatus.state != SymbolState::kUndefined) {
- if (srcEntry->symbolStatus.state == SymbolState::kPublic) {
- if (dstEntry->symbolStatus.state == SymbolState::kPublic &&
- dstEntry->id && srcEntry->id &&
- dstEntry->id.value() != srcEntry->id.value()) {
- // Both entries are public and have different IDs.
- mContext->getDiagnostics()->error(DiagMessage(src)
- << "can not merge entry '"
- << srcEntry->name
- << "': conflicting public IDs");
- error = true;
- continue;
- }
-
- if (srcEntry->id) {
- dstEntry->id = srcEntry->id;
- }
- }
-
- if (dstEntry->symbolStatus.state != SymbolState::kPublic &&
- dstEntry->symbolStatus.state != srcEntry->symbolStatus.state) {
- dstEntry->symbolStatus = std::move(srcEntry->symbolStatus);
- }
+ if (!mergeEntry(mContext, src, dstEntry, srcEntry.get())) {
+ error = true;
+ continue;
}
- ResourceNameRef resName(mMasterPackage->name, dstType->type, dstEntry->name);
+ for (auto& srcConfigValue : srcEntry->values) {
+ using CollisionResult = ResourceTable::CollisionResult;
- for (auto& srcValue : srcEntry->values) {
- ResourceConfigValue* dstValue = dstEntry->findValue(srcValue->config,
- srcValue->product);
- if (dstValue) {
- const int collisionResult = ResourceTable::resolveValueCollision(
- dstValue->value.get(), srcValue->value.get());
- if (collisionResult == 0 && !overlay) {
- // Error!
- ResourceNameRef resourceName(srcPackage->name,
- srcType->type,
- srcEntry->name);
-
- mContext->getDiagnostics()->error(DiagMessage(srcValue->value->getSource())
- << "resource '" << resourceName
- << "' has a conflicting value for "
- << "configuration ("
- << srcValue->config << ")");
- mContext->getDiagnostics()->note(DiagMessage(dstValue->value->getSource())
- << "originally defined here");
+ ResourceConfigValue* dstConfigValue = dstEntry->findValue(srcConfigValue->config,
+ srcConfigValue->product);
+ if (dstConfigValue) {
+ CollisionResult collisionResult = mergeConfigValue(
+ mContext, resName, overlay, dstConfigValue, srcConfigValue.get());
+ if (collisionResult == CollisionResult::kConflict) {
error = true;
continue;
- } else if (collisionResult < 0) {
- // Keep our existing value.
+ } else if (collisionResult == CollisionResult::kKeepOriginal) {
continue;
}
-
+ } else {
+ dstConfigValue = dstEntry->findOrCreateValue(srcConfigValue->config,
+ srcConfigValue->product);
}
- if (!dstValue) {
- // Force create the entry if we didn't have it.
- dstValue = dstEntry->findOrCreateValue(srcValue->config, srcValue->product);
- }
+ // Continue if we're taking the new resource.
- if (FileReference* f = valueCast<FileReference>(srcValue->value.get())) {
+ if (FileReference* f = valueCast<FileReference>(srcConfigValue->value.get())) {
std::unique_ptr<FileReference> newFileRef;
if (manglePackage) {
newFileRef = cloneAndMangleFile(srcPackage->name, *f);
@@ -258,15 +314,15 @@
}
if (callback) {
- if (!callback(resName, srcValue->config, newFileRef.get(), f)) {
+ if (!callback(resName, srcConfigValue->config, newFileRef.get(), f)) {
error = true;
continue;
}
}
- dstValue->value = std::move(newFileRef);
+ dstConfigValue->value = std::move(newFileRef);
} else {
- dstValue->value = std::unique_ptr<Value>(srcValue->value->clone(
+ dstConfigValue->value = std::unique_ptr<Value>(srcConfigValue->value->clone(
&mMasterTable->stringPool));
}
}
@@ -277,7 +333,6 @@
std::unique_ptr<FileReference> TableMerger::cloneAndMangleFile(const std::string& package,
const FileReference& fileRef) {
-
StringPiece prefix, entry, suffix;
if (util::extractResFilePathParts(*fileRef.path, &prefix, &entry, &suffix)) {
std::string mangledEntry = NameMangler::mangleEntry(package, entry.toString());