Add basic builtins injection functionality, behind a flag
BzlLoadFunction now requests the injected builtins from StarlarkBuiltinsFunction. Added BuildViewTestCase tests of injection. Improved error handling is in a follow-up CL.
Other changes:
- Add cautionary note about BuildViewTestCase#getConfiguredTarget returning null.
Work toward #11437.
RELNOTES: None
PiperOrigin-RevId: 315115168
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
index c1a3990..d8f9acf 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
@@ -92,8 +92,9 @@
// instead accept the set of predeclared bindings. Simplify the code path and then this comment.
private final RuleClassProvider ruleClassProvider;
- // TODO(#11437): Remove once we're getting builtins from StarlarkBuiltinsValue instead.
- private final ImmutableMap<String, Object> predeclaredForBuildBzl;
+ // Used for BUILD .bzls if injection is disabled.
+ // TODO(#11437): Remove once injection is on unconditionally.
+ private final StarlarkBuiltinsValue uninjectedStarlarkBuiltins;
private final ImmutableMap<String, Object> predeclaredForWorkspaceBzl;
@@ -115,12 +116,9 @@
ASTManager astManager,
@Nullable CachedBzlLoadDataManager cachedBzlLoadDataManager) {
this.ruleClassProvider = ruleClassProvider;
- this.predeclaredForBuildBzl =
- StarlarkBuiltinsFunction.createPredeclaredForBuildBzlUsingInjection(
- ruleClassProvider,
- packageFactory,
- /*exportedToplevels=*/ ImmutableMap.of(),
- /*exportedRules=*/ ImmutableMap.of());
+ this.uninjectedStarlarkBuiltins =
+ StarlarkBuiltinsFunction.createStarlarkBuiltinsValueWithoutInjection(
+ ruleClassProvider, packageFactory);
this.predeclaredForWorkspaceBzl =
StarlarkBuiltinsFunction.createPredeclaredForWorkspaceBzl(
ruleClassProvider, packageFactory);
@@ -338,6 +336,27 @@
cachedBzlLoadDataManager.reset();
}
+ /**
+ * Retrieves {@link StarlarkBuiltinsValue}, or uses a dummy value if either 1) the builtins
+ * mechanism is disabled or 2) the current .bzl is not being loaded for a BUILD file
+ *
+ * <p>Returns null if a Skyframe restart/error occurred.
+ */
+ @Nullable
+ private StarlarkBuiltinsValue getStarlarkBuiltinsValue(
+ BzlLoadValue.Key key, Environment env, StarlarkSemantics starlarkSemantics)
+ throws InterruptedException {
+ // Don't request @builtins if we're in workspace evaluation, or already in @builtins evaluation.
+ if (!(key instanceof BzlLoadValue.KeyForBuild)
+ // TODO(#11437): Remove ability to disable injection by setting flag to empty string.
+ || starlarkSemantics.experimentalBuiltinsBzlPath().equals("")) {
+ return uninjectedStarlarkBuiltins;
+ }
+ // May be null.
+ return (StarlarkBuiltinsValue) env.getValue(StarlarkBuiltinsValue.key());
+ }
+
+ @Nullable
private static ContainingPackageLookupValue getContainingPackageLookupValue(
Environment env, Label label)
throws InconsistentFilesystemException, BzlLoadFailedException, InterruptedException {
@@ -443,6 +462,12 @@
return null;
}
+ StarlarkBuiltinsValue starlarkBuiltinsValue =
+ getStarlarkBuiltinsValue(key, env, starlarkSemantics);
+ if (starlarkBuiltinsValue == null) {
+ return null;
+ }
+
if (getContainingPackageLookupValue(env, label) == null) {
return null;
}
@@ -462,7 +487,13 @@
try {
result =
computeInternalWithAST(
- key, filePath, starlarkSemantics, astLookupValue, env, inliningState);
+ key,
+ filePath,
+ starlarkSemantics,
+ starlarkBuiltinsValue,
+ astLookupValue,
+ env,
+ inliningState);
} catch (InconsistentFilesystemException | BzlLoadFailedException | InterruptedException e) {
astManager.doneWithASTFileLookupValue(label);
throw e;
@@ -479,6 +510,7 @@
BzlLoadValue.Key key,
PathFragment filePath,
StarlarkSemantics starlarkSemantics,
+ StarlarkBuiltinsValue starlarkBuiltinsValue,
ASTFileLookupValue astLookupValue,
Environment env,
@Nullable InliningState inliningState)
@@ -540,7 +572,9 @@
}
byte[] transitiveDigest = fp.digestAndReset();
- Module module = Module.withPredeclared(starlarkSemantics, getPredeclaredEnvironment(key));
+ Module module =
+ Module.withPredeclared(
+ starlarkSemantics, getPredeclaredEnvironment(key, starlarkBuiltinsValue));
module.setClientData(BazelModuleContext.create(label, transitiveDigest));
// executeBzlFile may post events to the Environment's handler, but events do not matter when
// caching BzlLoadValues. Note that executing the module mutates it.
@@ -693,6 +727,11 @@
Environment strippedEnv = ((RecordingSkyFunctionEnvironment) env).getDelegate();
List<BzlLoadValue> bzlLoads = Lists.newArrayListWithExpectedSize(keys.size());
+ // For determinism's sake while inlining, preserve the first exception and continue to run
+ // subsequently listed loads to completion/exception, loading all transitive deps anyway.
+ // TODO(brandjon, adgar): What's the deal with determinism w.r.t. InterruptedException, and
+ // don't null returns mean the first exception seen isn't deterministic? See also cl/263656490
+ // and discussion therein.
Exception deferredException = null;
boolean valuesMissing = false;
// NOTE: Iterating over loads in the order listed in the file.
@@ -701,8 +740,6 @@
try {
cachedData = computeInlineWithState(key, strippedEnv, inliningState);
} catch (BzlLoadFailedException | InconsistentFilesystemException e) {
- // For determinism's sake while inlining, preserve the first exception and continue to run
- // subsequently listed loads to completion/exception, loading all transitive deps anyway.
deferredException = MoreObjects.firstNonNull(deferredException, e);
continue;
}
@@ -729,9 +766,10 @@
* Obtains the predeclared environment for a .bzl file, based on the type of file and (if
* applicable) the injected builtins.
*/
- private ImmutableMap<String, Object> getPredeclaredEnvironment(BzlLoadValue.Key key) {
+ private ImmutableMap<String, Object> getPredeclaredEnvironment(
+ BzlLoadValue.Key key, StarlarkBuiltinsValue starlarkBuiltinsValue) {
if (key instanceof BzlLoadValue.KeyForBuild) {
- return predeclaredForBuildBzl;
+ return starlarkBuiltinsValue.predeclaredForBuildBzl;
} else if (key instanceof BzlLoadValue.KeyForWorkspace) {
return predeclaredForWorkspaceBzl;
} else if (key instanceof BzlLoadValue.KeyForBuiltins) {
@@ -860,6 +898,8 @@
static BzlLoadFailedException skylarkErrors(PathFragment file) {
return new BzlLoadFailedException(String.format("Extension '%s' has errors", file));
}
+
+ // TODO(#11437): Add builtinsFailed() factory method here.
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java
index a610a56..1b773f5 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java
@@ -136,9 +136,7 @@
* Returns the set of predeclared symbols to initialize a Starlark {@link Module} with, for
* evaluating .bzls loaded from a BUILD file.
*/
- // TODO(#11437): Make private in follow-up CL to retrieve predeclared environment from
- // StarlarkBuiltinsValue instead of this static helper.
- static ImmutableMap<String, Object> createPredeclaredForBuildBzlUsingInjection(
+ private static ImmutableMap<String, Object> createPredeclaredForBuildBzlUsingInjection(
RuleClassProvider ruleClassProvider,
PackageFactory packageFactory,
ImmutableMap<String, Object> exportedToplevels,
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index b4e8ee4..11a3b83 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -879,7 +879,10 @@
* Returns the ConfiguredTarget for the specified label, configured for the "build" (aka "target")
* configuration. If the label corresponds to a target with a top-level configuration transition,
* that transition is applied to the given config in the returned ConfiguredTarget.
+ *
+ * <p>May return null on error; see {@link #getConfiguredTarget(Label, BuildConfiguration)}.
*/
+ @Nullable
public ConfiguredTarget getConfiguredTarget(String label) throws LabelSyntaxException {
return getConfiguredTarget(label, targetConfig);
}
@@ -888,7 +891,10 @@
* Returns the ConfiguredTarget for the specified label, using the given build configuration. If
* the label corresponds to a target with a top-level configuration transition, that transition is
* applied to the given config in the returned ConfiguredTarget.
+ *
+ * <p>May return null on error; see {@link #getConfiguredTarget(Label, BuildConfiguration)}.
*/
+ @Nullable
protected ConfiguredTarget getConfiguredTarget(String label, BuildConfiguration config)
throws LabelSyntaxException {
return getConfiguredTarget(Label.parseAbsolute(label, ImmutableMap.of()), config);
@@ -906,6 +912,9 @@
*
* @throws AssertionError if the target cannot be transitioned into with the given configuration
*/
+ // TODO(bazel-team): Should we work around b/26382502 by asserting here that the result is not
+ // null?
+ @Nullable
protected ConfiguredTarget getConfiguredTarget(Label label, BuildConfiguration config) {
try {
return view.getConfiguredTargetForTesting(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BuiltinsInjectionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/BuiltinsInjectionTest.java
new file mode 100644
index 0000000..660090d
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/BuiltinsInjectionTest.java
@@ -0,0 +1,215 @@
+// Copyright 2020 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.skyframe;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
+import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
+import com.google.devtools.build.lib.analysis.util.MockRule;
+import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/**
+ * Tests for Starlark builtin injection.
+ *
+ * <p>Essentially these are integration tests between {@link StarlarkBuiltinsFunction} and {@link
+ * BzlLoadFunction}.
+ */
+@RunWith(JUnit4.class)
+public class BuiltinsInjectionTest extends BuildViewTestCase {
+
+ private static final MockRule OVERRIDABLE_RULE = () -> MockRule.define("overridable_rule");
+
+ @Override
+ protected ConfiguredRuleClassProvider getRuleClassProvider() {
+ // Add a fake rule and top-level symbol to override.
+ ConfiguredRuleClassProvider.Builder builder =
+ new ConfiguredRuleClassProvider.Builder()
+ .addRuleDefinition(OVERRIDABLE_RULE)
+ .addStarlarkAccessibleTopLevels("overridable_symbol", "original_value");
+ TestRuleClassProvider.addStandardRules(builder);
+ return builder.build();
+ }
+
+ @Before
+ public void setUp() throws Exception {
+ setStarlarkSemanticsOptions("--experimental_builtins_bzl_path=notdisabled");
+ }
+
+ /**
+ * Writes an exports.bzl file with the given content, along with a BUILD file, in the builtins
+ * location.
+ *
+ * <p>See {@link StarlarkBuiltinsFunction#EXPORTS_ENTRYPOINT} for the significance of exports.bzl.
+ */
+ // TODO(#11437): Don't write the BUILD file once it's no longer needed. Pass staging location into
+ // test setup above.
+ private void writeExportsBzl(String... lines) throws Exception {
+ scratch.file("tools/builtins_staging/BUILD");
+ scratch.file("tools/builtins_staging/exports.bzl", lines);
+ }
+
+ /**
+ * Writes a pkg/dummy.bzl file, and an accompanying BUILD file that loads it and defines a
+ * //pkg:dummy target.
+ */
+ private void writePkgBzl(String... lines) throws Exception {
+ scratch.file(
+ "pkg/BUILD",
+ "load(':dummy.bzl', 'dummy_symbol')",
+ // We define :dummy as a simple file rather than something like a java_library, because
+ // native rules may depend on exports.bzl, which we are messing with in this test suite.
+ "exports_files(['dummy'])");
+ scratch.file("pkg/dummy");
+ List<String> modifiedLines = new ArrayList<>(Arrays.asList(lines));
+ modifiedLines.add(0, "dummy_symbol = None");
+ scratch.file("pkg/dummy.bzl", modifiedLines.toArray(lines));
+ }
+
+ /**
+ * Builds {@code //pkg:dummy} and asserts success.
+ *
+ * <p>This helps us fail fast when {@link #getConfiguredTarget} returns null without emitting
+ * events; see b/26382502.
+ */
+ private void buildDummyAndAssertSuccess() throws Exception {
+ Object result = getConfiguredTarget("//pkg:dummy");
+ assertThat(result).isNotNull();
+ }
+
+ /** Builds {@code //pkg:dummy}, which may be in error. */
+ private void buildDummyWithoutAssertingSuccess() throws Exception {
+ getConfiguredTarget("//pkg:dummy");
+ }
+
+ @Test
+ public void evalWithInjection() throws Exception {
+ writeExportsBzl(
+ "exported_toplevels = {'overridable_symbol': 'new_value'}",
+ "exported_rules = {'overridable_rule': 'new_rule'}",
+ "exported_to_java = {}");
+ writePkgBzl(
+ "print('overridable_symbol :: ' + str(overridable_symbol))",
+ "print('overridable_rule :: ' + str(native.overridable_rule))");
+
+ buildDummyAndAssertSuccess();
+ assertContainsEvent("overridable_symbol :: new_value");
+ assertContainsEvent("overridable_rule :: new_rule");
+ }
+
+ // TODO(#11437): Remove once disabling is not allowed.
+ @Test
+ public void injectionDisabledByFlag() throws Exception {
+ writeExportsBzl(
+ "exported_toplevels = {'overridable_symbol': 'new_value'}",
+ "exported_rules = {'overridable_rule': 'new_rule'}",
+ "exported_to_java = {}");
+ writePkgBzl(
+ "print('overridable_symbol :: ' + str(overridable_symbol))",
+ "print('overridable_rule :: ' + str(native.overridable_rule))");
+ setStarlarkSemanticsOptions("--experimental_builtins_bzl_path=");
+
+ buildDummyAndAssertSuccess();
+ assertContainsEvent("overridable_symbol :: original_value");
+ assertContainsEvent("overridable_rule :: <built-in rule overridable_rule>");
+ }
+
+ // TODO(#11437): Remove once disabling is not allowed.
+ @Test
+ public void exportsBzlMayBeInErrorWhenInjectionIsDisabled() throws Exception {
+ writeExportsBzl( //
+ "PARSE ERROR");
+ writePkgBzl(
+ "print('overridable_symbol :: ' + str(overridable_symbol))",
+ "print('overridable_rule :: ' + str(native.overridable_rule))");
+ setStarlarkSemanticsOptions("--experimental_builtins_bzl_path=");
+
+ buildDummyAndAssertSuccess();
+ assertContainsEvent("overridable_symbol :: original_value");
+ assertContainsEvent("overridable_rule :: <built-in rule overridable_rule>");
+ }
+
+ @Test
+ public void evalWorkspaceBzl_doesNotUseInjection() throws Exception {
+ writeExportsBzl(
+ "exported_toplevels = {'overridable_symbol': 'new_value'}",
+ "exported_rules = {'overridable_rule': 'new_rule'}",
+ "exported_to_java = {}");
+ writePkgBzl();
+ scratch.appendFile(
+ "WORKSPACE", //
+ "load(':foo.bzl', 'dummy_symbol')",
+ "print(dummy_symbol)");
+ scratch.file("BUILD");
+ scratch.file(
+ "foo.bzl",
+ "dummy_symbol = None",
+ "print('overridable_symbol :: ' + str(overridable_symbol))");
+
+ buildDummyAndAssertSuccess();
+ // Builtins for WORKSPACE bzls are populated essentially the same as for BUILD bzls, except that
+ // injection doesn't apply.
+ assertContainsEvent("overridable_symbol :: original_value");
+ // We don't assert that the rule isn't injected because the workspace native object doesn't
+ // contain our original mock rule. We can test this for WORKSPACE files at the top-level though.
+ }
+
+ // TODO(#11437): Broaden to also assert that it can't access rule-specific symbols (like our
+ // override symbols) but can still access basic symbols (print(), rule()).
+ @Test
+ public void evalBuiltinsBzl_cannotAccessNative() throws Exception {
+ writeExportsBzl(
+ // The load string here references a regular label, but it gets loaded using a
+ // BzlLoadValue.KeyForBuiltins because we're in exports.bzl.
+ // TODO(#11437): Rewrite this load syntax to @builtins//...
+ "load('//pkg:builtins_dummy.bzl', 'builtins_dummy')",
+ "exported_toplevels = {'overridable_symbol': 'new_value'}",
+ "exported_rules = {'overridable_rule': 'new_rule'}",
+ "exported_to_java = {}");
+ writePkgBzl();
+ scratch.file(
+ "pkg/builtins_dummy.bzl", //
+ "builtins_dummy = None",
+ "print('overridable_symbol :: ' + str(overridable_symbol))",
+ "print('overridable_rule :: ' + str(native.overridable_rule))");
+ reporter.removeHandler(failFastHandler);
+
+ buildDummyWithoutAssertingSuccess();
+ // Currently overridable_symbol, a piece of (mock) rule logic, is accessible, but a future
+ // change will prevent that.
+ assertContainsEvent("overridable_symbol :: original_value");
+ // Currently, the "native" object actually exists, but isn't fully formed -- it contains generic
+ // methods but not specific rules. We will change this to be completely absent from @builtins.
+ assertContainsEvent("'native' value has no field or method 'overridable_rule'");
+ }
+
+ // TODO(#11437): Reject overriding symbols that don't already exist. Reject overriding "native".
+ // Reject overriding non-rule-logic symbols such as package(), select(), environment_extension,
+ // varref(), etc.
+
+ // TODO(#11437): Add tests that the _internal symbol is accessible nowhere but builtins bzls.
+
+ // TODO(#11437): Add tests of the _internal symbol's usage within builtins bzls.
+
+ // TODO(#11437): Add test cases for BUILD file injection, and WORKSPACE file non-injection, when
+ // we add injection support to PackageFunction.
+}
