Make all bundle_foo_dir properties optional
The bundle_foo_dir properties are only required if they are used by
a bundle_data expansion ({{bundle_foo_dir}}). Do not report an error
if they are not defined unless there is an expansion using them.
Bug: https://bugs.chromium.org/p/gn/issues/detail?id=64
Change-Id: I277fdfc209fb110352f4614addba7f35f127db51
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/4482
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
diff --git a/docs/reference.md b/docs/reference.md
index 8019687..9571fb3 100644
--- a/docs/reference.md
+++ b/docs/reference.md
@@ -685,6 +685,7 @@
"vs2013" - Visual Studio 2013 project/solution files.
"vs2015" - Visual Studio 2015 project/solution files.
"vs2017" - Visual Studio 2017 project/solution files.
+ "vs2019" - Visual Studio 2019 project/solution files.
"xcode" - Xcode workspace/solution files.
"qtcreator" - QtCreator project files.
"json" - JSON file containing target information
@@ -1385,8 +1386,9 @@
are computed from all "bundle_data" target this one depends on transitively
(the recursion stops at "create_bundle" targets).
- The "bundle_*_dir" properties must be defined. They will be used for the
- expansion of {{bundle_*_dir}} rules in "bundle_data" outputs.
+ The "bundle_*_dir" are be used for the expansion of {{bundle_*_dir}} rules in
+ "bundle_data" outputs. The properties are optional but must be defined if any
+ of the "bundle_data" target use them.
This target can be used on all platforms though it is designed only to
generate iOS or macOS bundle. In cross-platform projects, it is advised to put
@@ -1420,13 +1422,12 @@
#### **Variables**
```
- bundle_root_dir*, bundle_contents_dir*, bundle_resources_dir*,
- bundle_executable_dir*, bundle_plugins_dir*, bundle_deps_filter, deps,
+ bundle_root_dir, bundle_contents_dir, bundle_resources_dir,
+ bundle_executable_dir, bundle_plugins_dir, bundle_deps_filter, deps,
data_deps, public_deps, visibility, product_type, code_signing_args,
code_signing_script, code_signing_sources, code_signing_outputs,
xcode_extra_attributes, xcode_test_application_name, partial_info_plist,
metadata
- * = required
```
#### **Example**
@@ -4287,9 +4288,12 @@
]
}
```
-### <a name="var_bundle_executable_dir"></a>**bundle_executable_dir**: Expansion of {{bundle_executable_dir}} in create_bundle.
+### <a name="var_bundle_executable_dir"></a>**bundle_executable_dir**
```
+ bundle_executable_dir: Expansion of {{bundle_executable_dir}} in
+ create_bundle.
+
A string corresponding to a path in $root_build_dir.
This string is used by the "create_bundle" target to expand the
@@ -4312,7 +4316,8 @@
### <a name="var_bundle_resources_dir"></a>**bundle_resources_dir**
```
- bundle_resources_dir: Expansion of {{bundle_resources_dir}} in create_bundle.
+ bundle_resources_dir: Expansion of {{bundle_resources_dir}} in
+ create_bundle.
A string corresponding to a path in $root_build_dir.
diff --git a/tools/gn/bundle_data.cc b/tools/gn/bundle_data.cc
index 9f2cf2b..095e989 100644
--- a/tools/gn/bundle_data.cc
+++ b/tools/gn/bundle_data.cc
@@ -112,20 +112,29 @@
}
}
-void BundleData::GetOutputFiles(const Settings* settings,
- OutputFiles* outputs) const {
+bool BundleData::GetOutputFiles(const Settings* settings,
+ const Target* target,
+ OutputFiles* outputs,
+ Err* err) const {
SourceFiles outputs_as_sources;
- GetOutputsAsSourceFiles(settings, &outputs_as_sources);
+ if (!GetOutputsAsSourceFiles(settings, target, &outputs_as_sources, err))
+ return false;
for (const SourceFile& source_file : outputs_as_sources)
outputs->push_back(OutputFile(settings->build_settings(), source_file));
+ return true;
}
-void BundleData::GetOutputsAsSourceFiles(const Settings* settings,
- SourceFiles* outputs_as_source) const {
+bool BundleData::GetOutputsAsSourceFiles(const Settings* settings,
+ const Target* target,
+ SourceFiles* outputs_as_source,
+ Err* err) const {
for (const BundleFileRule& file_rule : file_rules_) {
for (const SourceFile& source : file_rule.sources()) {
- outputs_as_source->push_back(
- file_rule.ApplyPatternToSource(settings, *this, source));
+ SourceFile expanded_source_file;
+ if (!file_rule.ApplyPatternToSource(settings, target, *this, source,
+ &expanded_source_file, err))
+ return false;
+ outputs_as_source->push_back(expanded_source_file);
}
}
@@ -146,6 +155,8 @@
if (!root_dir_.is_null())
outputs_as_source->push_back(GetBundleRootDirOutput(settings));
+
+ return true;
}
SourceFile BundleData::GetCompiledAssetCatalogPath() const {
diff --git a/tools/gn/bundle_data.h b/tools/gn/bundle_data.h
index dc63bbd..60254a9 100644
--- a/tools/gn/bundle_data.h
+++ b/tools/gn/bundle_data.h
@@ -44,11 +44,16 @@
void GetSourceFiles(SourceFiles* sources) const;
// Returns the list of outputs.
- void GetOutputFiles(const Settings* settings, OutputFiles* outputs) const;
+ bool GetOutputFiles(const Settings* settings,
+ const Target* target,
+ OutputFiles* outputs,
+ Err* err) const;
// Returns the list of outputs as SourceFile.
- void GetOutputsAsSourceFiles(const Settings* settings,
- SourceFiles* outputs_as_source) const;
+ bool GetOutputsAsSourceFiles(const Settings* settings,
+ const Target* target,
+ SourceFiles* outputs_as_source,
+ Err* err) const;
// Returns the path to the compiled asset catalog. Only valid if
// assets_catalog_sources() is not empty.
diff --git a/tools/gn/bundle_file_rule.cc b/tools/gn/bundle_file_rule.cc
index 4d4ca3a..78012a0 100644
--- a/tools/gn/bundle_file_rule.cc
+++ b/tools/gn/bundle_file_rule.cc
@@ -4,11 +4,32 @@
#include "tools/gn/bundle_file_rule.h"
+#include "base/strings/stringprintf.h"
#include "tools/gn/output_file.h"
#include "tools/gn/settings.h"
#include "tools/gn/substitution_pattern.h"
#include "tools/gn/substitution_writer.h"
#include "tools/gn/target.h"
+#include "tools/gn/variables.h"
+
+namespace {
+
+Err ErrMissingPropertyForExpansion(const Settings* settings,
+ const Target* target,
+ const BundleFileRule* bundle_file_rule,
+ const char* property_name) {
+ std::string label = bundle_file_rule->target()->label().GetUserVisibleName(
+ settings->default_toolchain_label());
+
+ return Err(target->defined_from(),
+ base::StringPrintf("Property %s is required.", property_name),
+ base::StringPrintf(
+ "In order to expand {{%s}} in %s, the "
+ "property needs to be defined in the create_bundle target.",
+ property_name, label.c_str()));
+}
+
+} // namespace
BundleFileRule::BundleFileRule(const Target* bundle_data_target,
const std::vector<SourceFile> sources,
@@ -22,10 +43,12 @@
BundleFileRule::~BundleFileRule() = default;
-SourceFile BundleFileRule::ApplyPatternToSource(
- const Settings* settings,
- const BundleData& bundle_data,
- const SourceFile& source_file) const {
+bool BundleFileRule::ApplyPatternToSource(const Settings* settings,
+ const Target* target,
+ const BundleData& bundle_data,
+ const SourceFile& source_file,
+ SourceFile* expanded_source_file,
+ Err* err) const {
std::string output_path;
for (const auto& subrange : pattern_.ranges()) {
switch (subrange.type) {
@@ -33,18 +56,43 @@
output_path.append(subrange.literal);
break;
case SUBSTITUTION_BUNDLE_ROOT_DIR:
+ if (bundle_data.contents_dir().is_null()) {
+ *err = ErrMissingPropertyForExpansion(settings, target, this,
+ variables::kBundleRootDir);
+ return false;
+ }
output_path.append(bundle_data.root_dir().value());
break;
case SUBSTITUTION_BUNDLE_CONTENTS_DIR:
+ if (bundle_data.contents_dir().is_null()) {
+ *err = ErrMissingPropertyForExpansion(settings, target, this,
+ variables::kBundleContentsDir);
+ return false;
+ }
output_path.append(bundle_data.contents_dir().value());
break;
case SUBSTITUTION_BUNDLE_RESOURCES_DIR:
+ if (bundle_data.resources_dir().is_null()) {
+ *err = ErrMissingPropertyForExpansion(settings, target, this,
+ variables::kBundleResourcesDir);
+ return false;
+ }
output_path.append(bundle_data.resources_dir().value());
break;
case SUBSTITUTION_BUNDLE_EXECUTABLE_DIR:
+ if (bundle_data.executable_dir().is_null()) {
+ *err = ErrMissingPropertyForExpansion(
+ settings, target, this, variables::kBundleExecutableDir);
+ return false;
+ }
output_path.append(bundle_data.executable_dir().value());
break;
case SUBSTITUTION_BUNDLE_PLUGINS_DIR:
+ if (bundle_data.contents_dir().is_null()) {
+ *err = ErrMissingPropertyForExpansion(settings, target, this,
+ variables::kBundlePlugInsDir);
+ return false;
+ }
output_path.append(bundle_data.plugins_dir().value());
break;
default:
@@ -54,13 +102,24 @@
break;
}
}
- return SourceFile(SourceFile::SWAP_IN, &output_path);
+ *expanded_source_file = SourceFile(SourceFile::SWAP_IN, &output_path);
+ return true;
}
-OutputFile BundleFileRule::ApplyPatternToSourceAsOutputFile(
+bool BundleFileRule::ApplyPatternToSourceAsOutputFile(
const Settings* settings,
+ const Target* target,
const BundleData& bundle_data,
- const SourceFile& source_file) const {
- return OutputFile(settings->build_settings(),
- ApplyPatternToSource(settings, bundle_data, source_file));
+ const SourceFile& source_file,
+ OutputFile* expanded_output_file,
+ Err* err) const {
+ SourceFile expanded_source_file;
+ if (!ApplyPatternToSource(settings, target, bundle_data, source_file,
+ &expanded_source_file, err)) {
+ return false;
+ }
+
+ *expanded_output_file =
+ OutputFile(settings->build_settings(), expanded_source_file);
+ return true;
}
diff --git a/tools/gn/bundle_file_rule.h b/tools/gn/bundle_file_rule.h
index 372e628..b7a2428 100644
--- a/tools/gn/bundle_file_rule.h
+++ b/tools/gn/bundle_file_rule.h
@@ -27,13 +27,18 @@
// Applies the substitution pattern to a source file, returning the result
// as either a SourceFile or an OutputFile.
- SourceFile ApplyPatternToSource(const Settings* settings,
- const BundleData& bundle_data,
- const SourceFile& source_file) const;
- OutputFile ApplyPatternToSourceAsOutputFile(
- const Settings* settings,
- const BundleData& bundle_data,
- const SourceFile& source_file) const;
+ bool ApplyPatternToSource(const Settings* settings,
+ const Target* target,
+ const BundleData& bundle_data,
+ const SourceFile& source_file,
+ SourceFile* expanded_source_file,
+ Err* err) const;
+ bool ApplyPatternToSourceAsOutputFile(const Settings* settings,
+ const Target* target,
+ const BundleData& bundle_data,
+ const SourceFile& source_file,
+ OutputFile* expanded_output_file,
+ Err* err) const;
// Returns the associated target (of type Target::BUNDLE_DATA). May be
// null during testing.
diff --git a/tools/gn/create_bundle_target_generator.cc b/tools/gn/create_bundle_target_generator.cc
index 79edd78..768697d 100644
--- a/tools/gn/create_bundle_target_generator.cc
+++ b/tools/gn/create_bundle_target_generator.cc
@@ -78,6 +78,8 @@
const SourceDir& bundle_root_dir,
const base::StringPiece& name,
SourceDir* bundle_dir) {
+ // All bundle_foo_dir properties are optional. They are only required if they
+ // are used in an expansion. The check is performed there.
const Value* value = scope_->GetValue(name, true);
if (!value)
return true;
diff --git a/tools/gn/desc_builder.cc b/tools/gn/desc_builder.cc
index e377f3e..d029843 100644
--- a/tools/gn/desc_builder.cc
+++ b/tools/gn/desc_builder.cc
@@ -685,9 +685,12 @@
res->SetWithoutPathExpansion(variables::kOutputs, std::move(list));
} else if (target_->output_type() == Target::CREATE_BUNDLE ||
target_->output_type() == Target::GENERATED_FILE) {
+ Err err;
std::vector<SourceFile> output_files;
- target_->bundle_data().GetOutputsAsSourceFiles(target_->settings(),
- &output_files);
+ if (!target_->bundle_data().GetOutputsAsSourceFiles(
+ target_->settings(), target_, &output_files, &err)) {
+ err.PrintToStdout();
+ }
res->SetWithoutPathExpansion(variables::kOutputs,
RenderValue(output_files));
} else if (target_->output_type() == Target::ACTION_FOREACH ||
diff --git a/tools/gn/functions_target.cc b/tools/gn/functions_target.cc
index 24fc248..72ccf93 100644
--- a/tools/gn/functions_target.cc
+++ b/tools/gn/functions_target.cc
@@ -327,8 +327,9 @@
are computed from all "bundle_data" target this one depends on transitively
(the recursion stops at "create_bundle" targets).
- The "bundle_*_dir" properties must be defined. They will be used for the
- expansion of {{bundle_*_dir}} rules in "bundle_data" outputs.
+ The "bundle_*_dir" are be used for the expansion of {{bundle_*_dir}} rules in
+ "bundle_data" outputs. The properties are optional but must be defined if any
+ of the "bundle_data" target use them.
This target can be used on all platforms though it is designed only to
generate iOS or macOS bundle. In cross-platform projects, it is advised to put
@@ -358,13 +359,12 @@
Variables
- bundle_root_dir*, bundle_contents_dir*, bundle_resources_dir*,
- bundle_executable_dir*, bundle_plugins_dir*, bundle_deps_filter, deps,
+ bundle_root_dir, bundle_contents_dir, bundle_resources_dir,
+ bundle_executable_dir, bundle_plugins_dir, bundle_deps_filter, deps,
data_deps, public_deps, visibility, product_type, code_signing_args,
code_signing_script, code_signing_sources, code_signing_outputs,
xcode_extra_attributes, xcode_test_application_name, partial_info_plist,
metadata
- * = required
Example
diff --git a/tools/gn/ninja_create_bundle_target_writer.cc b/tools/gn/ninja_create_bundle_target_writer.cc
index 9834aba..cc816e1 100644
--- a/tools/gn/ninja_create_bundle_target_writer.cc
+++ b/tools/gn/ninja_create_bundle_target_writer.cc
@@ -135,12 +135,18 @@
// steps as this is most likely implemented using hardlink in the common case.
// See NinjaCopyTargetWriter::WriteCopyRules() for a detailed explanation.
for (const SourceFile& source_file : file_rule.sources()) {
- OutputFile output_file = file_rule.ApplyPatternToSourceAsOutputFile(
- settings_, target_->bundle_data(), source_file);
- output_files->push_back(output_file);
+ // There is no need to check for errors here as the substitution will have
+ // been performed when computing the list of output of the target during
+ // the Target::OnResolved phase earlier.
+ OutputFile expanded_output_file;
+ file_rule.ApplyPatternToSourceAsOutputFile(
+ settings_, target_, target_->bundle_data(), source_file,
+ &expanded_output_file,
+ /*err=*/nullptr);
+ output_files->push_back(expanded_output_file);
out_ << "build ";
- path_output_.WriteFile(out_, output_file);
+ path_output_.WriteFile(out_, expanded_output_file);
out_ << ": " << GetNinjaRulePrefixForToolchain(settings_)
<< Toolchain::ToolTypeToName(Toolchain::TYPE_COPY_BUNDLE_DATA) << " ";
path_output_.WriteFile(out_, source_file);
diff --git a/tools/gn/target.cc b/tools/gn/target.cc
index c61758b..94fb994 100644
--- a/tools/gn/target.cc
+++ b/tools/gn/target.cc
@@ -381,7 +381,8 @@
if (!ResolvePrecompiledHeaders(err))
return false;
- FillOutputFiles();
+ if (!FillOutputFiles(err))
+ return false;
if (!CheckVisibility(err))
return false;
@@ -629,7 +630,7 @@
bundle_data_.OnTargetResolved(this);
}
-void Target::FillOutputFiles() {
+bool Target::FillOutputFiles(Err* err) {
const Tool* tool = toolchain_->GetToolForTargetFinalOutput(this);
bool check_tool_outputs = false;
switch (output_type_) {
@@ -712,8 +713,10 @@
}
// Count anything generated from bundle_data dependencies.
- if (output_type_ == CREATE_BUNDLE)
- bundle_data_.GetOutputFiles(settings(), &computed_outputs_);
+ if (output_type_ == CREATE_BUNDLE) {
+ if (!bundle_data_.GetOutputFiles(settings(), this, &computed_outputs_, err))
+ return false;
+ }
// Count all outputs from this tool as something generated by this target.
if (check_tool_outputs) {
@@ -734,6 +737,8 @@
action_values_.GetOutputsAsSourceFiles(this, &outputs_as_sources);
for (const SourceFile& out : outputs_as_sources)
computed_outputs_.push_back(OutputFile(settings()->build_settings(), out));
+
+ return true;
}
bool Target::ResolvePrecompiledHeaders(Err* err) {
diff --git a/tools/gn/target.h b/tools/gn/target.h
index abc357e..b73ae57 100644
--- a/tools/gn/target.h
+++ b/tools/gn/target.h
@@ -346,7 +346,7 @@
void PullRecursiveBundleData();
// Fills the link and dependency output files when a target is resolved.
- void FillOutputFiles();
+ bool FillOutputFiles(Err* err);
// Checks precompiled headers from configs and makes sure the resulting
// values are in config_values_.
diff --git a/tools/gn/target_unittest.cc b/tools/gn/target_unittest.cc
index 9d9173e..b2d70fe 100644
--- a/tools/gn/target_unittest.cc
+++ b/tools/gn/target_unittest.cc
@@ -1044,12 +1044,18 @@
e.public_deps().push_back(LabelTargetPair(&f));
e.public_deps().push_back(LabelTargetPair(&b));
+ a.bundle_data().root_dir() = SourceDir("//out/foo_a.bundle");
+ a.bundle_data().resources_dir() = SourceDir("//out/foo_a.bundle/Resources");
+
b.sources().push_back(SourceFile("//foo/b1.txt"));
b.sources().push_back(SourceFile("//foo/b2.txt"));
b.action_values().outputs() = SubstitutionList::MakeForTest(
"{{bundle_resources_dir}}/{{source_file_part}}");
ASSERT_TRUE(b.OnResolved(&err));
+ c.bundle_data().root_dir() = SourceDir("//out/foo_e.bundle");
+ c.bundle_data().resources_dir() = SourceDir("//out/foo_e.bundle/Resources");
+
d.sources().push_back(SourceFile("//foo/d.txt"));
d.action_values().outputs() = SubstitutionList::MakeForTest(
"{{bundle_resources_dir}}/{{source_file_part}}");
