From 979ccb2e6f3f1f7f00a448eb440a85daf033dc9e Mon Sep 17 00:00:00 2001 From: Adam Lesinski Date: Mon, 11 Jan 2016 10:42:19 -0800 Subject: [PATCH] AAPT2: Warn when positional arguments exist and --legacy is on This is normally an error, but old AAPT didn't check for it correctly, so many projects violate this. With --legacy, this becomes a warning. Change-Id: I23647e029930e11b719591cd38609e1b43247e20 --- tools/aapt2/ResourceParser.cpp | 13 +++++++--- tools/aapt2/ResourceParser.h | 5 ++++ tools/aapt2/compile/Compile.cpp | 4 +++ tools/aapt2/link/Link.cpp | 46 +++++++++++++++++++++++++-------- 4 files changed, 53 insertions(+), 15 deletions(-) diff --git a/tools/aapt2/ResourceParser.cpp b/tools/aapt2/ResourceParser.cpp index e1f9642d27d72..5e7d3ec4c1e09 100644 --- a/tools/aapt2/ResourceParser.cpp +++ b/tools/aapt2/ResourceParser.cpp @@ -582,10 +582,15 @@ bool ResourceParser::parseString(xml::XmlPullParser* parser, ParsedResource* out if (formatted && translateable) { if (!util::verifyJavaStringFormat(*stringValue->value)) { - mDiag->error(DiagMessage(outResource->source) - << "multiple substitutions specified in non-positional format; " - "did you mean to add the formatted=\"false\" attribute?"); - return false; + DiagMessage msg(outResource->source); + msg << "multiple substitutions specified in non-positional format; " + "did you mean to add the formatted=\"false\" attribute?"; + if (mOptions.errorOnPositionalArguments) { + mDiag->error(msg); + return false; + } + + mDiag->warn(msg); } } diff --git a/tools/aapt2/ResourceParser.h b/tools/aapt2/ResourceParser.h index 9ad749e27dbce..51cbbe19384e7 100644 --- a/tools/aapt2/ResourceParser.h +++ b/tools/aapt2/ResourceParser.h @@ -44,6 +44,11 @@ struct ResourceParserOptions { * Whether the default setting for this parser is to allow translation. */ bool translatable = true; + + /** + * Whether positional arguments in formatted strings are treated as errors or warnings. + */ + bool errorOnPositionalArguments = true; }; /* diff --git a/tools/aapt2/compile/Compile.cpp b/tools/aapt2/compile/Compile.cpp index b3b0f65e54da9..c78670fbc20d7 100644 --- a/tools/aapt2/compile/Compile.cpp +++ b/tools/aapt2/compile/Compile.cpp @@ -107,6 +107,7 @@ struct CompileOptions { Maybe resDir; std::vector products; bool pseudolocalize = false; + bool legacyMode = false; bool verbose = false; }; @@ -192,6 +193,7 @@ static bool compileTable(IAaptContext* context, const CompileOptions& options, ResourceParserOptions parserOptions; parserOptions.products = options.products; + parserOptions.errorOnPositionalArguments = !options.legacyMode; // If the filename includes donottranslate, then the default translatable is false. parserOptions.translatable = pathData.name.find(u"donottranslate") == std::string::npos; @@ -438,6 +440,8 @@ int compile(const std::vector& args) { .optionalFlag("--dir", "Directory to scan for resources", &options.resDir) .optionalSwitch("--pseudo-localize", "Generate resources for pseudo-locales " "(en-XA and ar-XB)", &options.pseudolocalize) + .optionalSwitch("--legacy", "Treat errors that used to be valid in AAPT as warnings", + &options.legacyMode) .optionalSwitch("-v", "Enables verbose logging", &options.verbose); if (!flags.parse("aapt2 compile", args, &std::cerr)) { return 1; diff --git a/tools/aapt2/link/Link.cpp b/tools/aapt2/link/Link.cpp index 652e52fa0a67a..8a87d9691b5d8 100644 --- a/tools/aapt2/link/Link.cpp +++ b/tools/aapt2/link/Link.cpp @@ -228,29 +228,53 @@ public: return {}; } + /** + * Precondition: ResourceTable doesn't have any IDs assigned yet, nor is it linked. + * Postcondition: ResourceTable has only one package left. All others are stripped, or there + * is an error and false is returned. + */ bool verifyNoExternalPackages() { + auto isExtPackageFunc = [&](const std::unique_ptr& pkg) -> bool { + return mContext.getCompilationPackage() != pkg->name || + !pkg->id || + pkg->id.value() != mContext.getPackageId(); + }; + bool error = false; for (const auto& package : mFinalTable.packages) { - if (mContext.getCompilationPackage() != package->name || - !package->id || package->id.value() != mContext.getPackageId()) { + if (isExtPackageFunc(package)) { // We have a package that is not related to the one we're building! for (const auto& type : package->types) { for (const auto& entry : type->entries) { + ResourceNameRef resName(package->name, type->type, entry->name); + for (const auto& configValue : entry->values) { - mContext.getDiagnostics()->error( - DiagMessage(configValue.value->getSource()) - << "defined resource '" - << ResourceNameRef(package->name, - type->type, - entry->name) - << "' for external package '" - << package->name << "'"); - error = true; + // Special case the occurrence of an ID that is being generated for the + // 'android' package. This is due to legacy reasons. + if (valueCast(configValue.value.get()) && + package->name == u"android") { + mContext.getDiagnostics()->warn( + DiagMessage(configValue.value->getSource()) + << "generated id '" << resName + << "' for external package '" << package->name + << "'"); + } else { + mContext.getDiagnostics()->error( + DiagMessage(configValue.value->getSource()) + << "defined resource '" << resName + << "' for external package '" << package->name + << "'"); + error = true; + } } } } } } + + auto newEndIter = std::remove_if(mFinalTable.packages.begin(), mFinalTable.packages.end(), + isExtPackageFunc); + mFinalTable.packages.erase(newEndIter, mFinalTable.packages.end()); return !error; }