From 1766473944f58921b61e4e982d96099d52c5a3c4 Mon Sep 17 00:00:00 2001 From: Xdng Yng Date: Mon, 22 Apr 2024 13:22:11 -0700 Subject: [PATCH] Enforce file name format for MODULE.bazel includes They must end in `.MODULE.bazel`. Follow-up for https://github.com/bazelbuild/bazel/issues/17880 Closes #22075. PiperOrigin-RevId: 627136756 Change-Id: If9b1797f2e7ddc1aebd929646329e832288bfd8a --- .../lib/bazel/bzlmod/CompiledModuleFile.java | 2 +- .../lib/bazel/bzlmod/ModuleFileFunction.java | 8 +++ .../bazel/bzlmod/CompiledModuleFileTest.java | 5 +- .../bazel/bzlmod/ModuleFileFunctionTest.java | 50 ++++++++++++------- src/test/py/bazel/bzlmod/bazel_module_test.py | 4 +- 5 files changed, 48 insertions(+), 21 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 8d5b57e383b5e1..2cdcacf79bd31d 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 @@ -142,7 +142,7 @@ public void visit(AssignmentStatement node) { @Override public void visit(DotExpression node) { visit(node.getObject()); - if (includeWasAssigned || !node.getField().getName().equals(INCLUDE_IDENTIFIER)) { + if (!node.getField().getName().equals(INCLUDE_IDENTIFIER)) { // This is fine: `whatever.include` // (so `include` can be used as a tag class name) visit(node.getField()); 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 2eb45924d0fa4d..e6ac3416528a68 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 @@ -351,6 +351,14 @@ private static ImmutableList advanceHorizon( includeStatement.includeLabel(), includeStatement.location()); } + if (!includeStatement.includeLabel().endsWith(".MODULE.bazel")) { + throw errorf( + Code.BAD_MODULE, + "bad include label '%s' at %s: the file to be included must have a name ending in" + + " '.MODULE.bazel'", + includeStatement.includeLabel(), + includeStatement.location()); + } try { includeLabels.add(Label.parseCanonical(includeStatement.includeLabel())); } catch (LabelSyntaxException e) { 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 6a7125b6c3a6a1..34bfdd6f935d1a 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 @@ -91,13 +91,16 @@ public void checkSyntax_good_benignUsageOfInclude() throws Exception { public void checkSyntax_good_includeIdentifierReassigned() throws Exception { String program = """ + include('world') include = print # from this point on, we no longer check anything about `include` usage. include('hello') str(include) exclude = include """; - assertThat(checkSyntax(program)).isEmpty(); + assertThat(checkSyntax(program)) + .containsExactly( + new IncludeStatement("world", Location.fromFileLineColumn("test file", 1, 1))); } @Test 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 76d6f4ebd9bb65..cea99f30491dea 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 @@ -334,22 +334,22 @@ public void testRootModule_include_good() throws Exception { scratch.overwriteFile( rootDirectory.getRelative("MODULE.bazel").getPathString(), "module(name='aaa')", - "include('//java:MODULE.bazel.segment')", + "include('//java:java.MODULE.bazel')", "bazel_dep(name='foo', version='1.0')", "register_toolchains('//:whatever')", - "include('//python:MODULE.bazel.segment')"); + "include('//python:python.MODULE.bazel')"); scratch.overwriteFile(rootDirectory.getRelative("java/BUILD").getPathString()); scratch.overwriteFile( - rootDirectory.getRelative("java/MODULE.bazel.segment").getPathString(), + rootDirectory.getRelative("java/java.MODULE.bazel").getPathString(), "bazel_dep(name='java-foo', version='1.0')"); scratch.overwriteFile(rootDirectory.getRelative("python/BUILD").getPathString()); scratch.overwriteFile( - rootDirectory.getRelative("python/MODULE.bazel.segment").getPathString(), + rootDirectory.getRelative("python/python.MODULE.bazel").getPathString(), "bazel_dep(name='py-foo', version='1.0', repo_name='python-foo')", "single_version_override(module_name='java-foo', version='2.0')", - "include('//python:toolchains/MODULE.bazel.segment')"); + "include('//python:toolchains/toolchains.MODULE.bazel')"); scratch.overwriteFile( - rootDirectory.getRelative("python/toolchains/MODULE.bazel.segment").getPathString(), + rootDirectory.getRelative("python/toolchains/toolchains.MODULE.bazel").getPathString(), "register_toolchains('//:python-whatever')"); FakeRegistry registry = registryFactory.newFakeRegistry("/foo"); ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl())); @@ -387,7 +387,7 @@ public void testRootModule_include_bad_otherRepoLabel() throws Exception { scratch.overwriteFile( rootDirectory.getRelative("MODULE.bazel").getPathString(), "module(name='aaa')", - "include('@haha//java:MODULE.bazel.segment')"); + "include('@haha//java:java.MODULE.bazel')"); FakeRegistry registry = registryFactory.newFakeRegistry("/foo"); ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl())); @@ -403,7 +403,7 @@ public void testRootModule_include_bad_relativeLabel() throws Exception { scratch.overwriteFile( rootDirectory.getRelative("MODULE.bazel").getPathString(), "module(name='aaa')", - "include(':MODULE.bazel.segment')"); + "include(':relative.MODULE.bazel')"); FakeRegistry registry = registryFactory.newFakeRegistry("/foo"); ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl())); @@ -414,12 +414,28 @@ public void testRootModule_include_bad_relativeLabel() throws Exception { assertThat(result.getError().toString()).contains("starting with double slashes"); } + @Test + public void testRootModule_include_bad_notEndingInModuleBazel() throws Exception { + scratch.overwriteFile( + rootDirectory.getRelative("MODULE.bazel").getPathString(), + "module(name='aaa')", + "include('//:MODULE.bazel.segment')"); + FakeRegistry registry = registryFactory.newFakeRegistry("/foo"); + ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl())); + + EvaluationResult result = + evaluator.evaluate( + ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext); + assertThat(result.hasError()).isTrue(); + assertThat(result.getError().toString()).contains("have a name ending in '.MODULE.bazel'"); + } + @Test public void testRootModule_include_bad_badLabelSyntax() throws Exception { scratch.overwriteFile( rootDirectory.getRelative("MODULE.bazel").getPathString(), "module(name='aaa')", - "include('//haha/:::')"); + "include('//haha/:::.MODULE.bazel')"); FakeRegistry registry = registryFactory.newFakeRegistry("/foo"); ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl())); @@ -435,10 +451,10 @@ public void testRootModule_include_bad_badLabelSyntax() throws Exception { public void testRootModule_include_bad_moduleAfterInclude() throws Exception { scratch.overwriteFile( rootDirectory.getRelative("MODULE.bazel").getPathString(), - "include('//java:MODULE.bazel.segment')"); + "include('//java:java.MODULE.bazel')"); scratch.overwriteFile(rootDirectory.getRelative("java/BUILD").getPathString()); scratch.overwriteFile( - rootDirectory.getRelative("java/MODULE.bazel.segment").getPathString(), + rootDirectory.getRelative("java/java.MODULE.bazel").getPathString(), "module(name='bet-you-didnt-expect-this-didya')", "bazel_dep(name='java-foo', version='1.0', repo_name='foo')"); FakeRegistry registry = registryFactory.newFakeRegistry("/foo"); @@ -457,15 +473,15 @@ public void testRootModule_include_bad_repoNameCollision() throws Exception { scratch.overwriteFile( rootDirectory.getRelative("MODULE.bazel").getPathString(), "module(name='aaa')", - "include('//java:MODULE.bazel.segment')", - "include('//python:MODULE.bazel.segment')"); + "include('//java:java.MODULE.bazel')", + "include('//python:python.MODULE.bazel')"); scratch.overwriteFile(rootDirectory.getRelative("java/BUILD").getPathString()); scratch.overwriteFile( - rootDirectory.getRelative("java/MODULE.bazel.segment").getPathString(), + rootDirectory.getRelative("java/java.MODULE.bazel").getPathString(), "bazel_dep(name='java-foo', version='1.0', repo_name='foo')"); scratch.overwriteFile(rootDirectory.getRelative("python/BUILD").getPathString()); scratch.overwriteFile( - rootDirectory.getRelative("python/MODULE.bazel.segment").getPathString(), + rootDirectory.getRelative("python/python.MODULE.bazel").getPathString(), "bazel_dep(name='python-foo', version='1.0', repo_name='foo')"); FakeRegistry registry = registryFactory.newFakeRegistry("/foo"); ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl())); @@ -484,10 +500,10 @@ public void testRootModule_include_bad_tryingToLeakBindings() throws Exception { rootDirectory.getRelative("MODULE.bazel").getPathString(), "module(name='aaa')", "FOO_NAME = 'foo'", - "include('//java:MODULE.bazel.segment')"); + "include('//java:java.MODULE.bazel')"); scratch.overwriteFile(rootDirectory.getRelative("java/BUILD").getPathString()); scratch.overwriteFile( - rootDirectory.getRelative("java/MODULE.bazel.segment").getPathString(), + rootDirectory.getRelative("java/java.MODULE.bazel").getPathString(), "bazel_dep(name=FOO_NAME, version='1.0')"); FakeRegistry registry = registryFactory.newFakeRegistry("/foo"); ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl())); diff --git a/src/test/py/bazel/bzlmod/bazel_module_test.py b/src/test/py/bazel/bzlmod/bazel_module_test.py index ef71d338c9709d..41915a6bce390d 100644 --- a/src/test/py/bazel/bzlmod/bazel_module_test.py +++ b/src/test/py/bazel/bzlmod/bazel_module_test.py @@ -906,12 +906,12 @@ def testInclude(self): [ 'module(name="foo")', 'bazel_dep(name="bbb", version="1.0")', - 'include("//java:MODULE.bazel.segment")', + 'include("//java:java.MODULE.bazel")', ], ) self.ScratchFile('java/BUILD') self.ScratchFile( - 'java/MODULE.bazel.segment', + 'java/java.MODULE.bazel', [ 'bazel_dep(name="aaa", version="1.0", repo_name="lol")', ],