Template invocation fixes in GN

This adds an error check when invoking templates which caused really confusing messages if the template invocation encountered an error (because we'd continue running).

Hooks up the provider for programatically defined variables in template invocations so those can be used.

Sets the current directory in a template invocation to be that of the invoking file.

No longer define the target-related programatic variables in an import. Using these in an import will give the directory relative to the import, which is probabyl not what you want.

Fix the Windows build by adding a missing library. Add a warning not to add more to the main list (this added .lib is pretty obscure).

BUG=
R=cjhopman@chromium.org

Review URL: https://codereview.chromium.org/226223006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@262524 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 44 insertions, 21 deletions

diff --git a/tools/gn/import_manager.cc b/tools/gn/import_manager.cc
index 2e8293e..4bb4b63 100644
--- a/tools/gn/import_manager.cc
+++ b/tools/gn/import_manager.cc
@@ -25,8 +25,13 @@ Scope* UncachedImport(const Settings* settings,
   CHECK(block);
 
   scoped_ptr<Scope> scope(new Scope(settings->base_config()));
-  ScopePerFileProvider per_file_provider(scope.get());
   scope->set_source_dir(file.GetDir());
+
+  // Don't allow ScopePerFileProvider to provide target-related variables.
+  // These will be relative to the imported file, which is probably not what
+  // people mean when they use these.
+  ScopePerFileProvider per_file_provider(scope.get(), false);
+
   scope->SetProcessingImport();
   block->ExecuteBlockInScope(scope.get(), err);
   if (err->has_error())
diff --git a/tools/gn/loader.cc b/tools/gn/loader.cc
index 1384b92..2893e23 100644
--- a/tools/gn/loader.cc
+++ b/tools/gn/loader.cc
@@ -244,7 +244,7 @@ void LoaderImpl::BackgroundLoadFile(const Settings* settings,
   }
 
   Scope our_scope(settings->base_config());
-  ScopePerFileProvider per_file_provider(&our_scope);
+  ScopePerFileProvider per_file_provider(&our_scope, true);
   our_scope.set_source_dir(file_name.GetDir());
 
   ScopedTrace trace(TraceItem::TRACE_FILE_EXECUTE, file_name.value());
diff --git a/tools/gn/scope_per_file_provider.cc b/tools/gn/scope_per_file_provider.cc
index c6a0cd7..64b9a83 100644
--- a/tools/gn/scope_per_file_provider.cc
+++ b/tools/gn/scope_per_file_provider.cc
@@ -10,8 +10,10 @@
 #include "tools/gn/value.h"
 #include "tools/gn/variables.h"
 
-ScopePerFileProvider::ScopePerFileProvider(Scope* scope)
-    : ProgrammaticProvider(scope) {
+ScopePerFileProvider::ScopePerFileProvider(Scope* scope,
+                                           bool allow_target_vars)
+    : ProgrammaticProvider(scope),
+      allow_target_vars_(allow_target_vars) {
 }
 
 ScopePerFileProvider::~ScopePerFileProvider() {
@@ -32,10 +34,13 @@ const Value* ScopePerFileProvider::GetProgrammaticValue(
     return GetRootGenDir();
   if (ident == variables::kRootOutDir)
     return GetRootOutDir();
-  if (ident == variables::kTargetGenDir)
-    return GetTargetGenDir();
-  if (ident == variables::kTargetOutDir)
-    return GetTargetOutDir();
+
+  if (allow_target_vars_) {
+    if (ident == variables::kTargetGenDir)
+      return GetTargetGenDir();
+    if (ident == variables::kTargetOutDir)
+      return GetTargetOutDir();
+  }
   return NULL;
 }
 
diff --git a/tools/gn/scope_per_file_provider.h b/tools/gn/scope_per_file_provider.h
index 1021c27..2ef84cc 100644
--- a/tools/gn/scope_per_file_provider.h
+++ b/tools/gn/scope_per_file_provider.h
@@ -14,7 +14,10 @@
 // variable support.
 class ScopePerFileProvider : public Scope::ProgrammaticProvider {
  public:
-  ScopePerFileProvider(Scope* scope);
+  // allow_target_vars allows the target-related variables to get resolved.
+  // When allow_target_vars is unset, the target-related values will be
+  // undefined to GN script.
+  ScopePerFileProvider(Scope* scope, bool allow_target_vars);
   virtual ~ScopePerFileProvider();
 
   // ProgrammaticProvider implementation.
@@ -31,6 +34,8 @@ class ScopePerFileProvider : public Scope::ProgrammaticProvider {
   const Value* GetTargetGenDir();
   const Value* GetTargetOutDir();
 
+  bool allow_target_vars_;
+
   // All values are lazily created.
   scoped_ptr<Value> current_toolchain_;
   scoped_ptr<Value> default_toolchain_;
diff --git a/tools/gn/scope_per_file_provider_unittest.cc b/tools/gn/scope_per_file_provider_unittest.cc
index 776a0aa..8d01fb0 100644
--- a/tools/gn/scope_per_file_provider_unittest.cc
+++ b/tools/gn/scope_per_file_provider_unittest.cc
@@ -20,7 +20,7 @@ TEST(ScopePerFileProvider, Expected) {
   {
     Scope scope(test.settings());
     scope.set_source_dir(SourceDir("//source/"));
-    ScopePerFileProvider provider(&scope);
+    ScopePerFileProvider provider(&scope, true);
 
     EXPECT_EQ("//toolchain:default",    GPV(variables::kCurrentToolchain));
     // TODO(brettw) this test harness does not set up the Toolchain manager
@@ -41,7 +41,7 @@ TEST(ScopePerFileProvider, Expected) {
 
     Scope scope(&settings);
     scope.set_source_dir(SourceDir("//source/"));
-    ScopePerFileProvider provider(&scope);
+    ScopePerFileProvider provider(&scope, true);
 
     EXPECT_EQ("//toolchain:tc",            GPV(variables::kCurrentToolchain));
     // See above.
diff --git a/tools/gn/template.cc b/tools/gn/template.cc
index fa0ccf3..bf4eb61 100644
--- a/tools/gn/template.cc
+++ b/tools/gn/template.cc
@@ -8,6 +8,7 @@
 #include "tools/gn/functions.h"
 #include "tools/gn/parse_tree.h"
 #include "tools/gn/scope.h"
+#include "tools/gn/scope_per_file_provider.h"
 #include "tools/gn/value.h"
 
 Template::Template(const Scope* scope, const FunctionCallNode* def)
@@ -50,17 +51,24 @@ Value Template::Invoke(Scope* scope,
   if (err->has_error())
     return Value();
 
-  // Set up the scope to run the template. This should be dependent on the
-  // closure, but have the "invoker" and "target_name" values injected, and the
-  // current dir matching the invoker. We jump through some hoops to avoid
-  // copying the invocation scope when setting it in the template scope (since
-  // the invocation scope may have large lists of source files in it and could
-  // be expensive to copy).
+  // Set up the scope to run the template and set the current directory for the
+  // template (which ScopePerFileProvider uses to base the target-related
+  // variables target_gen_dir and target_out_dir on) to be that of the invoker.
+  // This way, files don't have to be rebased and target_*_dir works the way
+  // people expect (otherwise its to easy to be putting generated files in the
+  // gen dir corresponding to an imported file).
+  Scope template_scope(closure_.get());
+  template_scope.set_source_dir(scope->GetSourceDir());
+
+  ScopePerFileProvider per_file_provider(&template_scope, true);
+
+  // We jump through some hoops to avoid copying the invocation scope when
+  // setting it in the template scope (since the invocation scope may have
+  // large lists of source files in it and could be expensive to copy).
   //
   // Scope.SetValue will copy the value which will in turn copy the scope, but
   // if we instead create a value and then set the scope on it, the copy can
   // be avoided.
-  Scope template_scope(closure_.get());
   const char kInvoker[] = "invoker";
   template_scope.SetValue(kInvoker, Value(NULL, scoped_ptr<Scope>()),
                           invocation);
@@ -73,11 +81,11 @@ Value Template::Invoke(Scope* scope,
                           Value(invocation, args[0].string_value()),
                           invocation);
 
-  // Run the template code. Don't check for unused variables since the
-  // template could be executed in many different ways and it could be that
-  // not all executions use all values in the closure.
+  // Actually run the template code.
   Value result =
       definition_->block()->ExecuteBlockInScope(&template_scope, err);
+  if (err->has_error())
+    return Value();
 
   // Check for unused variables in the invocation scope. This will find typos
   // of things the caller meant to pass to the template but the template didn't