From 372a0ac1818ac5631e36e741cc6f2ca8166a1fc5 Mon Sep 17 00:00:00 2001 From: Keith Smiley Date: Thu, 27 Aug 2026 04:48:16 -0700 Subject: [PATCH] Add dev_dependency to include() statements (https://github.com/bazelbuild/bazel/pull/30782) This allows repos to use separate MODULE.bazel files only for testing, and not fail when they are used in the BCR by other repos. Fixes https://github.com/bazelbuild/bazel/issues/25362 Fixes https://github.com/bazelbuild/bazel/issues/25189 Closes #30782. PiperOrigin-RevId: 971881975 Change-Id: Ia7dc48ebe440dd2cfe2d585a6f803ea83fdcaca5 --- .../lib/bazel/bzlmod/CompiledModuleFile.java | 28 +++++++---- .../lib/bazel/bzlmod/ModuleFileFunction.java | 35 ++++++++++---- .../lib/bazel/bzlmod/ModuleFileGlobals.java | 13 ++++- .../lib/bazel/bzlmod/ModuleThreadContext.java | 5 +- .../bazel/repository/RepositoryOptions.java | 8 ++-- .../bazel/bzlmod/CompiledModuleFileTest.java | 44 +++++++++++++---- .../bazel/bzlmod/ModuleFileFunctionTest.java | 48 ++++++++++++++++++- 7 files changed, 149 insertions(+), 32 deletions(-) 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 index 87d23a07bda995..b25ad866bc3ad5 100644 --- 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 @@ -53,7 +53,7 @@ public record CompiledModuleFile( ImmutableList includeStatements) { public static final String INCLUDE_IDENTIFIER = "include"; - record IncludeStatement(String includeLabel, Location location) {} + record IncludeStatement(String includeLabel, boolean devDependency, Location location) {} /** Parses and compiles a given module file, checking it for syntax errors. */ public static CompiledModuleFile parseAndCompile( @@ -120,17 +120,28 @@ public void visit(ExpressionStatement node) { && call.getFunction() instanceof Identifier id && id.getName().equals(INCLUDE_IDENTIFIER)) { // Found a top-level call to `include`! - if (call.getArguments().size() == 1 + if (!call.getArguments().isEmpty() && call.getArguments().getFirst() instanceof Argument.Positional pos - && pos.getValue() instanceof StringLiteral str) { - includeStatements.add(new IncludeStatement(str.getValue(), call.getStartLocation())); + && pos.getValue() instanceof StringLiteral str + && (call.getArguments().size() == 1 + || (call.getArguments().size() == 2 + && call.getArguments().get(1) instanceof Argument.Keyword keyword + && keyword.getName().equals("dev_dependency") + && keyword.getValue() instanceof Identifier devDependency + && (devDependency.getName().equals("True") + || devDependency.getName().equals("False"))))) { + boolean devDependency = + call.getArguments().size() == 2 + && ((Identifier) call.getArguments().get(1).getValue()).getName().equals("True"); + includeStatements.add( + new IncludeStatement(str.getValue(), devDependency, call.getStartLocation())); // Nothing else to check, we can stop visiting sub-nodes now. return; } error( node.getStartLocation(), - "the `include` directive MUST be called with exactly one positional argument that " - + "is a string literal"); + "the `include` directive MUST be called with one positional string literal argument " + + "and, optionally, `dev_dependency` as a boolean literal"); return; } super.visit(node); @@ -168,8 +179,9 @@ public void visit(Identifier node) { * Checks the given `starlarkFile` for module file syntax, and returns the list of `include` * statements it contains. This is a somewhat crude sweep over the AST; we loudly complain about * any usage of `include` that is not in a top-level function call statement with one single - * string literal positional argument, *except* that we don't do this check once `include` is - * assigned to, due to backwards compatibility concerns. + * string literal positional argument and an optional boolean literal {@code dev_dependency} + * keyword argument, *except* that we don't do this check once `include` is assigned to, due to + * backwards compatibility concerns. */ @VisibleForTesting static ImmutableList checkModuleFileSyntax(StarlarkFile starlarkFile) 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 1e1132ccbb0616..a3ee061cd7dc04 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 @@ -220,12 +220,16 @@ public SkyValue compute(SkyKey skyKey, Environment env) } ModuleThreadContext moduleThreadContext; if (state.moduleFileMetadata.registry != null) { - if (!state.compiledModuleFile.includeStatements().isEmpty()) { + Optional nonDevInclude = + state.compiledModuleFile.includeStatements().stream() + .filter(include -> !include.devDependency()) + .findFirst(); + if (nonDevInclude.isPresent()) { throw errorf( Code.BAD_MODULE, "include() directive found at %s, but it can only be used in the root module or in " + "modules with non-registry overrides", - state.compiledModuleFile.includeStatements().getFirst().location()); + nonDevInclude.get().location()); } moduleThreadContext = execModuleFile( @@ -341,8 +345,11 @@ private ModuleThreadContext execNonRegistryModuleFile( SymbolGenerator symbolGenerator) throws ModuleFileFunctionException, InterruptedException { Preconditions.checkNotNull(state.compiledModuleFile); + boolean isRoot = moduleKey.equals(ModuleKey.ROOT); + boolean ignoreDevDeps = isRoot ? IGNORE_DEV_DEPS.get(env) : true; if (state.horizon == null) { - state.horizon = state.compiledModuleFile.includeStatements(); + state.horizon = + activeIncludeStatements(state.compiledModuleFile.includeStatements(), ignoreDevDeps); } while (!state.horizon.isEmpty()) { var newHorizon = @@ -352,18 +359,18 @@ private ModuleThreadContext execNonRegistryModuleFile( state.horizon, env, starlarkSemantics, - starlarkEnv); + starlarkEnv, + ignoreDevDeps); if (newHorizon == null) { return null; } state.horizon = newHorizon; } - boolean isRoot = moduleKey.equals(ModuleKey.ROOT); return execModuleFile( state.compiledModuleFile, ImmutableMap.copyOf(state.includeLabelToCompiledModuleFile), moduleKey, - isRoot ? IGNORE_DEV_DEPS.get(env) : true, + ignoreDevDeps, builtinModules, isRoot ? INJECTED_REPOSITORIES.get(env) : ImmutableMap.of(), // Allow printing to aid in debugging non-registry overrides, which are often edited by the @@ -391,7 +398,8 @@ private static ImmutableList advanceHorizon( ImmutableList horizon, Environment env, StarlarkSemantics starlarkSemantics, - BazelStarlarkEnvironment starlarkEnv) + BazelStarlarkEnvironment starlarkEnv, + boolean ignoreDevDeps) throws ModuleFileFunctionException, InterruptedException { var seenIncludeLabels = new HashSet<>(includeLabelToCompiledModuleFile.keySet()); var pendingBuilder = ImmutableList.builder(); @@ -518,7 +526,8 @@ private static ImmutableList advanceHorizon( starlarkEnv, env.getListener()); includeLabelToCompiledModuleFile.put(pending.get(i).includeLabel(), compiledModuleFile); - newHorizon.addAll(compiledModuleFile.includeStatements()); + newHorizon.addAll( + activeIncludeStatements(compiledModuleFile.includeStatements(), ignoreDevDeps)); } catch (ExternalDepsException e) { throw new ModuleFileFunctionException(e, Transience.PERSISTENT); } @@ -526,6 +535,16 @@ private static ImmutableList advanceHorizon( return newHorizon.build(); } + private static ImmutableList activeIncludeStatements( + ImmutableList includeStatements, boolean ignoreDevDeps) { + if (!ignoreDevDeps) { + return includeStatements; + } + return includeStatements.stream() + .filter(include -> !include.devDependency()) + .collect(toImmutableList()); + } + public static RootedPath getModuleFilePath(Path workspaceRoot) { return RootedPath.toRootedPath( Root.fromPath(workspaceRoot), LabelConstants.MODULE_DOT_BAZEL_FILE_NAME); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java index 4d3ee18c402f7e..219517264f1b34 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java @@ -889,14 +889,23 @@ public void call( + " main repo; in other words, it must start with double" + " slashes (//). The name of the file must end with" + " .MODULE.bazel and must not start with .."), + @Param( + name = "dev_dependency", + doc = + "If true, this include will be ignored if the current module is not the root" + + " module or --ignore_dev_dependency is enabled. The value must" + + " be a literal True or False.", + named = true, + positional = false, + defaultValue = "False"), }, useStarlarkThread = true) - public void include(String label, StarlarkThread thread) + public void include(String label, boolean devDependency, StarlarkThread thread) throws InterruptedException, EvalException { ModuleThreadContext context = ModuleThreadContext.fromOrFail(thread, CompiledModuleFile.INCLUDE_IDENTIFIER + "()"); context.setNonModuleCalled(); - context.include(label, thread); + context.include(label, devDependency, thread); } @StarlarkMethod( diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java index 38077f459d9f30..a1a9d642e78976 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java @@ -330,8 +330,11 @@ ModuleExtensionUsage buildUsage() throws EvalException { } } - public void include(String includeLabel, StarlarkThread thread) + public void include(String includeLabel, boolean devDependency, StarlarkThread thread) throws InterruptedException, EvalException { + if (shouldIgnoreDevDeps() && devDependency) { + return; + } if (includeLabelToCompiledModuleFile == null) { // This should never happen because compiling the non-root module file should have failed, way // before evaluation started. diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java index a27e72ffe8213f..f30a110c6e96ad 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java @@ -298,10 +298,10 @@ verified by hashes stored in the registry (and thus pinned by the lockfile). effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, help = """ - If true, Bazel ignores `bazel_dep` and `use_extension` declared as `dev_dependency` in - the `MODULE.bazel` of the root module. Note that, those dev dependencies are always - ignored in the `MODULE.bazel` if it's not the root module regardless of the value - of this flag. + If true, Bazel ignores `bazel_dep`, `use_extension`, and `include` declared as + `dev_dependency` in the `MODULE.bazel` of the root module. Note that these dev + dependencies are always ignored in the `MODULE.bazel` if it is not the root module, + regardless of the value of this flag. """) public boolean ignoreDevDependency; diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/CompiledModuleFileTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/CompiledModuleFileTest.java index 34bfdd6f935d1a..60427168438e74 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/CompiledModuleFileTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/CompiledModuleFileTest.java @@ -45,7 +45,7 @@ public void checkSyntax_good() throws Exception { """; assertThat(checkSyntax(program)) .containsExactly( - new IncludeStatement("hullo", Location.fromFileLineColumn("test file", 2, 1))); + new IncludeStatement("hullo", false, Location.fromFileLineColumn("test file", 2, 1))); } @Test @@ -59,8 +59,8 @@ public void checkSyntax_good_multiple() throws Exception { """; assertThat(checkSyntax(program)) .containsExactly( - new IncludeStatement("hullo", Location.fromFileLineColumn("test file", 2, 1)), - new IncludeStatement("world", Location.fromFileLineColumn("test file", 4, 1))); + new IncludeStatement("hullo", false, Location.fromFileLineColumn("test file", 2, 1)), + new IncludeStatement("world", false, Location.fromFileLineColumn("test file", 4, 1))); } @Test @@ -74,7 +74,24 @@ public void checkSyntax_good_multilineLiteral() throws Exception { """; assertThat(checkSyntax(program)) .containsExactly( - new IncludeStatement("hullo\nworld", Location.fromFileLineColumn("test file", 3, 1))); + new IncludeStatement( + "hullo\nworld", false, Location.fromFileLineColumn("test file", 3, 1))); + } + + @Test + public void checkSyntax_good_devDependency() throws Exception { + String program = + """ + include("dev.MODULE.bazel", dev_dependency = True) + include("prod.MODULE.bazel", dev_dependency = False) + """; + assertThat(checkSyntax(program)) + .containsExactly( + new IncludeStatement( + "dev.MODULE.bazel", true, Location.fromFileLineColumn("test file", 1, 1)), + new IncludeStatement( + "prod.MODULE.bazel", false, Location.fromFileLineColumn("test file", 2, 1))) + .inOrder(); } @Test @@ -100,7 +117,7 @@ public void checkSyntax_good_includeIdentifierReassigned() throws Exception { """; assertThat(checkSyntax(program)) .containsExactly( - new IncludeStatement("world", Location.fromFileLineColumn("test file", 1, 1))); + new IncludeStatement("world", false, Location.fromFileLineColumn("test file", 1, 1))); } @Test @@ -151,7 +168,7 @@ public void checkSyntax_bad_multipleArgumentsToInclude() throws Exception { var ex = assertThrows(SyntaxError.Exception.class, () -> checkSyntax(program)); assertThat(ex) .hasMessageThat() - .contains("the `include` directive MUST be called with exactly one positional"); + .contains("the `include` directive MUST be called with one positional string literal"); } @Test @@ -163,7 +180,7 @@ public void checkSyntax_bad_keywordArgumentToInclude() throws Exception { var ex = assertThrows(SyntaxError.Exception.class, () -> checkSyntax(program)); assertThat(ex) .hasMessageThat() - .contains("the `include` directive MUST be called with exactly one positional"); + .contains("the `include` directive MUST be called with one positional string literal"); } @Test @@ -176,6 +193,17 @@ public void checkSyntax_bad_nonLiteralArgumentToInclude() throws Exception { var ex = assertThrows(SyntaxError.Exception.class, () -> checkSyntax(program)); assertThat(ex) .hasMessageThat() - .contains("the `include` directive MUST be called with exactly one positional"); + .contains("the `include` directive MUST be called with one positional string literal"); + } + + @Test + public void checkSyntax_bad_nonLiteralDevDependency() throws Exception { + String program = + """ + dev = True + include('hello', dev_dependency = dev) + """; + var ex = assertThrows(SyntaxError.Exception.class, () -> checkSyntax(program)); + assertThat(ex).hasMessageThat().contains("optionally, `dev_dependency` as a boolean literal"); } } diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java index 71d2c95799856d..47090bf4a8956a 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java @@ -358,7 +358,7 @@ public void testRootModule_include_good() throws Exception { "include('//java:java.MODULE.bazel')", "bazel_dep(name='foo', version='1.0')", "register_toolchains('//:whatever')", - "include('//python:python.MODULE.bazel')"); + "include('//python:python.MODULE.bazel', dev_dependency=True)"); scratch.overwriteFile(rootDirectory.getRelative("java/BUILD").getPathString()); scratch.overwriteFile( rootDirectory.getRelative("java/java.MODULE.bazel").getPathString(), @@ -403,6 +403,33 @@ public void testRootModule_include_good() throws Exception { Version.parse("2.0"), "", ImmutableList.of(), ImmutableList.of(), 0)); } + @Test + public void testRootModule_devIncludeIgnoredWithIgnoreDevDependency() throws Exception { + scratch.overwriteFile( + rootDirectory.getRelative("MODULE.bazel").getPathString(), + "module(name='aaa')", + "bazel_dep(name='foo', version='1.0')", + "include('//missing:dev.MODULE.bazel', dev_dependency=True)", + "include('//prod:prod.MODULE.bazel')"); + scratch.overwriteFile(rootDirectory.getRelative("prod/BUILD").getPathString()); + scratch.overwriteFile( + rootDirectory.getRelative("prod/prod.MODULE.bazel").getPathString(), + "bazel_dep(name='bar', version='2.0')", + "include('//missing:nested-dev.MODULE.bazel', dev_dependency=True)"); + FakeRegistry registry = registryFactory.newFakeRegistry("/foo"); + ModuleFileFunction.REGISTRIES.set(differencer, ImmutableSet.of(registry.getUrl())); + ModuleFileFunction.IGNORE_DEV_DEPS.set(differencer, true); + + EvaluationResult result = + evaluator.evaluate( + ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext); + + assertThat(result.hasError()).isFalse(); + assertThat(result.get(ModuleFileValue.KEY_FOR_ROOT_MODULE).module().getDeps()) + .containsExactly("foo", createModuleKey("foo", "1.0"), "bar", createModuleKey("bar", "2.0")) + .inOrder(); + } + @Test public void testRootModule_include_bad_otherRepoLabel() throws Exception { scratch.overwriteFile( @@ -705,6 +732,25 @@ public void testNonRootModuleCannotUseInclude() throws Exception { assertThat(result.getError().toString()).contains("but it can only be used in the root module"); } + @Test + public void testRegistryModuleDevIncludeIsIgnored() throws Exception { + FakeRegistry registry = + registryFactory + .newFakeRegistry("/foo") + .addModule( + createModuleKey("foo", "1.0"), + "module(name='foo',version='1.0')", + "include('//missing:dev.MODULE.bazel', dev_dependency=True)"); + ModuleFileFunction.REGISTRIES.set(differencer, ImmutableSet.of(registry.getUrl())); + + SkyKey skyKey = ModuleFileValue.key(createModuleKey("foo", "1.0")); + EvaluationResult result = + evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext); + + assertThat(result.hasError()).isFalse(); + assertThat(result.get(skyKey).module().getName()).isEqualTo("foo"); + } + @Ignore( "b/389163906 - figure out how to convert this class to BuildViewTestCase; the presence of the" + " many builtin modules in the default AnalysisMock makes it very hard")