Fix persisting which resources use feature flags
File resources get into the table a different way when they haven't been
modified by the FlaggedXmlVersioner or XmlCompatVersioner and this
addresses that path.
With this change, during compile we now find if an xml resource uses
flags and store that in the XmlResource which is then persisted to and
retrieved from the proto. This means the FlaggedXmlVersioner doesn't
have to look for them in the case that the file config or minsdk is >=
baklava.
This also moved the FeatureFlagsFilter to before we version the files.
The FeatureFlagsFilter is what strips elements behind disabled read only
flags at compile time and removed the featureFlag attribute when the
element is behind an enabled read only flag. This is so that the
FlaggedXmlVersioner doesn't have to worry if the flags it sees are
read/write or readonly. It should only ever encounter read/write flags.
Test: Automation
Bug: 377974898
Flag: android.content.res.layout_readwrite_flags
Change-Id: Ia6cbf55bc9f8d594eeb5c44c143565e93684ae2c
diff --git a/tools/aapt2/ResourcesInternal.proto b/tools/aapt2/ResourcesInternal.proto
index f4735a2..380c5f2 100644
--- a/tools/aapt2/ResourcesInternal.proto
+++ b/tools/aapt2/ResourcesInternal.proto
@@ -50,8 +50,11 @@
// Any symbols this file auto-generates/exports (eg. @+id/foo in an XML file).
repeated Symbol exported_symbol = 5;
- // The status of the flag the file is behind if any
+ // The status of the read only flag the file is behind if any
uint32 flag_status = 6;
bool flag_negated = 7;
string flag_name = 8;
+
+ // Whether the file uses read/write feature flags
+ bool uses_readwrite_feature_flags = 9;
}
diff --git a/tools/aapt2/cmd/Compile.cpp b/tools/aapt2/cmd/Compile.cpp
index a5e18d35..3b4f542 100644
--- a/tools/aapt2/cmd/Compile.cpp
+++ b/tools/aapt2/cmd/Compile.cpp
@@ -407,6 +407,45 @@
return true;
}
+class FindReadWriteFlagsVisitor : public xml::Visitor {
+ public:
+ FindReadWriteFlagsVisitor(const FeatureFlagValues& feature_flag_values)
+ : feature_flag_values_(feature_flag_values) {
+ }
+
+ void Visit(xml::Element* node) override {
+ if (had_flags_) {
+ return;
+ }
+ auto* attr = node->FindAttribute(xml::kSchemaAndroid, xml::kAttrFeatureFlag);
+ if (attr != nullptr) {
+ std::string_view flag_name = util::TrimWhitespace(attr->value);
+ if (flag_name.starts_with('!')) {
+ flag_name = flag_name.substr(1);
+ }
+ if (auto it = feature_flag_values_.find(flag_name); it != feature_flag_values_.end()) {
+ if (!it->second.read_only) {
+ had_flags_ = true;
+ return;
+ }
+ } else {
+ // Flag not passed to aapt2, must evaluate at runtime
+ had_flags_ = true;
+ return;
+ }
+ }
+ VisitChildren(node);
+ }
+
+ bool HadFlags() const {
+ return had_flags_;
+ }
+
+ private:
+ bool had_flags_ = false;
+ const FeatureFlagValues& feature_flag_values_;
+};
+
static bool CompileXml(IAaptContext* context, const CompileOptions& options,
const ResourcePathData& path_data, io::IFile* file, IArchiveWriter* writer,
const std::string& output_path) {
@@ -436,6 +475,10 @@
xmlres->file.type = ResourceFile::Type::kProtoXml;
xmlres->file.flag = ParseFlag(path_data.flag_name);
+ FindReadWriteFlagsVisitor visitor(options.feature_flag_values);
+ xmlres->root->Accept(&visitor);
+ xmlres->file.uses_readwrite_feature_flags = visitor.HadFlags();
+
if (xmlres->file.flag) {
std::string error;
auto flag_status = GetFlagStatus(xmlres->file.flag, options.feature_flag_values, &error);
diff --git a/tools/aapt2/cmd/Link.cpp b/tools/aapt2/cmd/Link.cpp
index 755dbb6..0e18ee2 100644
--- a/tools/aapt2/cmd/Link.cpp
+++ b/tools/aapt2/cmd/Link.cpp
@@ -615,6 +615,8 @@
file_op.xml_to_flatten->file.source = file_ref->GetSource();
file_op.xml_to_flatten->file.name =
ResourceName(pkg->name, type->named_type, entry->name);
+ file_op.xml_to_flatten->file.uses_readwrite_feature_flags =
+ config_value->uses_readwrite_feature_flags;
}
// NOTE(adamlesinski): Explicitly construct a StringPiece here, or
@@ -647,6 +649,17 @@
}
}
+ FeatureFlagsFilterOptions flags_filter_options;
+ // Don't fail on unrecognized flags or flags without values as these flags might be
+ // defined and have a value by the time they are evaluated at runtime.
+ flags_filter_options.fail_on_unrecognized_flags = false;
+ flags_filter_options.flags_must_have_value = false;
+ flags_filter_options.remove_disabled_elements = true;
+ FeatureFlagsFilter flags_filter(options_.feature_flag_values, flags_filter_options);
+ if (!flags_filter.Consume(context_, file_op.xml_to_flatten.get())) {
+ return 1;
+ }
+
std::vector<std::unique_ptr<xml::XmlResource>> versioned_docs =
LinkAndVersionXmlFile(table, &file_op);
if (versioned_docs.empty()) {
@@ -673,6 +686,7 @@
// Update the output format of this XML file.
file_ref->type = XmlFileTypeForOutputFormat(options_.output_format);
+
bool result = table->AddResource(
NewResourceBuilder(file.name)
.SetValue(std::move(file_ref), file.config)
@@ -685,14 +699,6 @@
}
}
- FeatureFlagsFilterOptions flags_filter_options;
- flags_filter_options.fail_on_unrecognized_flags = false;
- flags_filter_options.flags_must_have_value = false;
- FeatureFlagsFilter flags_filter(options_.feature_flag_values, flags_filter_options);
- if (!flags_filter.Consume(context_, doc.get())) {
- return 1;
- }
-
error |= !FlattenXml(context_, *doc, dst_path, options_.keep_raw_values,
false /*utf16*/, options_.output_format, archive_writer);
}
diff --git a/tools/aapt2/format/proto/ProtoDeserialize.cpp b/tools/aapt2/format/proto/ProtoDeserialize.cpp
index 91ec348..b893655 100644
--- a/tools/aapt2/format/proto/ProtoDeserialize.cpp
+++ b/tools/aapt2/format/proto/ProtoDeserialize.cpp
@@ -640,6 +640,7 @@
out_file->name = name_ref.ToResourceName();
out_file->source.path = pb_file.source_path();
out_file->type = DeserializeFileReferenceTypeFromPb(pb_file.type());
+ out_file->uses_readwrite_feature_flags = pb_file.uses_readwrite_feature_flags();
out_file->flag_status = (FlagStatus)pb_file.flag_status();
if (!pb_file.flag_name().empty()) {
diff --git a/tools/aapt2/format/proto/ProtoSerialize.cpp b/tools/aapt2/format/proto/ProtoSerialize.cpp
index fcc77d5..da99c4f 100644
--- a/tools/aapt2/format/proto/ProtoSerialize.cpp
+++ b/tools/aapt2/format/proto/ProtoSerialize.cpp
@@ -767,6 +767,7 @@
out_file->set_flag_negated(file.flag->negated);
out_file->set_flag_name(file.flag->name);
}
+ out_file->set_uses_readwrite_feature_flags(file.uses_readwrite_feature_flags);
for (const SourcedResourceName& exported : file.exported_symbols) {
pb::internal::CompiledFile_Symbol* pb_symbol = out_file->add_exported_symbol();
diff --git a/tools/aapt2/link/FlaggedResources_test.cpp b/tools/aapt2/link/FlaggedResources_test.cpp
index 47a71fe..4dcb850 100644
--- a/tools/aapt2/link/FlaggedResources_test.cpp
+++ b/tools/aapt2/link/FlaggedResources_test.cpp
@@ -226,9 +226,11 @@
}
}
}
+
ASSERT_TRUE(found) << "No entry for layout1 at v36 with FLAG_USES_FEATURE_FLAGS bit set";
- // There should only be 1 entry that has the FLAG_USES_FEATURE_FLAGS bit of flags set to 1
- ASSERT_EQ(fields_flagged, 1);
+ // There should only be 2 entry that has the FLAG_USES_FEATURE_FLAGS bit of flags set to 1, the
+ // three versions of the layout file that has flags
+ ASSERT_EQ(fields_flagged, 3);
}
} // namespace aapt
diff --git a/tools/aapt2/link/FlaggedXmlVersioner.cpp b/tools/aapt2/link/FlaggedXmlVersioner.cpp
index 8a3337c..626cae7 100644
--- a/tools/aapt2/link/FlaggedXmlVersioner.cpp
+++ b/tools/aapt2/link/FlaggedXmlVersioner.cpp
@@ -35,10 +35,6 @@
VisitChildren(node);
}
- bool HadFlags() const {
- return had_flags_;
- }
-
private:
bool FixupOrShouldRemove(const std::unique_ptr<xml::Node>& node) {
if (auto* el = NodeCast<Element>(node.get())) {
@@ -47,7 +43,6 @@
return false;
}
- had_flags_ = true;
// This class assumes all flags are disabled so we want to remove any elements behind flags
// unless the flag specification is negated. In the negated case we remove the featureFlag
// attribute because we have already determined whether we are keeping the element or not.
@@ -62,56 +57,27 @@
return false;
}
-
- bool had_flags_ = false;
-};
-
-// An xml visitor that goes through the a doc and determines if any elements are behind a flag.
-class FindFlagsVisitor : public xml::Visitor {
- public:
- void Visit(xml::Element* node) override {
- if (had_flags_) {
- return;
- }
- auto* attr = node->FindAttribute(xml::kSchemaAndroid, xml::kAttrFeatureFlag);
- if (attr != nullptr) {
- had_flags_ = true;
- return;
- }
- VisitChildren(node);
- }
-
- bool HadFlags() const {
- return had_flags_;
- }
-
- bool had_flags_ = false;
};
std::vector<std::unique_ptr<xml::XmlResource>> FlaggedXmlVersioner::Process(IAaptContext* context,
xml::XmlResource* doc) {
std::vector<std::unique_ptr<xml::XmlResource>> docs;
- if ((static_cast<ApiVersion>(doc->file.config.sdkVersion) >= SDK_BAKLAVA) ||
- (static_cast<ApiVersion>(context->GetMinSdkVersion()) >= SDK_BAKLAVA)) {
+ if (!doc->file.uses_readwrite_feature_flags) {
+ docs.push_back(doc->Clone());
+ } else if ((static_cast<ApiVersion>(doc->file.config.sdkVersion) >= SDK_BAKLAVA) ||
+ (static_cast<ApiVersion>(context->GetMinSdkVersion()) >= SDK_BAKLAVA)) {
// Support for read/write flags was added in baklava so if the doc will only get used on
// baklava or later we can just return the original doc.
docs.push_back(doc->Clone());
- FindFlagsVisitor visitor;
- doc->root->Accept(&visitor);
- docs.back()->file.uses_readwrite_feature_flags = visitor.HadFlags();
} else {
auto preBaklavaVersion = doc->Clone();
AllDisabledFlagsVisitor visitor;
preBaklavaVersion->root->Accept(&visitor);
- preBaklavaVersion->file.uses_readwrite_feature_flags = false;
docs.push_back(std::move(preBaklavaVersion));
- if (visitor.HadFlags()) {
- auto baklavaVersion = doc->Clone();
- baklavaVersion->file.config.sdkVersion = SDK_BAKLAVA;
- baklavaVersion->file.uses_readwrite_feature_flags = true;
- docs.push_back(std::move(baklavaVersion));
- }
+ auto baklavaVersion = doc->Clone();
+ baklavaVersion->file.config.sdkVersion = SDK_BAKLAVA;
+ docs.push_back(std::move(baklavaVersion));
}
return docs;
}
diff --git a/tools/aapt2/link/FlaggedXmlVersioner_test.cpp b/tools/aapt2/link/FlaggedXmlVersioner_test.cpp
index 0c1314f..0dc4642 100644
--- a/tools/aapt2/link/FlaggedXmlVersioner_test.cpp
+++ b/tools/aapt2/link/FlaggedXmlVersioner_test.cpp
@@ -101,6 +101,7 @@
<TextView android:featureFlag="package.flag" /><TextView /><TextView />
</LinearLayout>)");
doc->file.config.sdkVersion = SDK_GINGERBREAD;
+ doc->file.uses_readwrite_feature_flags = true;
FlaggedXmlVersioner versioner;
auto results = versioner.Process(context_.get(), doc.get());
@@ -131,6 +132,7 @@
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android">
<TextView android:featureFlag="package.flag" /><TextView /><TextView />
</LinearLayout>)");
+ doc->file.uses_readwrite_feature_flags = true;
FlaggedXmlVersioner versioner;
auto results = versioner.Process(context_.get(), doc.get());
@@ -162,6 +164,7 @@
<TextView android:featureFlag="!package.flag" /><TextView /><TextView />
</LinearLayout>)");
doc->file.config.sdkVersion = SDK_GINGERBREAD;
+ doc->file.uses_readwrite_feature_flags = true;
FlaggedXmlVersioner versioner;
auto results = versioner.Process(context_.get(), doc.get());
@@ -192,6 +195,7 @@
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android">
<TextView android:featureFlag="!package.flag" /><TextView /><TextView />
</LinearLayout>)");
+ doc->file.uses_readwrite_feature_flags = true;
FlaggedXmlVersioner versioner;
auto results = versioner.Process(context_.get(), doc.get());
diff --git a/tools/aapt2/link/TableMerger.cpp b/tools/aapt2/link/TableMerger.cpp
index 1d4adc4..17f3323 100644
--- a/tools/aapt2/link/TableMerger.cpp
+++ b/tools/aapt2/link/TableMerger.cpp
@@ -295,6 +295,8 @@
dst_config_value =
dst_entry->FindOrCreateValue(src_config_value->config, src_config_value->product);
}
+ dst_config_value->uses_readwrite_feature_flags |=
+ src_config_value->uses_readwrite_feature_flags;
// Continue if we're taking the new resource.
CloningValueTransformer cloner(&main_table_->string_pool);
@@ -378,12 +380,13 @@
file_ref->file = file;
file_ref->SetFlagStatus(file_desc.flag_status);
file_ref->SetFlag(file_desc.flag);
-
ResourceTablePackage* pkg = table.FindOrCreatePackage(file_desc.name.package);
- pkg->FindOrCreateType(file_desc.name.type)
- ->FindOrCreateEntry(file_desc.name.entry)
- ->FindOrCreateValue(file_desc.config, {})
- ->value = std::move(file_ref);
+ ResourceConfigValue* config_value = pkg->FindOrCreateType(file_desc.name.type)
+ ->FindOrCreateEntry(file_desc.name.entry)
+ ->FindOrCreateValue(file_desc.config, {});
+
+ config_value->value = std::move(file_ref);
+ config_value->uses_readwrite_feature_flags = file_desc.uses_readwrite_feature_flags;
return DoMerge(file->GetSource(), pkg, false /*mangle*/, overlay /*overlay*/, true /*allow_new*/);
}