Skip to content

Commit

Permalink
Inject the Constants.TOOLS_REPOSITORY in SkylarkRuleClassFunctions.te…
Browse files Browse the repository at this point in the history
…stBaseRule

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
  • Loading branch information
lfpino authored and meteorcloudy committed Apr 29, 2016
1 parent 60166c5 commit 3fedf9e
Showing 10 changed files with 108 additions and 63 deletions.
3 changes: 0 additions & 3 deletions src/main/java/com/google/devtools/build/lib/Constants.java
Original file line number Diff line number Diff line change
@@ -27,7 +27,4 @@ private 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";
}
Original file line number Diff line number Diff line change
@@ -44,6 +44,7 @@
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 @@ private Environment createSkylarkRuleClassEnvironment(
.setEventHandler(eventHandler)
.setFileContentHashCode(astFileContentHashCode)
.setImportedExtensions(importMap)
.setLoadingPhase()
.setToolsRepository(toolsRepository)
.setPhase(Phase.LOADING)
.build();
}

Original file line number Diff line number Diff line change
@@ -44,6 +44,7 @@
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 Package.LegacyBuilder evaluateBuildFile(
.setGlobals(Environment.BUILD)
.setEventHandler(eventHandler)
.setImportedExtensions(imports)
.setLoadingPhase()
.setToolsRepository(ruleClassProvider.getToolsRepository())
.setPhase(Phase.LOADING)
.build();

pkgBuilder.setFilename(buildFilePath)
@@ -1576,7 +1578,8 @@ private void prefetchGlobs(PackageIdentifier packageId, BuildFileAST buildFileAS
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,
Original file line number Diff line number Diff line change
@@ -37,6 +37,7 @@
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 @@ private void execute(BuildFileAST ast, @Nullable Map<String, Extension> imported
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 {
Original file line number Diff line number Diff line change
@@ -33,7 +33,6 @@
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 Label load(String from) throws Exception {
.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 BaseFunction invoke(BaseFunction implementation, Boolean test, Object att
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);
63 changes: 46 additions & 17 deletions src/main/java/com/google/devtools/build/lib/syntax/Environment.java
Original file line number Diff line number Diff line change
@@ -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;
@@ -74,6 +76,11 @@
*/
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 boolean containsKey(String varname) {
* 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,
@@ -356,6 +363,11 @@ public boolean containsKey(String varname) {
*/
@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
@@ -391,19 +403,18 @@ void exitScope() {
/**
* 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;
}

/**
* Checks that the current Environment is in the loading phase.
* @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 Pair<FuncallExpression, BaseFunction> getTopCall() {
* @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 @@ private Environment(
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 @@ private Environment(
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 @@ private Environment(
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 Builder setSkylark() {
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 Builder setFileContentHashCode(String fileContentHashCode) {
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,15 +594,19 @@ public Environment build() {
if (importedExtensions == null) {
importedExtensions = ImmutableMap.of();
}
if (phase == Phase.LOADING) {
Preconditions.checkState(this.toolsRepository != null);
}
return new Environment(
globalFrame,
dynamicFrame,
eventHandler,
importedExtensions,
isSkylark,
fileContentHashCode,
isLoadingPhase,
label);
phase,
label,
toolsRepository);
}

public Builder setCallerLabel(Label label) {
@@ -941,4 +965,9 @@ public List<Statement> parseFile(String... input) {
}
return last;
}

public String getToolsRepository() {
checkState(toolsRepository != null);
return toolsRepository;
}
}
Original file line number Diff line number Diff line change
@@ -18,6 +18,7 @@

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 void testGetNameYieldsAnnotatedName() {
@Test
public void testGetOptionsYieldsAnnotatedOptions() {
ConfiguredRuleClassProvider ruleClassProvider = new ConfiguredRuleClassProvider.Builder()
.setToolsRepository(TestConstants.TOOLS_REPOSITORY)
.build();

assertThat(
@@ -99,6 +101,7 @@ private static class CommandB extends ConcreteCommand {}
@Test
public void testOptionsAreInherited() {
ConfiguredRuleClassProvider ruleClassProvider = new ConfiguredRuleClassProvider.Builder()
.setToolsRepository(TestConstants.TOOLS_REPOSITORY)
.build();
assertThat(
BlazeCommandUtils.getOptions(
Original file line number Diff line number Diff line change
@@ -32,6 +32,7 @@
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 void initializeRuleClasses(ConfiguredRuleClassProvider.Builder builder) {
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())
Loading
Oops, something went wrong.

0 comments on commit 3fedf9e

Please sign in to comment.