From a0cd355347b57b17f28695a84af168f9fd200ba1 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 1 May 2023 13:43:28 -0700 Subject: [PATCH] Add support for loading .scl files The Starlark Configuration Language (SCL) is a dialect of Starlark that resembles pure Starlark with just a few Bazel symbols (namely `visibility()` and `struct`), and a few restrictions on `load()` syntax and non-ASCII string data. This CL adds support for loading .scl files anywhere a .bzl could be used. The restrictions on `load()` are implemented in this CL, but follow-up CLs will handle differentiating the top-level environment from .bzl, restricting non-ASCII data, and adding user documentation. The dialect is gated on the flag `--experimental_enable_scl_dialect`. - `BzlLoadValue` gains a new bool field on its abstract Key type to indicate whether it is for .scl. This was chosen instead of creating a new Key subclass so that .scl can work equally well in different contexts (loaded by BUILD, WORKSPACE, MODULE.bazel, or even @_builtins). - `BzlLoadFunction.checkValidLoadLabel` and `.getLoadLabels` are both split into a public and private method. The public methods assume the load is coming from a .bzl file for validation purposes, and take a `StarlarkSemantics` to check whether the experimental flag is enabled. - Eliminate `BzlLoadFunction.getBUILDLabel()` helper, which is obsolete. - Modify existing tests to incidentally check that bzlmod .bzls can load .scl files and that .scl files can appear in transitive loads as reported by the Package (for `loadfiles()`). RELNOTES: None PiperOrigin-RevId: 528563228 Change-Id: I8493d1f33d35e1af8003dc61e5fdb626676d7e53 --- .../bzlmod/SingleExtensionEvalFunction.java | 3 +- .../semantics/BuildLanguageOptions.java | 11 ++ .../build/lib/skyframe/BzlLoadFunction.java | 152 +++++++++++++++--- .../build/lib/skyframe/BzlLoadValue.java | 22 ++- .../lib/skyframe/BzlmodRepoRuleFunction.java | 3 +- .../build/lib/skyframe/PackageFunction.java | 6 +- .../lib/skyframe/WorkspaceFileFunction.java | 3 +- .../lib/skyframe/BzlLoadFunctionTest.java | 102 +++++++++++- .../lib/skyframe/PackageFunctionTest.java | 10 +- 9 files changed, 274 insertions(+), 38 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java index 74f9f63f629bd1..222eb67bf9dbff 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java @@ -111,8 +111,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) // Check that the .bzl label isn't crazy. try { - BzlLoadFunction.checkValidLoadLabel( - extensionId.getBzlFileLabel(), /*fromBuiltinsRepo=*/ false); + BzlLoadFunction.checkValidLoadLabel(extensionId.getBzlFileLabel(), starlarkSemantics); } catch (LabelSyntaxException e) { throw new SingleExtensionEvalFunctionException( ExternalDepsException.withCauseAndMessage( diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java index 4cf7d122d5065a..2641aa3fdee03c 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java @@ -196,6 +196,15 @@ public final class BuildLanguageOptions extends OptionsBase { help = "If set to true, enables the APIs required to support the Android Starlark migration.") public boolean experimentalEnableAndroidMigrationApis; + @Option( + name = "experimental_enable_scl_dialect", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = OptionEffectTag.BUILD_FILE_SEMANTICS, + // TODO(brandjon): point to more extensive user documentation somewhere + help = "If set to true, .scl files may be used in load() statements.") + public boolean experimentalEnableSclDialect; + @Option( name = "enable_bzlmod", oldName = "experimental_enable_bzlmod", @@ -677,6 +686,7 @@ public StarlarkSemantics toStarlarkSemantics() { .setBool(CHECK_BZL_VISIBILITY, checkBzlVisibility) .setBool( EXPERIMENTAL_ENABLE_ANDROID_MIGRATION_APIS, experimentalEnableAndroidMigrationApis) + .setBool(EXPERIMENTAL_ENABLE_SCL_DIALECT, experimentalEnableSclDialect) .setBool(ENABLE_BZLMOD, enableBzlmod) .setBool( EXPERIMENTAL_JAVA_PROTO_LIBRARY_DEFAULT_HAS_SERVICES, @@ -770,6 +780,7 @@ public StarlarkSemantics toStarlarkSemantics() { "-experimental_disable_external_package"; public static final String EXPERIMENTAL_ENABLE_ANDROID_MIGRATION_APIS = "-experimental_enable_android_migration_apis"; + public static final String EXPERIMENTAL_ENABLE_SCL_DIALECT = "-experimental_enable_scl_dialect"; public static final String ENABLE_BZLMOD = "-enable_bzlmod"; public static final String EXPERIMENTAL_JAVA_PROTO_LIBRARY_DEFAULT_HAS_SERVICES = "+experimental_java_proto_library_default_has_services"; 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 a9e7b0e295e866..f6012b04939cfc 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 @@ -86,7 +86,11 @@ import net.starlark.java.syntax.StringLiteral; /** - * A Skyframe function to look up and load a single .bzl module. + * A Skyframe function to look up and load a single .bzl (or .scl) module. + * + *

Note: Historically, all modules had the .bzl suffix, but this is no longer true now that Bazel + * supports the .scl dialect. In identifiers, code comments, and documentation, you should generally + * assume any "bzl" term could mean a .scl file as well. * *

Given a {@link Label} referencing a .bzl file, attempts to locate the file and load it. The * Label must be absolute, and must not reference the special {@code external} package. If loading @@ -736,6 +740,14 @@ private BzlLoadValue computeInternalWithCompiledBzl( Label label = key.getLabel(); PackageIdentifier pkg = label.getPackageIdentifier(); + boolean isSclFlagEnabled = + builtins.starlarkSemantics.getBool(BuildLanguageOptions.EXPERIMENTAL_ENABLE_SCL_DIALECT); + if (key.isSclDialect() && !isSclFlagEnabled) { + throw new BzlLoadFailedException( + "loading .scl files requires setting --experimental_enable_scl_dialect", + Code.PARSE_ERROR); + } + // Determine dependency BzlLoadValue keys for the load statements in this bzl. // Labels are resolved relative to the current repo mapping. RepositoryMapping repoMapping = getRepositoryMapping(key, builtins.starlarkSemantics, env); @@ -744,7 +756,13 @@ private BzlLoadValue computeInternalWithCompiledBzl( } ImmutableList> programLoads = getLoadsFromProgram(prog); ImmutableList

Different restrictions apply depending on what type of source file the load appears in. For + * all kinds of files, {@code label}: + * + *

+ * + *

For source files appearing within {@code @_builtins}, {@code label} must also be within + * {@code @_builtins}. (The reverse, that those files may not be loaded by user-defined files, is + * enforced by the fact that the {@code @_builtins} pseudorepo cannot be resolved as an ordinary + * repo.) + * + *

For .scl files only, {@code label} must end with {@code .scl} (not {@code .bzl}). (Loads in + * .scl also should always begin with {@code //}, but that's syntactic and can't be enforced in + * this method.) + * + * @param label the label to validate + * @param fromBuiltinsRepo true if the file containing the load is within {@code @_builtins} + * @param withinSclDialect true if the file containing the load is a .scl file + * @param mentionSclInErrorMessage true if ".scl" should be advertised as a possible extension in + * error messaging + */ + private static void checkValidLoadLabel( + Label label, + boolean fromBuiltinsRepo, + boolean withinSclDialect, + boolean mentionSclInErrorMessage) throws LabelSyntaxException { - if (!label.getName().endsWith(".bzl")) { - throw new LabelSyntaxException("The label must reference a file with extension '.bzl'"); + // Check file extension. + String baseName = label.getName(); + if (withinSclDialect) { + if (!baseName.endsWith(".scl")) { + String msg = "The label must reference a file with extension \".scl\""; + if (baseName.endsWith(".bzl")) { + msg += " (.scl files cannot load .bzl files)"; + } + throw new LabelSyntaxException(msg); + } + } else { + if (!(baseName.endsWith(".scl") || baseName.endsWith(".bzl"))) { + String msg = "The label must reference a file with extension \".bzl\""; + if (mentionSclInErrorMessage) { + msg += " or \".scl\""; + } + throw new LabelSyntaxException(msg); + } } + if (label.getPackageIdentifier().equals(LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER)) { throw new LabelSyntaxException( "Starlark files may not be loaded from the //external package"); @@ -948,35 +1014,57 @@ public static void checkValidLoadLabel(Label label, boolean fromBuiltinsRepo) } } + /** + * Validates a label appearing in a {@code load()} statement, throwing {@link + * LabelSyntaxException} on failure. + */ + public static void checkValidLoadLabel(Label label, StarlarkSemantics starlarkSemantics) + throws LabelSyntaxException { + checkValidLoadLabel( + label, + /* fromBuiltinsRepo= */ false, + /* withinSclDialect= */ false, + /* mentionSclInErrorMessage= */ starlarkSemantics.getBool( + BuildLanguageOptions.EXPERIMENTAL_ENABLE_SCL_DIALECT)); + } + /** * Given a list of {@code load("module")} strings and their locations, in source order, returns a * corresponding list of Labels they each resolve to. Labels are resolved relative to {@code * base}, the file's package. If any label is malformed, the function reports one or more errors * to the handler and returns null. + * + *

If {@code withinSclDialect} is true, the labels are validated according to the rules of the + * .scl dialect: Only strings beginning with {@code //} are allowed (no repo syntax, no relative + * labels), and only .scl files may be loaded (not .bzl). If {@code isSclFlagEnabled} is true, + * then ".scl" is mentioned as a possible file extension in error messages. */ @Nullable - static ImmutableList

The key consists of an absolute {@link Label} and the context in which the load occurs. The * Label should not reference the special {@code external} package. @@ -95,6 +99,22 @@ boolean isBuiltins() { return false; } + /** Returns true if the requested file follows the .scl dialect. */ + // Note: Just as with .bzl, the same .scl file can be referred to from multiple key types, for + // instance if a BUILD file and a module rule both load foo.scl. Conceptually, .scl files + // shouldn't depend on what kind of top-level file caused them to load, but in practice, this + // implementation quirk means that the .scl file will be loaded twice as separate copies. + // + // This shouldn't matter except in rare edge cases, such as if a Starlark function is loaded + // from both copies and compared for equality. Performance wise, it also means that all + // transitive .scl files will be double-loaded, but we don't expect that to be significant. + // + // The alternative is to use a separate key type just for .scl, but that complicates repo logic; + // see BzlLoadFunction#getRepositoryMapping. + final boolean isSclDialect() { + return getLabel().getName().endsWith(".scl"); + } + /** * Constructs a new key suitable for evaluating a {@code load()} dependency of this key's .bzl * file. diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java index a7c5c48e0357b4..2e185816a13415 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java @@ -220,7 +220,8 @@ private ImmutableMap loadBzlModules( env.getListener(), programLoads, PackageIdentifier.EMPTY_PACKAGE_ID, - EMPTY_MAIN_REPO_MAPPING); + EMPTY_MAIN_REPO_MAPPING, + starlarkSemantics); if (loadLabels == null) { NoSuchPackageException e = PackageFunction.PackageFunctionException.builder() diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java index ebeeccdb2fc566..c0ae3f70aceb69 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java @@ -1288,7 +1288,11 @@ private LoadedPackage loadPackage( BzlLoadFunction.getLoadsFromProgram(compiled.prog); ImmutableList