From daadadb7c66aa558797e40434c923e5c31b8ab5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Thu, 28 Mar 2024 22:09:13 -0400 Subject: [PATCH] Add a new `module_import()` directive to MODULE.bazel files This new directive allows the root module to divide its MODULE.bazel into multiple segments. TODO: add more context RELNOTES: Added a new `module_import()` directive to MODULE.bazel files. TODO: elaborate a bit more Fixes https://github.com/bazelbuild/bazel/issues/17880. --- .../devtools/build/lib/bazel/bzlmod/BUILD | 5 + .../bazel/bzlmod/BazelModTidyFunction.java | 7 + .../lib/bazel/bzlmod/BazelModTidyValue.java | 4 + .../lib/bazel/bzlmod/CompiledModuleFile.java | 117 +++++++ .../lib/bazel/bzlmod/ModuleFileFunction.java | 310 +++++++++++++----- .../lib/bazel/bzlmod/ModuleFileGlobals.java | 16 +- .../lib/bazel/bzlmod/ModuleFileValue.java | 17 +- .../lib/bazel/bzlmod/ModuleThreadContext.java | 23 +- .../devtools/build/lib/bazel/commands/BUILD | 1 + .../build/lib/bazel/commands/ModCommand.java | 24 +- .../bazel/bzlmod/CompiledModuleFileTest.java | 139 ++++++++ .../bazel/bzlmod/ModuleFileFunctionTest.java | 214 +++++++++++- src/test/py/bazel/bzlmod/bazel_module_test.py | 33 ++ 13 files changed, 820 insertions(+), 90 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/bazel/bzlmod/CompiledModuleFile.java create mode 100644 src/test/java/com/google/devtools/build/lib/bazel/bzlmod/CompiledModuleFileTest.java diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD index 4887a5a5ca0bb0..26e244f7774146 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD @@ -124,6 +124,7 @@ java_library( "BazelModuleResolutionEvent.java", "BazelModuleResolutionValue.java", "BzlmodFlagsAndEnvVars.java", + "CompiledModuleFile.java", "GitOverride.java", "InterimModule.java", "LocalPathOverride.java", @@ -148,6 +149,7 @@ java_library( ], deps = [ ":common", + ":exception", ":inspection", ":module_extension", ":module_extension_metadata", @@ -168,6 +170,7 @@ java_library( "//src/main/java/net/starlark/java/annot", "//src/main/java/net/starlark/java/eval", "//src/main/java/net/starlark/java/syntax", + "//src/main/protobuf:failure_details_java_proto", "//third_party:auto_value", "//third_party:gson", "//third_party:guava", @@ -229,6 +232,8 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_value", "//src/main/java/com/google/devtools/build/lib/skyframe:client_environment_function", "//src/main/java/com/google/devtools/build/lib/skyframe:client_environment_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_function", + "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_value", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value", "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java index 6cdf50a1374476..682662eca8b9a4 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java @@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue.RootModuleFileValue; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.RepositoryName; @@ -50,6 +51,11 @@ public class BazelModTidyFunction implements SkyFunction { @Nullable public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException, SkyFunctionException { + RootModuleFileValue rootModuleFileValue = + (RootModuleFileValue) env.getValue(ModuleFileValue.KEY_FOR_ROOT_MODULE); + if (rootModuleFileValue == null) { + return null; + } BazelDepGraphValue depGraphValue = (BazelDepGraphValue) env.getValue(BazelDepGraphValue.KEY); if (depGraphValue == null) { return null; @@ -112,6 +118,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) return BazelModTidyValue.create( buildozer.asPath(), + rootModuleFileValue.getImportLabelToCompiledModuleFile(), MODULE_OVERRIDES.get(env), IGNORE_DEV_DEPS.get(env), LOCKFILE_MODE.get(env), diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyValue.java index 281d45cc547236..89caa6c7e49c5c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyValue.java @@ -35,6 +35,8 @@ public abstract class BazelModTidyValue implements SkyValue { /** The path of the buildozer binary provided by the "buildozer" module. */ public abstract Path buildozer(); + public abstract ImmutableMap importLabelToCompiledModuleFile(); + /** The value of {@link ModuleFileFunction#MODULE_OVERRIDES}. */ public abstract ImmutableMap moduleOverrides(); @@ -52,12 +54,14 @@ public abstract class BazelModTidyValue implements SkyValue { static BazelModTidyValue create( Path buildozer, + ImmutableMap importLabelToCompiledModuleFile, Map moduleOverrides, boolean ignoreDevDeps, LockfileMode lockfileMode, StarlarkSemantics starlarkSemantics) { return new AutoValue_BazelModTidyValue( buildozer, + importLabelToCompiledModuleFile, ImmutableMap.copyOf(moduleOverrides), ignoreDevDeps, lockfileMode, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/CompiledModuleFile.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/CompiledModuleFile.java new file mode 100644 index 00000000000000..3dd6590563923a --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/CompiledModuleFile.java @@ -0,0 +1,117 @@ +package com.google.devtools.build.lib.bazel.bzlmod; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.events.ExtendedEventHandler; +import com.google.devtools.build.lib.packages.BazelStarlarkEnvironment; +import com.google.devtools.build.lib.packages.DotBazelFileSyntaxChecker; +import com.google.devtools.build.lib.server.FailureDetails.ExternalDeps.Code; +import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.Module; +import net.starlark.java.eval.Starlark; +import net.starlark.java.eval.StarlarkSemantics; +import net.starlark.java.eval.StarlarkThread; +import net.starlark.java.syntax.Argument; +import net.starlark.java.syntax.CallExpression; +import net.starlark.java.syntax.ExpressionStatement; +import net.starlark.java.syntax.Identifier; +import net.starlark.java.syntax.Location; +import net.starlark.java.syntax.ParserInput; +import net.starlark.java.syntax.Program; +import net.starlark.java.syntax.StarlarkFile; +import net.starlark.java.syntax.StringLiteral; +import net.starlark.java.syntax.SyntaxError; + +/** + * Represents a compiled MODULE.bazel file, ready to be executed on a {@link StarlarkThread}. It's + * been successfully checked for syntax errors. + * + *

Use the {@link #parseAndCompile} factory method instead of directly instantiating this record. + */ +public record CompiledModuleFile( + ModuleFile moduleFile, + Program program, + Module predeclaredEnv, + ImmutableList importStatements) { + + record ModuleImportStatement(String importLabel, Location location) {} + + /** Parses and compiles a given module file, checking it for syntax errors. */ + public static CompiledModuleFile parseAndCompile( + ModuleFile moduleFile, + ModuleKey moduleKey, + StarlarkSemantics starlarkSemantics, + BazelStarlarkEnvironment starlarkEnv, + ExtendedEventHandler eventHandler) + throws ExternalDepsException { + StarlarkFile starlarkFile = + StarlarkFile.parse(ParserInput.fromUTF8(moduleFile.getContent(), moduleFile.getLocation())); + if (!starlarkFile.ok()) { + Event.replayEventsOn(eventHandler, starlarkFile.errors()); + throw ExternalDepsException.withMessage( + Code.BAD_MODULE, "error parsing MODULE.bazel file for %s", moduleKey); + } + try { + ImmutableList importStatements = checkModuleFileSyntax(starlarkFile); + Module predeclaredEnv = + Module.withPredeclared( + starlarkSemantics, starlarkEnv.getStarlarkGlobals().getModuleToplevels()); + Program program = Program.compileFile(starlarkFile, predeclaredEnv); + return new CompiledModuleFile(moduleFile, program, predeclaredEnv, importStatements); + } catch (SyntaxError.Exception e) { + Event.replayEventsOn(eventHandler, e.errors()); + throw ExternalDepsException.withMessage( + Code.BAD_MODULE, "syntax error in MODULE.bazel file for %s", moduleKey); + } + } + + @VisibleForTesting + static ImmutableList checkModuleFileSyntax(StarlarkFile starlarkFile) + throws SyntaxError.Exception { + var importStatements = ImmutableList.builder(); + new DotBazelFileSyntaxChecker("MODULE.bazel files", /* canLoadBzl= */ false) { + @Override + public void visit(ExpressionStatement node) { + // We can assume this statement isn't nested in any block, since we don't allow + // `if`/`def`/`for` in MODULE.bazel. + if (node.getExpression() instanceof CallExpression call + && call.getFunction() instanceof Identifier id + && id.getName().equals("module_import")) { + // Found a top-level call to module_import! + if (call.getArguments().size() == 1 + && call.getArguments().getFirst() instanceof Argument.Positional pos + && pos.getValue() instanceof StringLiteral str) { + importStatements.add( + new ModuleImportStatement(str.getValue(), call.getStartLocation())); + // We can stop going down this rabbit hole now. + return; + } + error( + node.getStartLocation(), + "the `module_import` directive MUST be called with exactly one positional " + + "argument that is a string literal"); + return; + } + super.visit(node); + } + + @Override + public void visit(Identifier node) { + if (node.getName().equals("module_import")) { + // If we somehow reach the `module_import` identifier but NOT as part of a top-level call + // expression, cry foul. + error( + node.getStartLocation(), + "the `module_import` directive MUST be called directly at the top-level"); + } + super.visit(node); + } + }.check(starlarkFile); + return importStatements.build(); + } + + public void runOnThread(StarlarkThread thread) throws EvalException, InterruptedException { + Starlark.execFileProgram(program, predeclaredEnv, thread); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java index 9dec704209acd3..c72c839f5c80b9 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java @@ -16,6 +16,7 @@ package com.google.devtools.build.lib.bazel.bzlmod; import static com.google.common.collect.ImmutableMap.toImmutableMap; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.collect.ImmutableList; @@ -27,12 +28,12 @@ import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue.RootModuleFileValue; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelConstants; +import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.packages.BazelStarlarkEnvironment; -import com.google.devtools.build.lib.packages.DotBazelFileSyntaxChecker; import com.google.devtools.build.lib.packages.StarlarkExportable; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.ProfilerTask; @@ -41,6 +42,8 @@ import com.google.devtools.build.lib.server.FailureDetails.ExternalDeps.Code; import com.google.devtools.build.lib.skyframe.ClientEnvironmentFunction; import com.google.devtools.build.lib.skyframe.ClientEnvironmentValue; +import com.google.devtools.build.lib.skyframe.PackageLookupFunction; +import com.google.devtools.build.lib.skyframe.PackageLookupValue; import com.google.devtools.build.lib.skyframe.PrecomputedValue; import com.google.devtools.build.lib.skyframe.PrecomputedValue.Precomputed; import com.google.devtools.build.lib.util.Fingerprint; @@ -50,13 +53,17 @@ import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.skyframe.SkyFunction; +import com.google.devtools.build.skyframe.SkyFunction.Environment.SkyKeyComputeState; import com.google.devtools.build.skyframe.SkyFunctionException; +import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; +import com.google.devtools.build.skyframe.SkyframeLookupResult; import com.google.errorprone.annotations.FormatMethod; import java.io.IOException; import java.net.URISyntaxException; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -64,13 +71,8 @@ import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; import net.starlark.java.eval.Mutability; -import net.starlark.java.eval.Starlark; import net.starlark.java.eval.StarlarkSemantics; import net.starlark.java.eval.StarlarkThread; -import net.starlark.java.syntax.ParserInput; -import net.starlark.java.syntax.Program; -import net.starlark.java.syntax.StarlarkFile; -import net.starlark.java.syntax.SyntaxError; /** * Takes a {@link ModuleKey} and its override (if any), retrieves the module file from a registry or @@ -161,10 +163,28 @@ public SkyValue compute(SkyKey skyKey, Environment env) String moduleFileHash = new Fingerprint().addBytes(getModuleFileResult.moduleFile.getContent()).hexDigestAndReset(); + CompiledModuleFile compiledModuleFile; + try { + compiledModuleFile = + CompiledModuleFile.parseAndCompile( + getModuleFileResult.moduleFile, + moduleKey, + starlarkSemantics, + starlarkEnv, + env.getListener()); + } catch (ExternalDepsException e) { + throw new ModuleFileFunctionException(e, Transience.PERSISTENT); + } + if (!compiledModuleFile.importStatements().isEmpty()) { + throw errorf( + Code.BAD_MODULE, + "module_import() directives found at %s, but they can only be used in the root module", + compiledModuleFile.importStatements().getFirst().location()); + } ModuleThreadContext moduleThreadContext = execModuleFile( - getModuleFileResult.moduleFile, - getModuleFileResult.registry, + compiledModuleFile, + /* importLabelToParsedModuleFile= */ null, moduleKey, // Dev dependencies should always be ignored if the current module isn't the root module /* ignoreDevDeps= */ true, @@ -172,13 +192,12 @@ public SkyValue compute(SkyKey skyKey, Environment env) // We try to prevent most side effects of yanked modules, in particular print(). /* printIsNoop= */ getModuleFileResult.yankedInfo != null, starlarkSemantics, - starlarkEnv, env.getListener()); // Perform some sanity checks. InterimModule module; try { - module = moduleThreadContext.buildModule(); + module = moduleThreadContext.buildModule(getModuleFileResult.registry); } catch (EvalException e) { env.getListener().handle(Event.error(e.getMessageWithStack())); throw errorf(Code.BAD_MODULE, "error executing MODULE.bazel file for %s", moduleKey); @@ -218,69 +237,217 @@ public SkyValue compute(SkyKey skyKey, Environment env) return NonRootModuleFileValue.create(module, moduleFileHash); } + /** + * State object used during root module file evaluation. We try to compile the root module file + * itself first, and then read, parse, and compile any imported module files layer by layer, in a + * BFS fashion (hence the {@code horizon} field). Finally, everything is collected into the {@code + * importLabelToCompiledModuleFile} map for use during actual Starlark execution. + */ + static class RootState implements SkyKeyComputeState { + CompiledModuleFile compiledRootModuleFile; + ImmutableList horizon; + HashMap importLabelToCompiledModuleFile = new HashMap<>(); + } + @Nullable private SkyValue computeForRootModule(StarlarkSemantics starlarkSemantics, Environment env) throws ModuleFileFunctionException, InterruptedException { - RootedPath moduleFilePath = getModuleFilePath(workspaceRoot); - if (env.getValue(FileValue.key(moduleFilePath)) == null) { - return null; + RootState state = env.getState(RootState::new); + if (state.compiledRootModuleFile == null) { + RootedPath moduleFilePath = getModuleFilePath(workspaceRoot); + if (env.getValue(FileValue.key(moduleFilePath)) == null) { + return null; + } + byte[] moduleFileContents; + if (moduleFilePath.asPath().exists()) { + moduleFileContents = readModuleFile(moduleFilePath.asPath()); + } else { + moduleFileContents = BZLMOD_REMINDER.getBytes(UTF_8); + createModuleFile(moduleFilePath.asPath(), moduleFileContents); + env.getListener() + .handle( + Event.warn( + "--enable_bzlmod is set, but no MODULE.bazel file was found at the workspace" + + " root. Bazel will create an empty MODULE.bazel file. Please consider" + + " migrating your external dependencies from WORKSPACE to MODULE.bazel. For" + + " more details, please refer to" + + " https://github.com/bazelbuild/bazel/issues/18958.")); + } + try { + state.compiledRootModuleFile = + CompiledModuleFile.parseAndCompile( + ModuleFile.create(moduleFileContents, moduleFilePath.asPath().toString()), + ModuleKey.ROOT, + starlarkSemantics, + starlarkEnv, + env.getListener()); + } catch (ExternalDepsException e) { + throw new ModuleFileFunctionException(e, Transience.PERSISTENT); + } + state.horizon = state.compiledRootModuleFile.importStatements(); } - byte[] moduleFileContents; - if (moduleFilePath.asPath().exists()) { - moduleFileContents = readModuleFile(moduleFilePath.asPath()); - } else { - moduleFileContents = BZLMOD_REMINDER.getBytes(UTF_8); - createModuleFile(moduleFilePath.asPath(), moduleFileContents); - env.getListener() - .handle( - Event.warn( - "--enable_bzlmod is set, but no MODULE.bazel file was found at the workspace" - + " root. Bazel will create an empty MODULE.bazel file. Please consider" - + " migrating your external dependencies from WORKSPACE to MODULE.bazel. For" - + " more details, please refer to" - + " https://github.com/bazelbuild/bazel/issues/18958.")); + while (!state.horizon.isEmpty()) { + var newHorizon = + advanceHorizon( + state.importLabelToCompiledModuleFile, + state.horizon, + env, + starlarkSemantics, + starlarkEnv); + if (newHorizon == null) { + return null; + } + state.horizon = newHorizon; } return evaluateRootModuleFile( - moduleFileContents, - moduleFilePath, + state.compiledRootModuleFile, + ImmutableMap.copyOf(state.importLabelToCompiledModuleFile), builtinModules, MODULE_OVERRIDES.get(env), IGNORE_DEV_DEPS.get(env), starlarkSemantics, - starlarkEnv, env.getListener()); } + /** + * Reads, parses, and compiles all imported module files named by {@code horizon}, stores the + * result in {@code importLabelToCompiledModuleFile}, and finally returns the import statements of + * these newly compiled module files as a new "horizon". + */ + private static ImmutableList advanceHorizon( + HashMap importLabelToCompiledModuleFile, + ImmutableList horizon, + Environment env, + StarlarkSemantics starlarkSemantics, + BazelStarlarkEnvironment starlarkEnv) + throws ModuleFileFunctionException, InterruptedException { + var importLabels = new ArrayList