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 24221c9415eb79..8f228b661e6eeb 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 @@ -127,7 +127,9 @@ public static class Builder implements RuleDefinitionEnvironment { OptionsDiffPredicate.ALWAYS_INVALIDATE; private PrerequisiteValidator prerequisiteValidator; private final ImmutableList.Builder starlarkBootstraps = ImmutableList.builder(); - private ImmutableMap.Builder starlarkAccessibleTopLevels = + private final ImmutableMap.Builder starlarkAccessibleTopLevels = + ImmutableMap.builder(); + private final ImmutableMap.Builder starlarkBuiltinsInternals = ImmutableMap.builder(); private final ImmutableList.Builder symlinkDefinitions = ImmutableList.builder(); @@ -261,6 +263,11 @@ public Builder addStarlarkAccessibleTopLevels(String name, Object object) { return this; } + public Builder addStarlarkBuiltinsInternal(String name, Object object) { + this.starlarkBuiltinsInternals.put(name, object); + return this; + } + public Builder addSymlinkDefinition(SymlinkDefinition symlinkDefinition) { this.symlinkDefinitions.add(symlinkDefinition); return this; @@ -497,6 +504,7 @@ public ConfiguredRuleClassProvider build() { shouldInvalidateCacheForOptionDiff, prerequisiteValidator, starlarkAccessibleTopLevels.build(), + starlarkBuiltinsInternals.build(), starlarkBootstraps.build(), symlinkDefinitions.build(), ImmutableSet.copyOf(reservedActionMnemonics), @@ -586,6 +594,8 @@ public String getToolsRepository() { private final ImmutableMap nativeRuleSpecificBindings; + private final ImmutableMap starlarkBuiltinsInternals; + private final ImmutableMap environment; private final ImmutableList symlinkDefinitions; @@ -619,7 +629,8 @@ private ConfiguredRuleClassProvider( PatchTransition toolchainTaggedTrimmingTransition, OptionsDiffPredicate shouldInvalidateCacheForOptionDiff, PrerequisiteValidator prerequisiteValidator, - ImmutableMap starlarkAccessibleJavaClasses, + ImmutableMap starlarkAccessibleTopLevels, + ImmutableMap starlarkBuiltinsInternals, ImmutableList starlarkBootstraps, ImmutableList symlinkDefinitions, ImmutableSet reservedActionMnemonics, @@ -646,7 +657,8 @@ private ConfiguredRuleClassProvider( this.shouldInvalidateCacheForOptionDiff = shouldInvalidateCacheForOptionDiff; this.prerequisiteValidator = prerequisiteValidator; this.nativeRuleSpecificBindings = - createNativeRuleSpecificBindings(starlarkAccessibleJavaClasses, starlarkBootstraps); + createNativeRuleSpecificBindings(starlarkAccessibleTopLevels, starlarkBootstraps); + this.starlarkBuiltinsInternals = starlarkBuiltinsInternals; this.environment = createEnvironment(nativeRuleSpecificBindings); this.symlinkDefinitions = symlinkDefinitions; this.reservedActionMnemonics = reservedActionMnemonics; @@ -844,6 +856,11 @@ public ImmutableMap getNativeRuleSpecificBindings() { return nativeRuleSpecificBindings; } + @Override + public ImmutableMap getStarlarkBuiltinsInternals() { + return starlarkBuiltinsInternals; + } + @Override public ImmutableMap getEnvironment() { return environment; diff --git a/src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironment.java b/src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironment.java index be1c87fcbddaf6..4a6bcce7ed2e91 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironment.java @@ -14,12 +14,16 @@ package com.google.devtools.build.lib.packages; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; import java.util.HashMap; import java.util.List; import java.util.Map; +import net.starlark.java.eval.FlagGuardedValue; import net.starlark.java.eval.Starlark; +// TODO(adonovan): move skyframe.PackageFunction into lib.packages so we needn't expose this and +// the other env-building functions. /** * This class encapsulates knowledge of how to set up the Starlark environment for BUILD, WORKSPACE, * and bzl file evaluation, including the top-level predeclared symbols, the {@code native} module, @@ -31,12 +35,6 @@ * result of (1) is cached by an instance of this class. (2) is obtained using helper methods on * this class, and cached in {@link StarlarkBuiltinsValue}. */ -// TODO(#11437): To deal with StarlarkSemantics inspection via _builtins.flags, we can make it a -// function (not a struct field) that accesses the StarlarkThread. To deal with flag-guarded -// top-level symbols exposed under _builtins.toplevel, we can simply make them all accessible -// unconditionally, under the principle that the flag migrator should be responsible for deleting -// all uses from @_builtins code, and @_builtins code can be trusted to use flag-guarded features -// responsibly. public final class BazelStarlarkEnvironment { // TODO(#11954): Eventually the BUILD and WORKSPACE bzl dialects should converge. Right now they @@ -73,7 +71,9 @@ public final class BazelStarlarkEnvironment { this.uninjectedBuildBzlEnv = createUninjectedBuildBzlEnv(ruleClassProvider, uninjectedBuildBzlNativeBindings); this.workspaceBzlEnv = createWorkspaceBzlEnv(ruleClassProvider, workspaceBzlNativeBindings); - this.builtinsBzlEnv = createBuiltinsBzlEnv(ruleClassProvider); + this.builtinsBzlEnv = + createBuiltinsBzlEnv( + ruleClassProvider, uninjectedBuildBzlNativeBindings, uninjectedBuildBzlEnv); this.uninjectedBuildEnv = createUninjectedBuildEnv(ruleFunctions, packageFunction, environmentExtensions); } @@ -194,14 +194,43 @@ private static ImmutableMap createWorkspaceBzlEnv( } private static ImmutableMap createBuiltinsBzlEnv( - RuleClassProvider ruleClassProvider) { + RuleClassProvider ruleClassProvider, + ImmutableMap uninjectedBuildBzlNativeBindings, + ImmutableMap uninjectedBuildBzlEnv) { Map env = new HashMap<>(); env.putAll(ruleClassProvider.getEnvironment()); // Clear out rule-specific symbols like CcInfo. env.keySet().removeAll(ruleClassProvider.getNativeRuleSpecificBindings().keySet()); - env.put("_internal", InternalModule.INSTANCE); + // For _builtins.toplevel, replace all FlagGuardedValues with the underlying value; + // StarlarkSemantics flags do not affect @_builtins. + // + // We do this because otherwise we'd need to differentiate the _builtins.toplevel object (and + // therefore the @_builtins environment) based on StarlarkSemantics. That seems unnecessary. + // Instead we trust @_builtins to not misuse flag-guarded features, same as native code. + // + // If foo is flag-guarded (either experimental or incompatible), it is unconditionally visible + // as _builtins.toplevel.foo. It is legal to list it in exported_toplevels unconditionally, but + // the flag still controls whether the symbol is actually visible to user code. + Map unwrappedBuildBzlSymbols = new HashMap<>(); + for (Map.Entry entry : uninjectedBuildBzlEnv.entrySet()) { + Object symbol = entry.getValue(); + if (symbol instanceof FlagGuardedValue) { + symbol = ((FlagGuardedValue) symbol).getObject(); + } + unwrappedBuildBzlSymbols.put(entry.getKey(), symbol); + } + + Object builtinsModule = + new BuiltinsInternalModule( + createNativeModule(uninjectedBuildBzlNativeBindings), + // createNativeModule() is good enough for the "toplevel" and "internal" objects too. + createNativeModule(unwrappedBuildBzlSymbols), + createNativeModule(ruleClassProvider.getStarlarkBuiltinsInternals())); + Object conflictingValue = env.put("_builtins", builtinsModule); + Preconditions.checkState( + conflictingValue == null, "'_builtins' name is reserved for builtins injection"); return ImmutableMap.copyOf(env); } @@ -220,16 +249,6 @@ private static ImmutableMap createBuiltinsBzlEnv( public ImmutableMap createBuildBzlEnvUsingInjection( Map injectedToplevels, Map injectedRules) throws InjectionException { - // TODO(#11437): Builtins injection should take into account StarlarkSemantics and - // FlagGuardedValues. If a builtin is disabled by a flag, we can either: - // - // 1) Treat it as if it doesn't exist for the purposes of injection. In this case it's an - // error to attempt to inject it, so exports.bzl is required to explicitly check the flag's - // value (via the _internal module) before exporting it. - // - // 2) Allow it to be exported and automatically suppress/omit it from the final environment, - // effectively rewrapping the injected builtin in the FlagGuardedValue. - // Determine top-level symbols. Map env = new HashMap<>(); env.putAll(uninjectedBuildBzlEnv); @@ -276,8 +295,6 @@ public ImmutableMap createBuildBzlEnvUsingInjection( * overridden in this manner, not generic built-ins such as {@code package} or {@code glob}. * Throws InjectionException if these conditions are not met. */ - // TODO(adonovan): move skyframe.PackageFunction into lib.packages so we needn't expose this and - // the other env-building functions. public ImmutableMap createBuildEnvUsingInjection( Map injectedRules) throws InjectionException { HashMap env = new HashMap<>(uninjectedBuildEnv); diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuiltinsInternalModule.java b/src/main/java/com/google/devtools/build/lib/packages/BuiltinsInternalModule.java new file mode 100644 index 00000000000000..ed0bfcf5aa925d --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/packages/BuiltinsInternalModule.java @@ -0,0 +1,142 @@ +// 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.packages; + +import com.google.devtools.build.docgen.annot.DocCategory; +import net.starlark.java.annot.Param; +import net.starlark.java.annot.StarlarkBuiltin; +import net.starlark.java.annot.StarlarkMethod; +import net.starlark.java.eval.Printer; +import net.starlark.java.eval.Starlark; +import net.starlark.java.eval.StarlarkThread; +import net.starlark.java.eval.StarlarkValue; + +// TODO(#11437): Note that if Stardoc's current design were to be long-lived, we'd want to factor +// out an API into starlarkbuildapi. As it is that almost certainly won't be necessary. +/** + * The {@code _builtins} Starlark object, visible only to {@code @_builtins} .bzl files, supporting + * access to internal APIs. + */ +@StarlarkBuiltin( + name = "_builtins", + category = DocCategory.BUILTIN, + documented = false, + doc = + "A module accessible only to @_builtins .bzls, that permits access to the original " + + "(uninjected) native builtins, as well as internal-only symbols not accessible to " + + "users.") +public class BuiltinsInternalModule implements StarlarkValue { + + // _builtins.native + private final Object uninjectedNativeObject; + // _builtins.toplevel + private final Object uninjectedToplevelObject; + // _builtins.internal + private final Object internalObject; + + public BuiltinsInternalModule( + Object uninjectedNativeObject, Object uninjectedToplevelObject, Object internalObject) { + this.uninjectedNativeObject = uninjectedNativeObject; + this.uninjectedToplevelObject = uninjectedToplevelObject; + this.internalObject = internalObject; + } + + @Override + public void repr(Printer printer) { + printer.append("<_builtins module>"); + } + + @Override + public boolean isImmutable() { + return true; + } + + @StarlarkMethod( + name = "native", + doc = + "A view of the native object as it would exist if builtins injection were" + + " disabled. For example, if builtins injection provides a Starlark definition for" + + " cc_library in exported_rules, then" + + " native.cc_library in a user .bzl file would refer to that" + + " definition, but _builtins.native.cc_library in a" + + " @_builtins .bzl file would still be the one defined in Java code." + + " (Note that for clarity and to avoid a conceptual cycle, the regular top-level" + + " native object is not defined for @_builtins .bzl" + + " files.)", + documented = false, + structField = true) + public Object getUninjectedNativeObject() { + return uninjectedNativeObject; + } + + @StarlarkMethod( + name = "toplevel", + doc = + "A view of the top-level .bzl symbols that would exist if builtins injection were" + + " disabled; analogous to _builtins.native. For example, if builtins" + + " injection provides a Starlark definition for CcInfo in" + + " exported_toplevels, then _builtins.toplevel.CcInfo" + + " refers to the original Java definition, not the Starlark one. (Just as for" + + " _builtins.native, the top-level CcInfo symbol is not" + + " available to @_builtins .bzl files.)", + documented = false, + structField = true) + public Object getUninjectedToplevelObject() { + return uninjectedToplevelObject; + } + + @StarlarkMethod( + name = "internal", + doc = + "A view of symbols that were registered (via the Java method" + + "ConfiguredRuleClassProvider#addStarlarkBuiltinsInternal) to be made" + + " available to @_builtins code but not necessarily user code.", + documented = false, + structField = true) + public Object getInternalObject() { + return internalObject; + } + + @StarlarkMethod( + name = "get_flag", + doc = + "Returns the value of a StarlarkSemantics flag, or a default value if it" + + " could not be retrieved (either because the flag does not exist or because it was" + + " not assigned an explicit value). Fails if the flag value exists but is not a" + + " Starlark value.", + documented = false, + parameters = { + @Param(name = "name", doc = "Name of the flag, without the leading dashes"), + /* + * Because of the way flag values are stored in StarlarkSemantics, we cannot retrieve a flag + * whose value was not explicitly set, nor can we programmatically determine its default + * value. This parameter is essentially a hack to avoid a slightly costlier refactoring of + * StarlarkSemantics and BuildLanguageOptions. If the value passed in here by the caller + * differs from the true default value of the flag, you could end up in a situation where + * the semantics of some @_builtins code varies depending on whether a flag was set to its + * default value implicitly or explicitly. + */ + @Param( + name = "default", + doc = + "Value to return if flag was not set or does not exist. This should always be set" + + " to the same value as the flag's default value.") + }, + useStarlarkThread = true) + public Object getFlag(String name, Object defaultValue, StarlarkThread thread) { + Object value = thread.getSemantics().getGeneric(name, defaultValue); + return Starlark.fromJava(value, thread.mutability()); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/packages/InternalModule.java b/src/main/java/com/google/devtools/build/lib/packages/InternalModule.java deleted file mode 100644 index 7e721bdcdf2dc5..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/packages/InternalModule.java +++ /dev/null @@ -1,49 +0,0 @@ -// 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.packages; - -import com.google.devtools.build.docgen.annot.DocCategory; -import net.starlark.java.annot.StarlarkBuiltin; -import net.starlark.java.eval.Printer; -import net.starlark.java.eval.StarlarkValue; - -// TODO(#11437): Factor an API out into skylarkbuildapi, for stardoc's benefit. Otherwise, stardoc -// can't run on @_builtins bzls. -/** The {@code _internal} Starlark object, visible only to {@code @_builtins} .bzls. */ -@StarlarkBuiltin( - name = "_internal", - category = DocCategory.BUILTIN, - documented = false, - doc = - "A module accessible only to @_builtins .bzls, that permits access to the original " - + "(uninjected) native builtins, as well as internal-only symbols not accessible to " - + "users.") -public class InternalModule implements StarlarkValue { - - // TODO(#11437): Can't use a singleton once we're re-exporting fields of the native object. - public static final InternalModule INSTANCE = new InternalModule(); - - private InternalModule() {} - - @Override - public void repr(Printer printer) { - printer.append("<_internal module>"); - } - - @Override - public boolean isImmutable() { - return true; - } -} diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClassProvider.java index c1bc890d59b6d4..3a7e22f713f6c5 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClassProvider.java @@ -82,6 +82,15 @@ void setStarlarkThreadContext( */ ImmutableMap getNativeRuleSpecificBindings(); + /** + * Returns the set of symbols to be made available to {@code @_builtins} .bzl files under the + * _builtins.internal object. + * + *

These symbols are not exposed to user .bzl code and do not constitute a public or stable API + * (unless exposed through another means). + */ + ImmutableMap getStarlarkBuiltinsInternals(); + /** * Returns the Starlark builtins registered with this RuleClassProvider. * diff --git a/src/main/java/net/starlark/java/eval/StarlarkSemantics.java b/src/main/java/net/starlark/java/eval/StarlarkSemantics.java index f4e0b55a055289..3e937b92dda5b8 100644 --- a/src/main/java/net/starlark/java/eval/StarlarkSemantics.java +++ b/src/main/java/net/starlark/java/eval/StarlarkSemantics.java @@ -72,6 +72,25 @@ public T get(Key key) { return v != null ? v : key.defaultValue; } + // TODO(bazel-team): This exists solely for BuiltinsInternalModule#getFlag, which allows a + // (privileged) Starlark caller to programmatically retrieve a flag's value without knowing its + // schema and default value. Reconsider whether we should support that use case from this class. + /** + * Returns the value of the option with the given name, or the default value if it is not set or + * does not exist. + */ + public Object getGeneric(String name, Object defaultValue) { + Object v = map.get(name); + // Try boolean prefixes if that didn't work. + if (v == null) { + v = map.get("+" + name); + } + if (v == null) { + v = map.get("-" + name); + } + return v != null ? v : defaultValue; + } + /** A Key identifies an option, providing its name, type, and default value. */ public static class Key { public final String name; diff --git a/src/test/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironmentTest.java b/src/test/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironmentTest.java index e03af2eabfcd69..8651afa4efd6ea 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironmentTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironmentTest.java @@ -76,14 +76,14 @@ public void buildAndWorkspaceBzlEnvsAreSameExceptForNative() throws Exception { } @Test - public void builtinsBzlEnvCanSeeGeneralToplevels() throws Exception { - assertThat(pkgFactory.getBazelStarlarkEnvironment().getBuiltinsBzlEnv()).containsKey("rule"); - } - - @Test - public void builtinsBzlEnvCannotSeeRuleSpecificToplevels() throws Exception { - assertThat(pkgFactory.getBazelStarlarkEnvironment().getBuiltinsBzlEnv()) - .doesNotContainKey("overridable_symbol"); + public void builtinsBzlEnv() throws Exception { + ImmutableMap env = pkgFactory.getBazelStarlarkEnvironment().getBuiltinsBzlEnv(); + // Can see general toplevel symbols. + assertThat(env).containsKey("rule"); + // Cannot see rule-specific toplevel symbols. + assertThat(env).doesNotContainKey("overridable_symbol"); + // Has the special builtins-internal module. + assertThat(env).containsKey("_builtins"); } /** diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD index bc02a34acc2711..e75cbd6ac8e304 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD @@ -18,6 +18,11 @@ CROSS_PLATFORM_WINDOWS_TESTS = [ "PathCasingLookupFunctionTest.java", ] +# Tests that are broken out from the SkyframeTests target into separate targets. +EXCLUDED_FROM_SKYFRAME_TESTS = [ + "PrepareDepsOfTargetsUnderDirectoryFunctionTest.java", # b/179148968 +] + CROSS_PLATFORM_WINDOWS_TESTS + java_library( name = "testutil", srcs = glob([ @@ -71,11 +76,11 @@ java_test( srcs = select({ "//src/conditions:darwin": glob( ["*.java"], - exclude = CROSS_PLATFORM_WINDOWS_TESTS, + exclude = EXCLUDED_FROM_SKYFRAME_TESTS, ), "//conditions:default": glob( ["*.java"], - exclude = ["MacOSXFsEventsDiffAwarenessTest.java"] + CROSS_PLATFORM_WINDOWS_TESTS, + exclude = ["MacOSXFsEventsDiffAwarenessTest.java"] + EXCLUDED_FROM_SKYFRAME_TESTS, ), }), exec_compatible_with = ["//:highcpu_machine"], @@ -351,3 +356,27 @@ test_suite( ], visibility = ["//src/test/java/com/google/devtools/build/lib:__pkg__"], ) + +# TODO(b/179148968): This used to be part of SkyframeTests but was broken off because it has some +# non-hermetic interaction with another test, depending on how the tests get sharded. +java_test( + name = "PrepareDepsOfTargetsUnderDirectoryFunctionTest", + srcs = ["PrepareDepsOfTargetsUnderDirectoryFunctionTest.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/pkgcache", + "//src/main/java/com/google/devtools/build/lib/skyframe:collect_packages_under_directory_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:prepare_deps_of_targets_under_directory_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster", + "//src/main/java/com/google/devtools/build/lib/skyframe:transitive_traversal_value", + "//src/main/java/com/google/devtools/build/lib/vfs", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", + "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + "//src/test/java/com/google/devtools/build/lib/analysis/util", + "//src/test/java/com/google/devtools/build/skyframe:testutil", + "//third_party:guava", + "//third_party:junit4", + "//third_party:truth", + ], +) 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 index d3bd5c58f137bb..c6149389b63469 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BuiltinsInjectionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BuiltinsInjectionTest.java @@ -197,7 +197,8 @@ protected ConfiguredRuleClassProvider createRuleClassProvider() { // For this mock symbol, we reuse the same flag that guards the production // _builtins_dummy symbol. FlagGuardedValue.onlyWhenExperimentalFlagIsTrue( - BuildLanguageOptions.EXPERIMENTAL_BUILTINS_DUMMY, "original value")); + BuildLanguageOptions.EXPERIMENTAL_BUILTINS_DUMMY, "original_value")) + .addStarlarkBuiltinsInternal("internal_symbol", "internal_value"); return builder.build(); } @@ -206,13 +207,19 @@ public void setUp() throws Exception { setBuildLanguageOptions("--experimental_builtins_bzl_path=tools/builtins_staging"); } + public void setBuiltinsDummyFlag() throws Exception { + setBuildLanguageOptions( + "--experimental_builtins_bzl_path=tools/builtins_staging", + "--experimental_builtins_dummy=true"); + } + /** * Writes an exports.bzl file with the given content, in the builtins location. * *

See {@link StarlarkBuiltinsFunction#EXPORTS_ENTRYPOINT} for the significance of exports.bzl. */ private void writeExportsBzl(String... lines) throws Exception { - scratch.file("tools/builtins_staging/exports.bzl", lines); + scratch.overwriteFile("tools/builtins_staging/exports.bzl", lines); } /** @@ -227,7 +234,7 @@ private void writePkgBzl(String... lines) throws Exception { modifiedLines.add("dummy_symbol = None"); // The marker phrase might not be needed, but I don't entirely trust BuildViewTestCase. modifiedLines.add("print('dummy.bzl evaluation completed')"); - scratch.file("pkg/dummy.bzl", modifiedLines.toArray(lines)); + scratch.overwriteFile("pkg/dummy.bzl", modifiedLines.toArray(lines)); } /** @@ -239,7 +246,7 @@ private void writePkgBzl(String... lines) throws Exception { private void writePkgBuild(String... lines) throws Exception { List modifiedLines = new ArrayList<>(Arrays.asList(lines)); modifiedLines.add(0, "load(':dummy.bzl', 'dummy_symbol')"); - scratch.file("pkg/BUILD", modifiedLines.toArray(lines)); + scratch.overwriteFile("pkg/BUILD", modifiedLines.toArray(lines)); } /** Builds {@code //pkg} and asserts success, including that the marker print() event occurs. */ @@ -265,10 +272,10 @@ public void basicFunctionality() throws Exception { "exported_toplevels = {'overridable_symbol': 'new_value'}", "exported_rules = {'overridable_rule': 'new_rule'}", "exported_to_java = {}"); - writePkgBuild("print('In BUILD: overridable_rule :: ' + str(overridable_rule))"); + writePkgBuild("print('In BUILD: overridable_rule :: %s' % overridable_rule)"); writePkgBzl( - "print('In bzl: overridable_symbol :: ' + str(overridable_symbol))", - "print('In bzl: overridable_rule :: ' + str(native.overridable_rule))"); + "print('In bzl: overridable_symbol :: %s' % overridable_symbol)", + "print('In bzl: overridable_rule :: %s' % native.overridable_rule)"); buildAndAssertSuccess(); assertContainsEvent("In bzl: overridable_symbol :: new_value"); @@ -294,14 +301,14 @@ public void injectedBzlToplevelsAreNotVisibleToBuild() throws Exception { @Test public void builtinsCanLoadFromBuiltins() throws Exception { // Define a few files that we can load with different kinds of label syntax. In each case, - // access the `_internal` symbol to demonstrate that we're being loaded as a builtins bzl. + // access the `_builtins` symbol to demonstrate that we're being loaded as a builtins bzl. scratch.file( "tools/builtins_staging/absolute.bzl", // - "_internal", + "_builtins", "a = 'A'"); scratch.file( "tools/builtins_staging/repo_relative.bzl", // - "_internal", + "_builtins", "b = 'B'"); scratch.file( "tools/builtins_staging/subdir/pkg_relative1.bzl", // @@ -309,11 +316,11 @@ public void builtinsCanLoadFromBuiltins() throws Exception { // root, and not relative to the file. That is, we specify 'subdir/pkg_relative2.bzl', not // just 'pkg_relative2.bzl'. "load('subdir/pkg_relative2.bzl', 'c2')", - "_internal", + "_builtins", "c = c2"); scratch.file( "tools/builtins_staging/subdir/pkg_relative2.bzl", // - "_internal", + "_builtins", "c2 = 'C'"); // Also create a file in the main repo whose package path coincides with a file in the builtins @@ -329,7 +336,7 @@ public void builtinsCanLoadFromBuiltins() throws Exception { "exported_rules = {}", "exported_to_java = {}"); writePkgBuild(); - writePkgBzl("print('overridable_symbol :: ' + str(overridable_symbol))"); + writePkgBzl("print('overridable_symbol :: %s' % overridable_symbol)"); buildAndAssertSuccess(); assertContainsEvent("overridable_symbol :: ABC"); @@ -447,10 +454,10 @@ public void injectionDisabledByFlag() throws Exception { "exported_toplevels = {'overridable_symbol': 'new_value'}", "exported_rules = {'overridable_rule': 'new_rule'}", "exported_to_java = {}"); - writePkgBuild("print('In BUILD: overridable_rule :: ' + str(overridable_rule))"); + writePkgBuild("print('In BUILD: overridable_rule :: %s' % overridable_rule)"); writePkgBzl( - "print('In bzl: overridable_symbol :: ' + str(overridable_symbol))", - "print('In bzl: overridable_rule :: ' + str(native.overridable_rule))"); + "print('In bzl: overridable_symbol :: %s' % overridable_symbol)", + "print('In bzl: overridable_rule :: %s' % native.overridable_rule)"); setBuildLanguageOptions("--experimental_builtins_bzl_path="); buildAndAssertSuccess(); @@ -464,10 +471,10 @@ public void injectionDisabledByFlag() throws Exception { public void exportsBzlMayBeInErrorWhenInjectionIsDisabled() throws Exception { writeExportsBzl( // "PARSE ERROR"); - writePkgBuild("print('In BUILD: overridable_rule :: ' + str(overridable_rule))"); + writePkgBuild("print('In BUILD: overridable_rule :: %s' % overridable_rule)"); writePkgBzl( - "print('In bzl: overridable_symbol :: ' + str(overridable_symbol))", - "print('In bzl: overridable_rule :: ' + str(native.overridable_rule))"); + "print('In bzl: overridable_symbol :: %s' % overridable_symbol)", + "print('In bzl: overridable_rule :: %s' % native.overridable_rule)"); setBuildLanguageOptions("--experimental_builtins_bzl_path="); buildAndAssertSuccess(); @@ -492,13 +499,13 @@ public void workspaceAndWorkspaceBzlDoNotUseInjection() throws Exception { scratch.appendFile( "WORKSPACE", // "load(':foo.bzl', 'dummy_symbol')", - "print('In WORKSPACE: overridable_rule :: ' + str(overridable_rule))", + "print('In WORKSPACE: overridable_rule :: %s' % overridable_rule)", "print(dummy_symbol)"); scratch.file("BUILD"); scratch.file( "foo.bzl", "dummy_symbol = None", - "print('In bzl: overridable_symbol :: ' + str(overridable_symbol))"); + "print('In bzl: overridable_symbol :: %s' % overridable_symbol)"); buildAndAssertSuccess(); // Builtins for WORKSPACE bzls are populated essentially the same as for BUILD bzls, except that @@ -509,26 +516,168 @@ public void workspaceAndWorkspaceBzlDoNotUseInjection() throws Exception { assertContainsEvent("In WORKSPACE: overridable_rule :: "); } - // TODO(#11437): Add tests of the _internal symbol's usage within builtins bzls. + @Test + public void builtinsCanSeeOriginalNativeToplevels() throws Exception { + writeExportsBzl( + "print('In builtins: overridable_symbol :: %s' % _builtins.toplevel.overridable_symbol)", + "exported_toplevels = {'overridable_symbol': 'new_value'}", + "exported_rules = {}", + "exported_to_java = {}"); + writePkgBuild(); + writePkgBzl("print('In bzl: overridable_symbol :: %s' % overridable_symbol)"); + + buildAndAssertSuccess(); + assertContainsEvent("In builtins: overridable_symbol :: original_value"); + assertContainsEvent("In bzl: overridable_symbol :: new_value"); + } + + @Test + public void builtinsCanSeeOriginalNativeRules() throws Exception { + writeExportsBzl( + "print('In builtins: overridable_rule :: %s' % _builtins.native.overridable_rule)", + "exported_toplevels = {}", + "exported_rules = {'overridable_rule': 'new_rule'}", + "exported_to_java = {}"); + writePkgBuild(); + writePkgBzl("print('In bzl: overridable_rule :: %s' % native.overridable_rule)"); + + buildAndAssertSuccess(); + assertContainsEvent("In builtins: overridable_rule :: "); + assertContainsEvent("In bzl: overridable_rule :: new_rule"); + } + + @Test + public void builtinsCanSeeBuiltinsInternalSymbol() throws Exception { + writeExportsBzl( + "print('internal_symbol :: %s' % _builtins.internal.internal_symbol)", + "exported_toplevels = {}", // + "exported_rules = {}", + "exported_to_java = {}"); + writePkgBuild(); + writePkgBzl(); + + buildAndAssertSuccess(); + assertContainsEvent("internal_symbol :: internal_value"); + } + + @Test + public void otherBzlsCannotSeeBuiltinsInternalSymbol() throws Exception { + writeExportsBzl( + "exported_toplevels = {}", // + "exported_rules = {}", + "exported_to_java = {}"); + writePkgBuild(); + writePkgBzl("internal_symbol"); + + buildAndAssertFailure(); + assertContainsEvent("name 'internal_symbol' is not defined"); + } + + @Test + public void builtinsCanSeeFlags_unset() throws Exception { + writeExportsBzl( + // We use a None default here, but note that that's brittle if any machinery explicitly sets + // flags to their default values. In practice the flag's real default value should be used. + "print('experimental_builtins_dummy :: %s' % ", + " _builtins.get_flag('experimental_builtins_dummy', None))", + "exported_toplevels = {}", // + "exported_rules = {}", + "exported_to_java = {}"); + writePkgBuild(); + writePkgBzl(); + + buildAndAssertSuccess(); + assertContainsEvent("experimental_builtins_dummy :: None"); + } + + @Test + public void builtinsCanSeeFlags_set() throws Exception { + writeExportsBzl( + "print('experimental_builtins_dummy :: %s' % ", + " _builtins.get_flag('experimental_builtins_dummy', None))", + "exported_toplevels = {}", // + "exported_rules = {}", + "exported_to_java = {}"); + writePkgBuild(); + writePkgBzl(); + + setBuiltinsDummyFlag(); + buildAndAssertSuccess(); + assertContainsEvent("experimental_builtins_dummy :: True"); + } + + @Test + public void builtinsCanSeeFlags_doesNotExist() throws Exception { + writeExportsBzl( + // We use a None default here, but note that that's brittle if any machinery explicitly sets + // flags to their default values. In practice the flag's real default value should be used. + "print('experimental_does_not_exist :: %s' % ", + " _builtins.get_flag('experimental_does_not_exist', None))", + "exported_toplevels = {}", // + "exported_rules = {}", + "exported_to_java = {}"); + writePkgBuild(); + writePkgBzl(); + + buildAndAssertSuccess(); + assertContainsEvent("experimental_does_not_exist :: None"); + } + + @Test + public void flagGuarding_canExportEvenWhenDisabled() throws Exception { + writeExportsBzl( + "exported_toplevels = {'flag_guarded_symbol': 'overridden value'}", + "exported_rules = {}", + "exported_to_java = {}"); + writePkgBuild(); + writePkgBzl(); + + // Default value of --experimental_builtins_dummy is false. + buildAndAssertSuccess(); + } @Test - public void overriddenSymbolsAreStillFlagGuarded() throws Exception { - // Implementation note: This works not because of any special handling in the builtins logic, - // but rather because flag guarding is implemented at name resolution time, before builtins + public void flagGuarding_cannotUseWhenDisabledEvenIfInjected() throws Exception { + // Implementation note: Flag guarding is implemented at name resolution time, before builtins // injection is applied. writeExportsBzl( "exported_toplevels = {'flag_guarded_symbol': 'overridden value'}", "exported_rules = {}", "exported_to_java = {}"); writePkgBuild(); - writePkgBzl("flag_guarded_symbol"); + writePkgBzl("print('flag_guarded_symbol :: %s' % flag_guarded_symbol)"); buildAndAssertFailure(); assertContainsEvent("flag_guarded_symbol is experimental"); } - // TODO(#11437): Once we allow access to native symbols via _internal, verify that flag guarding - // works correctly within builtins. + @Test + public void flagGuarding_injectedValueIsSeenWhenFlagIsEnabled() throws Exception { + writeExportsBzl( + "exported_toplevels = {'flag_guarded_symbol': 'overridden value'}", + "exported_rules = {}", + "exported_to_java = {}"); + writePkgBuild(); + writePkgBzl("print('flag_guarded_symbol :: %s' % flag_guarded_symbol)"); + + setBuiltinsDummyFlag(); + buildAndAssertSuccess(); + assertContainsEvent("flag_guarded_symbol :: overridden value"); + } + + @Test + public void flagGuarding_unconditionallyAccessibleToBuiltins() throws Exception { + writeExportsBzl( + "print('flag_guarded_symbol :: %s' % _builtins.toplevel.flag_guarded_symbol)", + "exported_toplevels = {}", // + "exported_rules = {}", + "exported_to_java = {}"); + writePkgBuild(); + writePkgBzl(); + + buildAndAssertSuccess(); + assertContainsEvent("flag_guarded_symbol :: original_value"); + } @Test public void nativeRulesCanUseSymbolsFromBuiltins() throws Exception { diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunctionTest.java index 097429fb9f4843..f262f11a0df2df 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunctionTest.java @@ -216,30 +216,30 @@ public void builtinsBzlCannotAccessRuleSpecificSymbol() throws Exception { } @Test - public void builtinsBzlCanAccessInternal() throws Exception { + public void builtinsBzlCanAccessBuiltinsInternalModule() throws Exception { EvaluationResult result = evalBuiltins( - "print(_internal)", + "print(_builtins)", "", "exported_toplevels = {}", "exported_rules = {}", "exported_to_java = {}"); assertThatEvaluationResult(result).hasNoError(); - assertContainsEvent("<_internal module>"); + assertContainsEvent("<_builtins module>"); } @Test - public void regularBzlCannotAccessInternal() throws Exception { + public void regularBzlCannotAccessBuiltinsInternalModule() throws Exception { scratch.file( "pkg/BUILD", // "load(':dummy.bzl', 'dummy_symbol')"); scratch.file( "pkg/dummy.bzl", // - "_internal", + "_builtins", "dummy_symbol = None"); reporter.removeHandler(failFastHandler); getConfiguredTarget("//pkg:BUILD"); - assertContainsEvent("name '_internal' is not defined"); + assertContainsEvent("name '_builtins' is not defined"); } }