Split the execution of the WORKSPACE file after each load statement

This is the main step toward supporting load statement of remote
repository in the WORKSPACE file (bug #824). Load statement that
refers to a remote repository will depend on the previous fragment
of the workspace file.

Issue #824 Step 3.

--
MOS_MIGRATED_REVID=114237027

4 files changed, 209 insertions, 49 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
index dcf364a3e1..90f4ea87c8 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
@@ -27,6 +27,7 @@ import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.Package.Builder;
 import com.google.devtools.build.lib.packages.Package.LegacyBuilder;
+import com.google.devtools.build.lib.packages.Package.NameConflictException;
 import com.google.devtools.build.lib.packages.PackageFactory.EnvironmentExtension;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature;
 import com.google.devtools.build.lib.syntax.BaseFunction;
@@ -35,6 +36,7 @@ import com.google.devtools.build.lib.syntax.BuiltinFunction;
 import com.google.devtools.build.lib.syntax.ClassObject;
 import com.google.devtools.build.lib.syntax.Environment;
 import com.google.devtools.build.lib.syntax.Environment.Extension;
+import com.google.devtools.build.lib.syntax.Environment.Frame;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.FuncallExpression;
 import com.google.devtools.build.lib.syntax.FunctionSignature;
@@ -60,6 +62,16 @@ public class WorkspaceFactory {
   public static final String BIND = "bind";
   private static final Pattern LEGAL_WORKSPACE_NAME = Pattern.compile("^\\p{Alpha}\\w*$");
 
+  // List of static function added by #addWorkspaceFunctions. Used to trim them out from the
+  // serialized list of variables bindings.
+  private static final ImmutableList<String> STATIC_WORKSPACE_FUNCTIONS =
+      ImmutableList.of(
+          "workspace",
+          "__embedded_dir__", // serializable so optional
+          "__workspace_dir__", // serializable so optional
+          "DEFAULT_SERVER_JAVABASE", // serializable so optional
+          PackageFactory.PKG_CONTEXT);
+
   private final LegacyBuilder builder;
   
   private final Path installDir;
@@ -69,6 +81,18 @@ public class WorkspaceFactory {
   private final ImmutableMap<String, BaseFunction> workspaceFunctions;
   private final ImmutableList<EnvironmentExtension> environmentExtensions;
 
+  // Values from the previous workspace file parts.
+  // List of load statements
+  private ImmutableMap<String, Extension> parentImportMap = ImmutableMap.of();
+  // List of top level variable bindings
+  private ImmutableMap<String, Object> parentVariableBindings = ImmutableMap.of();
+
+  // Values accumulated up to the currently parsed workspace file part.
+  // List of load statements
+  private ImmutableMap<String, Extension> importMap = ImmutableMap.of();
+  // List of top level variable bindings
+  private ImmutableMap<String, Object> variableBindings = ImmutableMap.of();
+
   /**
    * @param builder a builder for the Workspace
    * @param ruleClassProvider a provider for known rule classes
@@ -144,7 +168,8 @@ public class WorkspaceFactory {
     Preconditions.checkNotNull(importedExtensions);
     execute(ast, importedExtensions, new StoredEventHandler());
   }
-  
+
+
   private void execute(BuildFileAST ast, @Nullable Map<String, Extension> importedExtensions,
       StoredEventHandler localReporter)
       throws InterruptedException {
@@ -152,14 +177,45 @@ public class WorkspaceFactory {
         .setGlobals(Environment.BUILD)
         .setEventHandler(localReporter);
     if (importedExtensions != null) {
-      environmentBuilder.setImportedExtensions(importedExtensions);
+      importMap =
+          ImmutableMap.<String, Extension>builder()
+              .putAll(parentImportMap)
+              .putAll(importedExtensions)
+              .build();
+    } else {
+      importMap = parentImportMap;
     }
+    environmentBuilder.setImportedExtensions(importMap);
     Environment workspaceEnv = environmentBuilder.setLoadingPhase().build();
     addWorkspaceFunctions(workspaceEnv, localReporter);
+    for (Map.Entry<String, Object> binding : parentVariableBindings.entrySet()) {
+      try {
+        workspaceEnv.update(binding.getKey(), binding.getValue());
+      } catch (EvalException e) {
+        // This should never happen because everything was already evaluated.
+        throw new IllegalStateException(e);
+      }
+    }
     if (!ast.exec(workspaceEnv, localReporter)) {
       localReporter.handle(Event.error("Error evaluating WORKSPACE file"));
     }
 
+    // Save the list of variable bindings for the next part of the workspace file. The list of
+    // variable bindings of interest are the global variable bindings that are defined by the user,
+    // so not the workspace functions.
+    // Workspace functions are not serializable and should not be passed over sky values. They
+    // also have a package builder specific to the current part and should be reinitialized for
+    // each workspace file.
+    ImmutableMap.Builder<String, Object> bindingsBuilder = ImmutableMap.builder();
+    Frame globals = workspaceEnv.getGlobals();
+    for (String s : globals.getDirectVariableNames()) {
+      Object o = globals.get(s);
+      if (!isAWorkspaceFunction(s, o)) {
+        bindingsBuilder.put(s, o);
+      }
+    }
+    variableBindings = bindingsBuilder.build();
+
     builder.addEvents(localReporter.getEvents());
     if (localReporter.hasErrors()) {
       builder.setContainsErrors();
@@ -167,11 +223,38 @@ public class WorkspaceFactory {
     localReporter.clear();
   }
 
+  private boolean isAWorkspaceFunction(String name, Object o) {
+    return STATIC_WORKSPACE_FUNCTIONS.contains(name) || (workspaceFunctions.get(name) == o);
+  }
+
   private static boolean isLegalWorkspaceName(String name) {
     Matcher matcher = LEGAL_WORKSPACE_NAME.matcher(name);
     return matcher.matches();
   }
 
+  /**
+   * Adds the various values returned by the parsing of the previous workspace file parts.
+   * {@code aPackage} is the package returned by the parent WorkspaceFileFunction, {@code importMap}
+   * is the list of load statements imports computed by the parent WorkspaceFileFunction and
+   * {@code variableBindings} the list of top level variable bindings of that same call.
+   */
+  public void setParent(
+      Package aPackage,
+      ImmutableMap<String, Extension> importMap,
+      ImmutableMap<String, Object> bindings)
+      throws NameConflictException {
+    this.parentVariableBindings = bindings;
+    this.parentImportMap = importMap;
+    // Transmit the content of the parent package to the new package builder.
+    builder.addEvents(aPackage.getEvents());
+    if (aPackage.containsErrors()) {
+      builder.setContainsErrors();
+    }
+    for (Target target : aPackage.getTargets(Rule.class)) {
+      builder.addRule((Rule) target);
+    }
+  }
+
   @SkylarkSignature(name = "workspace", objectType = Object.class, returnType = SkylarkList.class,
       doc = "Sets the name for this workspace. Workspace names should be a Java-package-style "
           + "description of the project, using underscores as separators, e.g., "
@@ -328,4 +411,12 @@ public class WorkspaceFactory {
   static {
     SkylarkSignatureProcessor.configureSkylarkFunctions(WorkspaceFactory.class);
   }
+
+  public Map<String, Extension> getImportMap() {
+    return importMap;
+  }
+
+  public Map<String, Object> getVariableBindings() {
+    return variableBindings;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
index fbf079c85b..7f230a7ef6 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
@@ -23,7 +23,6 @@ import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.packages.AggregatingAttributeMapper;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
-import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Rule;
@@ -31,12 +30,12 @@ import com.google.devtools.build.lib.packages.RuleSerializer;
 import com.google.devtools.build.lib.skyframe.FileSymlinkException;
 import com.google.devtools.build.lib.skyframe.FileValue;
 import com.google.devtools.build.lib.skyframe.InconsistentFilesystemException;
-import com.google.devtools.build.lib.skyframe.PackageValue;
+import com.google.devtools.build.lib.skyframe.PackageLookupValue;
+import com.google.devtools.build.lib.skyframe.WorkspaceFileValue;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.util.Preconditions;
-import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -340,7 +339,6 @@ public abstract class RepositoryFunction {
       throws RepositoryFunctionException {
     try {
       FileSystemUtils.createDirectoryAndParents(repositoryDirectory);
-      FileSystem fs = repositoryDirectory.getFileSystem();
       if (repositoryDirectory.getFileSystem().supportsSymbolicLinksNatively()) {
         for (Path target : targetDirectory.getDirectoryEntries()) {
           Path symlinkPath =
@@ -380,33 +378,37 @@ public abstract class RepositoryFunction {
   @Nullable
   public static Rule getRule(String repository, Environment env)
       throws RepositoryFunctionException {
-    SkyKey packageKey = PackageValue.key(Label.EXTERNAL_PACKAGE_IDENTIFIER);
-    PackageValue packageValue;
-    try {
-      packageValue = (PackageValue) env.getValueOrThrow(packageKey,
-          NoSuchPackageException.class);
-    } catch (NoSuchPackageException e) {
-      throw new RepositoryFunctionException(
-          new BuildFileNotFoundException(
-              Label.EXTERNAL_PACKAGE_IDENTIFIER, "Could not load //external package"),
-          Transience.PERSISTENT);
-    }
-    if (packageValue == null) {
+
+    SkyKey packageLookupKey = PackageLookupValue.key(Label.EXTERNAL_PACKAGE_IDENTIFIER);
+    PackageLookupValue packageLookupValue;
+    packageLookupValue = (PackageLookupValue) env.getValue(packageLookupKey);
+    if (packageLookupValue == null) {
       return null;
     }
+    RootedPath workspacePath =
+        RootedPath.toRootedPath(packageLookupValue.getRoot(), new PathFragment("WORKSPACE"));
 
-    Package externalPackage = packageValue.getPackage();
-    if (externalPackage.containsErrors()) {
-      throw new RepositoryFunctionException(
-          new BuildFileContainsErrorsException(
-              Label.EXTERNAL_PACKAGE_IDENTIFIER, "Could not load //external package"),
-          Transience.PERSISTENT);
-    }
-    Rule rule = externalPackage.getRule(repository);
-    if (rule == null) {
-      throw new RepositoryNotFoundException(repository);
-    }
-    return rule;
+    SkyKey workspaceKey = WorkspaceFileValue.key(workspacePath);
+    do {
+      WorkspaceFileValue value = (WorkspaceFileValue) env.getValue(workspaceKey);
+      if (value == null) {
+        return null;
+      }
+      // TODO(dmarting): stop at cycle and report a more intelligible error than cycle reporting.
+      Package externalPackage = value.getPackage();
+      if (externalPackage.containsErrors()) {
+        throw new RepositoryFunctionException(
+            new BuildFileContainsErrorsException(
+                Label.EXTERNAL_PACKAGE_IDENTIFIER, "Could not load //external package"),
+            Transience.PERSISTENT);
+      }
+      Rule rule = externalPackage.getRule(repository);
+      if (rule != null) {
+        return rule;
+      }
+      workspaceKey = value.next();
+    } while (workspaceKey != null);
+    throw new RepositoryNotFoundException(repository);
   }
 
   @Nullable
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
index 305d2e89fc..2ab3267d87 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
@@ -14,15 +14,19 @@
 
 package com.google.devtools.build.lib.skyframe;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Package.LegacyBuilder;
+import com.google.devtools.build.lib.packages.Package.NameConflictException;
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.packages.WorkspaceFactory;
 import com.google.devtools.build.lib.skyframe.PackageFunction.PackageFunctionException;
 import com.google.devtools.build.lib.skyframe.WorkspaceFileValue.WorkspaceFileKey;
 import com.google.devtools.build.lib.syntax.BuildFileAST;
+import com.google.devtools.build.lib.syntax.Environment.Extension;
 import com.google.devtools.build.lib.syntax.Mutability;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.RootedPath;
@@ -64,10 +68,20 @@ public class WorkspaceFileFunction implements SkyFunction {
 
     Path repoWorkspace = workspaceRoot.getRoot().getRelative(workspaceRoot.getRelativePath());
     LegacyBuilder builder =
-        com.google.devtools.build.lib.packages.Package.newExternalPackageBuilder(
-            repoWorkspace, ruleClassProvider.getRunfilesPrefix());
+        Package.newExternalPackageBuilder(repoWorkspace, ruleClassProvider.getRunfilesPrefix());
+
+    if (workspaceASTValue.getASTs().isEmpty()) {
+      return new WorkspaceFileValue(
+          builder.build(), // resulting package
+          ImmutableMap.<String, Extension>of(), // list of imports
+          ImmutableMap.<String, Object>of(), // list of symbol bindings
+          workspaceRoot, // Workspace root
+          0, // first fragment, idx = 0
+          false); // last fragment
+    }
+    WorkspaceFactory parser;
     try (Mutability mutability = Mutability.create("workspace %s", repoWorkspace)) {
-      WorkspaceFactory parser =
+      parser =
           new WorkspaceFactory(
               builder,
               ruleClassProvider,
@@ -75,23 +89,39 @@ public class WorkspaceFileFunction implements SkyFunction {
               mutability,
               directories.getEmbeddedBinariesRoot(),
               directories.getWorkspace());
-      try {
-        for (BuildFileAST ast : workspaceASTValue.getASTs()) {
-          PackageFunction.SkylarkImportResult importResult =
-              PackageFunction.fetchImportsFromBuildFile(
-                  repoWorkspace, Label.EXTERNAL_PACKAGE_IDENTIFIER, ast, env, null);
-          if (importResult != null) {
-            parser.execute(ast, importResult.importMap);
-          } else {
-            return null;
-          }
+      if (key.getIndex() > 0) {
+        WorkspaceFileValue prevValue =
+            (WorkspaceFileValue)
+                env.getValue(WorkspaceFileValue.key(key.getPath(), key.getIndex() - 1));
+        if (prevValue == null) {
+          return null;
         }
-      } catch (PackageFunctionException e) {
-        throw new WorkspaceFileFunctionException(e, Transience.PERSISTENT);
+        if (prevValue.next() == null) {
+          return prevValue;
+        }
+        parser.setParent(prevValue.getPackage(), prevValue.getImportMap(), prevValue.getBindings());
+      }
+      BuildFileAST ast = workspaceASTValue.getASTs().get(key.getIndex());
+      PackageFunction.SkylarkImportResult importResult =
+          PackageFunction.fetchImportsFromBuildFile(
+              repoWorkspace, Label.EXTERNAL_PACKAGE_IDENTIFIER, ast, env, null);
+      if (importResult == null) {
+        return null;
       }
+      // TODO(dmarting): give a nice error message when redefining a repository name and
+      // getIndex() > 0.
+      parser.execute(ast, importResult.importMap);
+    } catch (PackageFunctionException | NameConflictException e) {
+      throw new WorkspaceFileFunctionException(e, Transience.PERSISTENT);
     }
 
-    return new WorkspaceFileValue(builder.build(), workspaceRoot, 0, false);
+    return new WorkspaceFileValue(
+        builder.build(),
+        parser.getImportMap(),
+        parser.getVariableBindings(),
+        workspaceRoot,
+        key.getIndex(),
+        key.getIndex() < workspaceASTValue.getASTs().size() - 1);
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileValue.java
index 877d7c5e33..cb49728cbc 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileValue.java
@@ -14,14 +14,17 @@
 
 package com.google.devtools.build.lib.skyframe;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.Package;
+import com.google.devtools.build.lib.syntax.Environment.Extension;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
+import java.util.Map;
 import java.util.Objects;
 
 /**
@@ -84,12 +87,38 @@ public class WorkspaceFileValue implements SkyValue {
   private final int idx;
   private final RootedPath path;
   private final boolean hasNext;
+  // TODO(dmarting): The bindings bind to "Object" which we should ultimately replace by a super
+  // type in the Environment class (that would ease the serialization of this object).
+  private final ImmutableMap<String, Object> bindings;
+  private final ImmutableMap<String, Extension> importMap;
 
-  public WorkspaceFileValue(Package pkg, RootedPath path, int idx, boolean hasNext) {
+  /**
+   * Create a WorkspaceFileValue containing the various values necessary to compute the split
+   * WORKSPACE file.
+   * @param pkg Package built by agreggating all parts of the split WORKSPACE file up to this one.
+   * @param importMap List of imports (i.e., load statements) present in all parts of the split
+   * WORKSPACE file up to this one.
+   * @param bindings List of top-level variable bindings from the all parts of the split
+   * WORKSPACE file up to this one. The key is the name of the bindings and the value is the actual
+   * object.
+   * @param path The rooted path to workspace file to parse.
+   * @param idx The index of this part of the split WORKSPACE file (0 for the first one, 1 for the
+   * second one and so on).
+   * @param hasNext Is there a next part in the WORKSPACE file or this part the last one?
+   */
+  public WorkspaceFileValue(
+      Package pkg,
+      Map<String, Extension> importMap,
+      Map<String, Object> bindings,
+      RootedPath path,
+      int idx,
+      boolean hasNext) {
     this.pkg = Preconditions.checkNotNull(pkg);
     this.idx = idx;
     this.path = path;
     this.hasNext = hasNext;
+    this.bindings = ImmutableMap.copyOf(bindings);
+    this.importMap = ImmutableMap.copyOf(importMap);
   }
 
   /**
@@ -102,10 +131,10 @@ public class WorkspaceFileValue implements SkyValue {
 
   @Override
   public String toString() {
-    return "<WorkspaceFileValue idx=" + idx + ">";
+    return "<WorkspaceFileValue path=" + path + " idx=" + idx + ">";
   }
 
-  private static SkyKey key(RootedPath path, int idx) {
+  static SkyKey key(RootedPath path, int idx) {
     return new SkyKey(SkyFunctions.WORKSPACE_FILE, new WorkspaceFileKey(path, idx));
   }
 
@@ -149,4 +178,12 @@ public class WorkspaceFileValue implements SkyValue {
   public RootedPath getPath() {
     return path;
   }
+
+  public ImmutableMap<String, Object> getBindings() {
+    return bindings;
+  }
+
+  public ImmutableMap<String, Extension> getImportMap() {
+    return importMap;
+  }
 }