Inject the Constants.TOOLS_REPOSITORY in SkylarkRuleClassFunctions.testBaseRule

via the Skylark and delete the constant. Also, change the isLoadingPhase in the 
Skylark environment an enum Phase in order to:
- Decide whether testRules are enabled or not and,
- Check that the toolsRepository is set when in the LOADING phase.

Finally, a few tests that were using ConfiguredRuleClassProvider directly 
had to be updated to set a tools repository, otherwise createGlobals() fails.

--
MOS_MIGRATED_REVID=121022804

10 files changed, 108 insertions, 63 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/Constants.java b/src/main/java/com/google/devtools/build/lib/Constants.java
index 556a742612..03547219ea 100644
--- a/src/main/java/com/google/devtools/build/lib/Constants.java
+++ b/src/main/java/com/google/devtools/build/lib/Constants.java
@@ -27,7 +27,4 @@ public final class Constants {
 
   // Native Java deps are all linked into a single file, which is named with this value + ".so".
   public static final String NATIVE_DEPS_LIB_SUFFIX = "_nativedeps";
-
-  // Most other tools dependencies use this; we plan to split it into per-language repositories.
-  public static final String TOOLS_REPOSITORY = "@bazel_tools";
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
index bfc8b5bc2e..a39737ce7c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
@@ -44,6 +44,7 @@ import com.google.devtools.build.lib.rules.SkylarkModules;
 import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
 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.Phase;
 import com.google.devtools.build.lib.syntax.Mutability;
 import com.google.devtools.common.options.OptionsClassProvider;
 
@@ -537,7 +538,8 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
         .setEventHandler(eventHandler)
         .setFileContentHashCode(astFileContentHashCode)
         .setImportedExtensions(importMap)
-        .setLoadingPhase()
+        .setToolsRepository(toolsRepository)
+        .setPhase(Phase.LOADING)
         .build();
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index d2f70902ec..6f86bbd76d 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -44,6 +44,7 @@ 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.NoSuchVariableException;
+import com.google.devtools.build.lib.syntax.Environment.Phase;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.EvalUtils;
 import com.google.devtools.build.lib.syntax.Expression;
@@ -1504,7 +1505,8 @@ public final class PackageFactory {
           .setGlobals(Environment.BUILD)
           .setEventHandler(eventHandler)
           .setImportedExtensions(imports)
-          .setLoadingPhase()
+          .setToolsRepository(ruleClassProvider.getToolsRepository())
+          .setPhase(Phase.LOADING)
           .build();
 
       pkgBuilder.setFilename(buildFilePath)
@@ -1576,7 +1578,8 @@ public final class PackageFactory {
       Environment pkgEnv = Environment.builder(mutability)
           .setGlobals(Environment.BUILD)
           .setEventHandler(NullEventHandler.INSTANCE)
-          .setLoadingPhase()
+          .setToolsRepository(ruleClassProvider.getToolsRepository())
+          .setPhase(Phase.LOADING)
           .build();
 
       Package.LegacyBuilder pkgBuilder = new Package.LegacyBuilder(packageId,
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 7608c31293..f8d885fc01 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
@@ -37,6 +37,7 @@ 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.Environment.Phase;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.FuncallExpression;
 import com.google.devtools.build.lib.syntax.FunctionSignature;
@@ -73,7 +74,7 @@ public class WorkspaceFactory {
           PackageFactory.PKG_CONTEXT);
 
   private final LegacyBuilder builder;
-  
+
   private final Path installDir;
   private final Path workspaceDir;
   private final Mutability mutability;
@@ -190,7 +191,7 @@ public class WorkspaceFactory {
       importMap = parentImportMap;
     }
     environmentBuilder.setImportedExtensions(importMap);
-    Environment workspaceEnv = environmentBuilder.setLoadingPhase().build();
+    Environment workspaceEnv = environmentBuilder.setPhase(Phase.WORKSPACE).build();
     addWorkspaceFunctions(workspaceEnv, localReporter);
     for (Map.Entry<String, Object> binding : parentVariableBindings.entrySet()) {
       try {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
index a5f3d646fe..c8c0c86494 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
@@ -33,7 +33,6 @@ import com.google.common.cache.LoadingCache;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-import com.google.devtools.build.lib.Constants;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.BaseRuleClasses;
 import com.google.devtools.build.lib.analysis.OutputGroupProvider;
@@ -194,42 +193,43 @@ public class SkylarkRuleClassFunctions {
           .build();
 
   /** Parent rule class for test Skylark rules. */
-  public static final RuleClass testBaseRule =
-      new RuleClass.Builder("$test_base_rule", RuleClassType.ABSTRACT, true, baseRule)
-          .add(attr("size", STRING).value("medium").taggable()
-              .nonconfigurable("used in loading phase rule validation logic"))
-          .add(attr("timeout", STRING).taggable()
-              .nonconfigurable("used in loading phase rule validation logic").value(
-              new Attribute.ComputedDefault() {
-                @Override
-                public Object getDefault(AttributeMap rule) {
-                  TestSize size = TestSize.getTestSize(rule.get("size", Type.STRING));
-                  if (size != null) {
-                    String timeout = size.getDefaultTimeout().toString();
-                    if (timeout != null) {
-                      return timeout;
+  public static final RuleClass getTestBaseRule(String toolsRespository) {
+    return new RuleClass.Builder("$test_base_rule", RuleClassType.ABSTRACT, true, baseRule)
+        .add(attr("size", STRING).value("medium").taggable()
+            .nonconfigurable("used in loading phase rule validation logic"))
+        .add(attr("timeout", STRING).taggable()
+            .nonconfigurable("used in loading phase rule validation logic").value(
+                new Attribute.ComputedDefault() {
+                  @Override
+                  public Object getDefault(AttributeMap rule) {
+                    TestSize size = TestSize.getTestSize(rule.get("size", Type.STRING));
+                    if (size != null) {
+                      String timeout = size.getDefaultTimeout().toString();
+                      if (timeout != null) {
+                        return timeout;
+                      }
                     }
+                    return "illegal";
                   }
-                  return "illegal";
-                }
-              }))
-          .add(attr("flaky", BOOLEAN).value(false).taggable()
-              .nonconfigurable("taggable - called in Rule.getRuleTags"))
-          .add(attr("shard_count", INTEGER).value(-1))
-          .add(attr("local", BOOLEAN).value(false).taggable()
-              .nonconfigurable("policy decision: this should be consistent across configurations"))
-          .add(attr("args", STRING_LIST)
-              .nonconfigurable("policy decision: should be consistent across configurations"))
-          .add(attr("$test_runtime", LABEL_LIST).cfg(HOST).value(ImmutableList.of(
-              labelCache.getUnchecked(Constants.TOOLS_REPOSITORY + "//tools/test:runtime"))))
-          .add(attr(":run_under", LABEL).cfg(DATA).value(RUN_UNDER))
-          .add(attr(":gcov", LABEL_LIST).cfg(HOST).value(GCOV))
-          .add(attr(":coverage_support", LABEL_LIST).cfg(HOST).value(COVERAGE_SUPPORT))
-          .add(
-              attr(":coverage_report_generator", LABEL_LIST)
-                  .cfg(HOST)
-                  .value(COVERAGE_REPORT_GENERATOR))
-          .build();
+                }))
+        .add(attr("flaky", BOOLEAN).value(false).taggable()
+            .nonconfigurable("taggable - called in Rule.getRuleTags"))
+        .add(attr("shard_count", INTEGER).value(-1))
+        .add(attr("local", BOOLEAN).value(false).taggable()
+            .nonconfigurable("policy decision: this should be consistent across configurations"))
+        .add(attr("args", STRING_LIST)
+            .nonconfigurable("policy decision: should be consistent across configurations"))
+        .add(attr("$test_runtime", LABEL_LIST).cfg(HOST).value(ImmutableList.of(
+            labelCache.getUnchecked(toolsRespository + "//tools/test:runtime"))))
+        .add(attr(":run_under", LABEL).cfg(DATA).value(RUN_UNDER))
+        .add(attr(":gcov", LABEL_LIST).cfg(HOST).value(GCOV))
+        .add(attr(":coverage_support", LABEL_LIST).cfg(HOST).value(COVERAGE_SUPPORT))
+        .add(
+            attr(":coverage_report_generator", LABEL_LIST)
+            .cfg(HOST)
+            .value(COVERAGE_REPORT_GENERATOR))
+        .build();
+  }
 
   /**
    * In native code, private values start with $.
@@ -313,7 +313,8 @@ public class SkylarkRuleClassFunctions {
         throws EvalException, ConversionException {
       funcallEnv.checkLoadingPhase("rule", ast.getLocation());
       RuleClassType type = test ? RuleClassType.TEST : RuleClassType.NORMAL;
-      RuleClass parent = test ? testBaseRule : (executable ? binaryBaseRule : baseRule);
+      RuleClass parent = test ? getTestBaseRule(funcallEnv.getToolsRepository())
+          : (executable ? binaryBaseRule : baseRule);
 
       // We'll set the name later, pass the empty string for now.
       RuleClass.Builder builder = new RuleClass.Builder("", type, true, parent);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
index 8c1def42b2..8ef298a697 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
@@ -14,6 +14,8 @@
 
 package com.google.devtools.build.lib.syntax;
 
+import static com.google.common.base.Preconditions.checkState;
+
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
@@ -75,6 +77,11 @@ import javax.annotation.Nullable;
 public final class Environment implements Freezable {
 
   /**
+   * A phase for enabling or disabling certain builtin functions
+   */
+  public enum Phase { WORKSPACE, LOADING, ANALYSIS }
+
+  /**
    * A Frame is a Map of bindings, plus a {@link Mutability} and a parent Frame
    * from which to inherit bindings.
    *
@@ -331,7 +338,7 @@ public final class Environment implements Freezable {
    * Is this Environment being executed during the loading phase?
    * Many builtin functions are only enabled during the loading phase, and check this flag.
    */
-  private final boolean isLoadingPhase;
+  private Phase phase;
 
   /**
    * When in a lexical (Skylark) Frame, this set contains the variable names that are global,
@@ -357,6 +364,11 @@ public final class Environment implements Freezable {
   @Nullable private final Label callerLabel;
 
   /**
+   * The path to the tools repository.
+   */
+  private final String toolsRepository;
+
+  /**
    * Enters a scope by saving state to a new Continuation
    * @param function the function whose scope to enter
    * @param caller the source AST node for the caller
@@ -391,11 +403,10 @@ public final class Environment implements Freezable {
   /**
    * Is this Environment being evaluated during the loading phase?
    * This is fixed during Environment setup, and enables various functions
-   * that are not available during the analysis phase.
-   * @return true if this Environment corresponds to code during the loading phase.
+   * that are not available during the analysis or workspace phase.
    */
-  private boolean isLoadingPhase() {
-    return isLoadingPhase;
+  public Phase getPhase() {
+    return phase;
   }
 
   /**
@@ -403,7 +414,7 @@ public final class Environment implements Freezable {
    * @param symbol name of the function being only authorized thus.
    */
   public void checkLoadingPhase(String symbol, Location loc) throws EvalException {
-    if (!isLoadingPhase()) {
+    if (phase != Phase.LOADING) {
       throw new EvalException(loc, symbol + "() can only be called during the loading phase");
     }
   }
@@ -481,7 +492,7 @@ public final class Environment implements Freezable {
    * @param importedExtensions Extension-s from which to import bindings with load()
    * @param isSkylark true if in Skylark context
    * @param fileContentHashCode a hash for the source file being evaluated, if any
-   * @param isLoadingPhase true if in loading phase
+   * @param phase the current phase
    * @param callerLabel the label this environment came from
    */
   private Environment(
@@ -491,8 +502,9 @@ public final class Environment implements Freezable {
       Map<String, Extension> importedExtensions,
       boolean isSkylark,
       @Nullable String fileContentHashCode,
-      boolean isLoadingPhase,
-      @Nullable Label callerLabel) {
+      Phase phase,
+      @Nullable Label callerLabel,
+      String toolsRepository) {
     this.globalFrame = Preconditions.checkNotNull(globalFrame);
     this.dynamicFrame = Preconditions.checkNotNull(dynamicFrame);
     Preconditions.checkArgument(globalFrame.mutability().isMutable());
@@ -501,8 +513,9 @@ public final class Environment implements Freezable {
     this.importedExtensions = importedExtensions;
     this.isSkylark = isSkylark;
     this.fileContentHashCode = fileContentHashCode;
-    this.isLoadingPhase = isLoadingPhase;
+    this.phase = phase;
     this.callerLabel = callerLabel;
+    this.toolsRepository = toolsRepository;
   }
 
   /**
@@ -511,12 +524,13 @@ public final class Environment implements Freezable {
   public static class Builder {
     private final Mutability mutability;
     private boolean isSkylark = false;
-    private boolean isLoadingPhase = false;
+    private Phase phase = Phase.ANALYSIS;
     @Nullable private Frame parent;
     @Nullable private EventHandler eventHandler;
     @Nullable private Map<String, Extension> importedExtensions;
     @Nullable private String fileContentHashCode;
     private Label label;
+    private String toolsRepository;
 
     Builder(Mutability mutability) {
       this.mutability = mutability;
@@ -529,10 +543,10 @@ public final class Environment implements Freezable {
       return this;
     }
 
-    /** Enables loading phase only functions in this Environment. */
-    public Builder setLoadingPhase() {
-      Preconditions.checkState(!isLoadingPhase);
-      isLoadingPhase = true;
+    /** Enables loading or workspace phase only functions in this Environment. */
+    public Builder setPhase(Phase phase) {
+      Preconditions.checkState(this.phase == Phase.ANALYSIS);
+      this.phase = phase;
       return this;
     }
 
@@ -563,6 +577,12 @@ public final class Environment implements Freezable {
       return this;
     }
 
+    /** Sets the path to the tools repository */
+    public Builder setToolsRepository(String toolsRepository) {
+      this.toolsRepository = toolsRepository;
+      return this;
+    }
+
     /** Builds the Environment. */
     public Environment build() {
       Preconditions.checkArgument(mutability.isMutable());
@@ -574,6 +594,9 @@ public final class Environment implements Freezable {
       if (importedExtensions == null) {
         importedExtensions = ImmutableMap.of();
       }
+      if (phase == Phase.LOADING) {
+        Preconditions.checkState(this.toolsRepository != null);
+      }
       return new Environment(
           globalFrame,
           dynamicFrame,
@@ -581,8 +604,9 @@ public final class Environment implements Freezable {
           importedExtensions,
           isSkylark,
           fileContentHashCode,
-          isLoadingPhase,
-          label);
+          phase,
+          label,
+          toolsRepository);
     }
 
     public Builder setCallerLabel(Label label) {
@@ -941,4 +965,9 @@ public final class Environment implements Freezable {
     }
     return last;
   }
+
+  public String getToolsRepository() {
+    checkState(toolsRepository != null);
+    return toolsRepository;
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/AbstractCommandTest.java b/src/test/java/com/google/devtools/build/lib/runtime/AbstractCommandTest.java
index 3f745ce2cf..529200f5c6 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/AbstractCommandTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/AbstractCommandTest.java
@@ -18,6 +18,7 @@ import static org.junit.Assert.assertEquals;
 
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
+import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.util.ExitCode;
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionsBase;
@@ -76,6 +77,7 @@ public class AbstractCommandTest {
   @Test
   public void testGetOptionsYieldsAnnotatedOptions() {
     ConfiguredRuleClassProvider ruleClassProvider = new ConfiguredRuleClassProvider.Builder()
+        .setToolsRepository(TestConstants.TOOLS_REPOSITORY)
         .build();
 
     assertThat(
@@ -99,6 +101,7 @@ public class AbstractCommandTest {
   @Test
   public void testOptionsAreInherited() {
     ConfiguredRuleClassProvider ruleClassProvider = new ConfiguredRuleClassProvider.Builder()
+        .setToolsRepository(TestConstants.TOOLS_REPOSITORY)
         .build();
     assertThat(
             BlazeCommandUtils.getOptions(
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcherRcoptionsTest.java b/src/test/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcherRcoptionsTest.java
index ad2d864a33..9398b147f5 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcherRcoptionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcherRcoptionsTest.java
@@ -32,6 +32,7 @@ import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.LockingMode;
 import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.ShutdownBlazeServerException;
 import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass;
 import com.google.devtools.build.lib.testutil.Scratch;
+import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.util.ExitCode;
 import com.google.devtools.build.lib.util.io.RecordingOutErr;
 import com.google.devtools.common.options.Option;
@@ -146,6 +147,8 @@ public class BlazeCommandDispatcherRcoptionsTest {
                     builder.addConfigurationOptions(BuildConfiguration.Options.class);
                     // The defaults package asserts that it is not empty, so we provide options.
                     builder.addConfigurationOptions(MockFragmentOptions.class);
+                    // The tools repository is needed for createGlobals
+                    builder.setToolsRepository(TestConstants.TOOLS_REPOSITORY);
                   }
                 })
             .setInvocationPolicy(InvocationPolicyOuterClass.InvocationPolicy.getDefaultInstance())
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/util/SkylarkTestCase.java b/src/test/java/com/google/devtools/build/lib/skylark/util/SkylarkTestCase.java
index 01af86f231..c19af161a0 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/util/SkylarkTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/util/SkylarkTestCase.java
@@ -28,8 +28,10 @@ import com.google.devtools.build.lib.rules.SkylarkModules;
 import com.google.devtools.build.lib.rules.SkylarkRuleContext;
 import com.google.devtools.build.lib.rules.SkylarkRuleContext.Kind;
 import com.google.devtools.build.lib.syntax.Environment;
+import com.google.devtools.build.lib.syntax.Environment.Phase;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.util.EvaluationTestCase;
+import com.google.devtools.build.lib.testutil.TestConstants;
 
 import org.junit.Before;
 
@@ -55,7 +57,8 @@ public abstract class SkylarkTestCase extends BuildViewTestCase {
             .setSkylark()
             .setEventHandler(getEventHandler())
             .setGlobals(SkylarkModules.getGlobals(SkylarkModules.MODULES))
-            .setLoadingPhase()
+            .setToolsRepository(TestConstants.TOOLS_REPOSITORY)
+            .setPhase(Phase.LOADING)
             .build()
             .setupDynamic(
                 PackageFactory.PKG_CONTEXT,
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
index a67a4539aa..a9873e3fc1 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
@@ -25,12 +25,14 @@ import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.events.util.EventCollectionApparatus;
 import com.google.devtools.build.lib.syntax.Environment;
+import com.google.devtools.build.lib.syntax.Environment.Phase;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.Expression;
 import com.google.devtools.build.lib.syntax.Mutability;
 import com.google.devtools.build.lib.syntax.Parser;
 import com.google.devtools.build.lib.syntax.ParserInputSource;
 import com.google.devtools.build.lib.syntax.Statement;
+import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.testutil.TestMode;
 
 import org.junit.Before;
@@ -69,7 +71,8 @@ public class EvaluationTestCase {
     return Environment.builder(mutability)
         .setGlobals(Environment.BUILD)
         .setEventHandler(getEventHandler())
-        .setLoadingPhase()
+        .setToolsRepository(TestConstants.TOOLS_REPOSITORY)
+        .setPhase(Phase.LOADING)
         .build();
   }