Merge changes from topic "aapt2-warn-manifest-validation"

am: 61f92e7e13

Change-Id: Idb04f6750abd4dbdce62979794f68833a39e4c3b

tools/aapt2/cmd/Link.cpp

@@ -1968,6 +1968,9 @@ int Link(const std::vector<StringPiece>& args, IDiagnostics* diagnostics) {
                         &options.manifest_fixer_options.rename_instrumentation_target_package)
           .OptionalFlagList("-0", "File extensions not to compress.",
                             &options.extensions_to_not_compress)
+          .OptionalSwitch("--warn-manifest-validation",
+                          "Treat manifest validation errors as warnings.",
+                          &options.manifest_fixer_options.warn_validation)
           .OptionalFlagList("--split",
                             "Split resources matching a set of configs out to a Split APK.\n"
                             "Syntax: path/to/output.apk:<config>[,<config>[...]].\n"

tools/aapt2/link/ManifestFixer.cpp

@@ -409,7 +409,10 @@ bool ManifestFixer::Consume(IAaptContext* context, xml::XmlResource* doc) {
     return false;
   }
 
-  if (!executor.Execute(xml::XmlActionExecutorPolicy::kWhitelist, context->GetDiagnostics(), doc)) {
+  xml::XmlActionExecutorPolicy policy = options_.warn_validation
+                                            ? xml::XmlActionExecutorPolicy::kWhitelistWarning
+                                            : xml::XmlActionExecutorPolicy::kWhitelist;
+  if (!executor.Execute(policy, context->GetDiagnostics(), doc)) {
     return false;
   }
 

tools/aapt2/link/ManifestFixer.h

@@ -35,6 +35,11 @@ struct ManifestFixerOptions {
   Maybe<std::string> rename_instrumentation_target_package;
   Maybe<std::string> version_name_default;
   Maybe<std::string> version_code_default;
+
+  // Wether validation errors should be treated only as warnings. If this is 'true', then an
+  // incorrect node will not result in an error, but only as a warning, and the parsing will
+  // continue.
+  bool warn_validation = false;
 };
 
 /**

tools/aapt2/link/ManifestFixer_test.cpp

@@ -19,6 +19,7 @@
 #include "test/Test.h"
 
 using ::android::StringPiece;
+using ::testing::IsNull;
 using ::testing::NotNull;
 
 namespace aapt {
@@ -109,7 +110,9 @@ TEST_F(ManifestFixerTest, AllowMetaData) {
 }
 
 TEST_F(ManifestFixerTest, UseDefaultSdkVersionsIfNonePresent) {
-  ManifestFixerOptions options = {std::string("8"), std::string("22")};
+  ManifestFixerOptions options;
+  options.min_sdk_version_default = std::string("8");
+  options.target_sdk_version_default = std::string("22");
 
   std::unique_ptr<xml::XmlResource> doc = VerifyWithOptions(R"EOF(
       <manifest xmlns:android="http://schemas.android.com/apk/res/android"
@@ -190,7 +193,9 @@ TEST_F(ManifestFixerTest, UseDefaultSdkVersionsIfNonePresent) {
 }
 
 TEST_F(ManifestFixerTest, UsesSdkMustComeBeforeApplication) {
-  ManifestFixerOptions options = {std::string("8"), std::string("22")};
+  ManifestFixerOptions options;
+  options.min_sdk_version_default = std::string("8");
+  options.target_sdk_version_default = std::string("22");
   std::unique_ptr<xml::XmlResource> doc = VerifyWithOptions(R"EOF(
           <manifest xmlns:android="http://schemas.android.com/apk/res/android"
                     package="android">
@@ -439,4 +444,27 @@ TEST_F(ManifestFixerTest, SupportKeySets) {
   EXPECT_THAT(Verify(input), NotNull());
 }
 
+TEST_F(ManifestFixerTest, UnexpectedElementsInManifest) {
+  std::string input = R"(
+      <manifest xmlns:android="http://schemas.android.com/apk/res/android"
+          package="android">
+        <beep/>
+      </manifest>)";
+  ManifestFixerOptions options;
+  options.warn_validation = true;
+
+  // Unexpected element should result in a warning if the flag is set to 'true'.
+  std::unique_ptr<xml::XmlResource> manifest = VerifyWithOptions(input, options);
+  ASSERT_THAT(manifest, NotNull());
+
+  // Unexpected element should result in an error if the flag is set to 'false'.
+  options.warn_validation = false;
+  manifest = VerifyWithOptions(input, options);
+  ASSERT_THAT(manifest, IsNull());
+
+  // By default the flag should be set to 'false'.
+  manifest = Verify(input);
+  ASSERT_THAT(manifest, IsNull());
+}
+
 }  // namespace aapt

tools/aapt2/xml/XmlActionExecutor.cpp

@@ -66,7 +66,7 @@ bool XmlNodeAction::Execute(XmlActionExecutorPolicy policy, std::vector<StringPi
         continue;
       }
 
-      if (policy == XmlActionExecutorPolicy::kWhitelist) {
+      if (policy != XmlActionExecutorPolicy::kNone) {
         DiagMessage error_msg(child_el->line_number);
         error_msg << "unexpected element ";
         PrintElementToDiagMessage(child_el, &error_msg);
@@ -74,8 +74,14 @@ bool XmlNodeAction::Execute(XmlActionExecutorPolicy policy, std::vector<StringPi
         for (const StringPiece& element : *bread_crumb) {
           error_msg << "<" << element << ">";
         }
-        diag->Error(error_msg);
+        if (policy == XmlActionExecutorPolicy::kWhitelistWarning) {
-        error = true;
+          // Treat the error only as a warning.
+          diag->Warn(error_msg);
+        } else {
+          // Policy is XmlActionExecutorPolicy::kWhitelist, we should fail.
+          diag->Error(error_msg);
+          error = true;
+        }
       }
     }
   }

tools/aapt2/xml/XmlActionExecutor.h

@@ -34,10 +34,15 @@ enum class XmlActionExecutorPolicy {
   // Actions are run if elements are matched, errors occur only when actions return false.
   kNone,
 
-  // The actions defined must match and run. If an element is found that does
+  // The actions defined must match and run. If an element is found that does not match an action,
-  // not match an action, an error occurs.
+  // an error occurs.
   // Note: namespaced elements are always ignored.
   kWhitelist,
+
+  // The actions defined should match and run. if an element is found that does not match an
+  // action, a warning is printed.
+  // Note: namespaced elements are always ignored.
+  kWhitelistWarning,
 };
 
 // Contains the actions to perform at this XML node. This is a recursive data structure that