From 7cf935d2916b80ea9ed9ca5dfe6a24932e5e9daa Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 20 Jul 2023 08:02:23 -0700 Subject: [PATCH 1/2] Add support for isolated extension usages to the lockfile Previously, Bazel would crash if a module declares an isolated extension usage and the lockfile is used. Closes #18991. PiperOrigin-RevId: 549631385 Change-Id: Id8e706991dc5053b2873847a62f5e0b777347c69 --- .../lib/bazel/bzlmod/GsonTypeAdapterUtil.java | 65 ++++++++++++++----- .../lib/bazel/bzlmod/ModuleExtensionId.java | 13 ++++ .../build/lib/bazel/bzlmod/ModuleKey.java | 15 +++++ .../py/bazel/bzlmod/bazel_lockfile_test.py | 52 +++++++++++++++ 4 files changed, 127 insertions(+), 18 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java index 313471c8468b85..f0190f4c9fb66f 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java @@ -41,7 +41,6 @@ import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; import java.util.Base64; -import java.util.List; import java.util.Optional; import javax.annotation.Nullable; import net.starlark.java.syntax.Location; @@ -82,52 +81,81 @@ public void write(JsonWriter jsonWriter, ModuleKey moduleKey) throws IOException @Override public ModuleKey read(JsonReader jsonReader) throws IOException { String jsonString = jsonReader.nextString(); - if (jsonString.equals("")) { - return ModuleKey.ROOT; - } - List parts = Splitter.on('@').splitToList(jsonString); - if (parts.get(1).equals("_")) { - return ModuleKey.create(parts.get(0), Version.EMPTY); - } - - Version version; try { - version = Version.parse(parts.get(1)); + return ModuleKey.fromString(jsonString); } catch (ParseException e) { throw new JsonParseException( String.format("Unable to parse ModuleKey %s version from the lockfile", jsonString), e); } - return ModuleKey.create(parts.get(0), version); } }; - // TODO(salmasamy) need to handle "isolated" in module extensions when it is stable public static final TypeAdapter MODULE_EXTENSION_ID_TYPE_ADAPTER = new TypeAdapter<>() { @Override public void write(JsonWriter jsonWriter, ModuleExtensionId moduleExtId) throws IOException { - jsonWriter.value(moduleExtId.getBzlFileLabel() + "%" + moduleExtId.getExtensionName()); + String isolationKeyPart = moduleExtId.getIsolationKey().map(key -> "%" + key).orElse(""); + jsonWriter.value( + moduleExtId.getBzlFileLabel() + + "%" + + moduleExtId.getExtensionName() + + isolationKeyPart); } @Override public ModuleExtensionId read(JsonReader jsonReader) throws IOException { String jsonString = jsonReader.nextString(); - // [0] is labelString, [1] is extensionName - List extIdParts = Splitter.on("%").splitToList(jsonString); + var extIdParts = Splitter.on('%').splitToList(jsonString); + Optional isolationKey; + if (extIdParts.size() > 2) { + try { + isolationKey = + Optional.of(ModuleExtensionId.IsolationKey.fromString(extIdParts.get(2))); + } catch (ParseException e) { + throw new JsonParseException( + String.format( + "Unable to parse ModuleExtensionID isolation key: '%s' from the lockfile", + extIdParts.get(2)), + e); + } + } else { + isolationKey = Optional.empty(); + } try { return ModuleExtensionId.create( - Label.parseCanonical(extIdParts.get(0)), extIdParts.get(1), Optional.empty()); + Label.parseCanonical(extIdParts.get(0)), extIdParts.get(1), isolationKey); } catch (LabelSyntaxException e) { throw new JsonParseException( String.format( - "Unable to parse ModuleExtensionID bzl file label: '%s' from the lockfile", + "Unable to parse ModuleExtensionID bzl file label: '%s' from the lockfile", extIdParts.get(0)), e); } } }; + public static final TypeAdapter ISOLATION_KEY_TYPE_ADAPTER = + new TypeAdapter<>() { + @Override + public void write(JsonWriter jsonWriter, ModuleExtensionId.IsolationKey isolationKey) + throws IOException { + jsonWriter.value(isolationKey.toString()); + } + + @Override + public ModuleExtensionId.IsolationKey read(JsonReader jsonReader) throws IOException { + String jsonString = jsonReader.nextString(); + try { + return ModuleExtensionId.IsolationKey.fromString(jsonString); + } catch (ParseException e) { + throw new JsonParseException( + String.format("Unable to parse isolation key: '%s' from the lockfile", jsonString), + e); + } + } + }; + public static final TypeAdapter BYTE_ARRAY_TYPE_ADAPTER = new TypeAdapter<>() { @Override @@ -283,6 +311,7 @@ public static Gson createLockFileGson(Path moduleFilePath) { .registerTypeAdapter(Version.class, VERSION_TYPE_ADAPTER) .registerTypeAdapter(ModuleKey.class, MODULE_KEY_TYPE_ADAPTER) .registerTypeAdapter(ModuleExtensionId.class, MODULE_EXTENSION_ID_TYPE_ADAPTER) + .registerTypeAdapter(ModuleExtensionId.IsolationKey.class, ISOLATION_KEY_TYPE_ADAPTER) .registerTypeAdapter(AttributeValues.class, new AttributeValuesAdapter()) .registerTypeAdapter(byte[].class, BYTE_ARRAY_TYPE_ADAPTER) .create(); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionId.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionId.java index 7690cf0ef884f5..250f83a27bcc65 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionId.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionId.java @@ -19,8 +19,10 @@ import static java.util.Comparator.comparing; import com.google.auto.value.AutoValue; +import com.google.common.base.Splitter; import com.google.devtools.build.lib.cmdline.Label; import java.util.Comparator; +import java.util.List; import java.util.Optional; /** A unique identifier for a {@link ModuleExtension}. */ @@ -49,6 +51,17 @@ abstract static class IsolationKey { public static IsolationKey create(ModuleKey module, String usageExportedName) { return new AutoValue_ModuleExtensionId_IsolationKey(module, usageExportedName); } + + @Override + public final String toString() { + return getModule() + "~" + getUsageExportedName(); + } + + public static IsolationKey fromString(String s) throws Version.ParseException { + List isolationKeyParts = Splitter.on("~").splitToList(s); + return ModuleExtensionId.IsolationKey.create( + ModuleKey.fromString(isolationKeyParts.get(0)), isolationKeyParts.get(1)); + } } public abstract Label getBzlFileLabel(); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleKey.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleKey.java index 5ae1c263366aba..1aae4ed38856ea 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleKey.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleKey.java @@ -16,9 +16,11 @@ package com.google.devtools.build.lib.bazel.bzlmod; import com.google.auto.value.AutoValue; +import com.google.common.base.Splitter; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.cmdline.RepositoryName; import java.util.Comparator; +import java.util.List; /** A module name, version pair that identifies a module in the external dependency graph. */ @AutoValue @@ -82,4 +84,17 @@ public RepositoryName getCanonicalRepoName() { return RepositoryName.createUnvalidated( String.format("%s~%s", getName(), getVersion().isEmpty() ? "override" : getVersion())); } + + public static ModuleKey fromString(String s) throws Version.ParseException { + if (s.equals("")) { + return ModuleKey.ROOT; + } + List parts = Splitter.on('@').splitToList(s); + if (parts.get(1).equals("_")) { + return ModuleKey.create(parts.get(0), Version.EMPTY); + } + + Version version = Version.parse(parts.get(1)); + return ModuleKey.create(parts.get(0), version); + } } diff --git a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py index 1bbed9274f7392..df592803066fdb 100644 --- a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py +++ b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py @@ -300,6 +300,58 @@ def testModuleExtension(self): _, _, stderr = self.RunBazel(['build', '@hello//:all']) self.assertNotIn('Hello from the other side!', ''.join(stderr)) + def testIsolatedModuleExtension(self): + self.ScratchFile( + 'MODULE.bazel', + [ + ( + 'lockfile_ext = use_extension("extension.bzl", "lockfile_ext",' + ' isolate = True)' + ), + 'lockfile_ext.dep(name = "bmbm", versions = ["v1", "v2"])', + 'use_repo(lockfile_ext, "hello")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'extension.bzl', + [ + 'def _repo_rule_impl(ctx):', + ' ctx.file("WORKSPACE")', + ' ctx.file("BUILD", "filegroup(name=\'lala\')")', + '', + 'repo_rule = repository_rule(implementation=_repo_rule_impl)', + '', + 'def _module_ext_impl(ctx):', + ' print("Hello from the other side!")', + ' repo_rule(name="hello")', + ' for mod in ctx.modules:', + ' for dep in mod.tags.dep:', + ' print("Name:", dep.name, ", Versions:", dep.versions)', + '', + '_dep = tag_class(attrs={"name": attr.string(), "versions":', + ' attr.string_list()})', + 'lockfile_ext = module_extension(', + ' implementation=_module_ext_impl,', + ' tag_classes={"dep": _dep},', + ' environ=["GREEN_TREES", "NOT_SET"],', + ')', + ], + ) + + # Only set one env var, to make sure null variables don't crash + _, _, stderr = self.RunBazel( + ['build', '@hello//:all'], env_add={'GREEN_TREES': 'In the city'} + ) + self.assertIn('Hello from the other side!', ''.join(stderr)) + self.assertIn('Name: bmbm , Versions: ["v1", "v2"]', ''.join(stderr)) + + self.RunBazel(['shutdown']) + _, _, stderr = self.RunBazel( + ['build', '@hello//:all'], env_add={'GREEN_TREES': 'In the city'} + ) + self.assertNotIn('Hello from the other side!', ''.join(stderr)) + def testModuleExtensionsInDifferentBuilds(self): # Test that the module extension stays in the lockfile (as long as it's # used in the module) even if it is not in the current build From ff55d363857071613101319fa9f3a01c3ffb063d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Thu, 20 Jul 2023 19:00:51 +0200 Subject: [PATCH 2/2] remove environ stuff from the test --- src/test/py/bazel/bzlmod/bazel_lockfile_test.py | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py index df592803066fdb..76f603b48e9c4c 100644 --- a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py +++ b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py @@ -334,22 +334,16 @@ def testIsolatedModuleExtension(self): 'lockfile_ext = module_extension(', ' implementation=_module_ext_impl,', ' tag_classes={"dep": _dep},', - ' environ=["GREEN_TREES", "NOT_SET"],', ')', ], ) - # Only set one env var, to make sure null variables don't crash - _, _, stderr = self.RunBazel( - ['build', '@hello//:all'], env_add={'GREEN_TREES': 'In the city'} - ) + _, _, stderr = self.RunBazel(['build', '@hello//:all']) self.assertIn('Hello from the other side!', ''.join(stderr)) self.assertIn('Name: bmbm , Versions: ["v1", "v2"]', ''.join(stderr)) self.RunBazel(['shutdown']) - _, _, stderr = self.RunBazel( - ['build', '@hello//:all'], env_add={'GREEN_TREES': 'In the city'} - ) + _, _, stderr = self.RunBazel(['build', '@hello//:all']) self.assertNotIn('Hello from the other side!', ''.join(stderr)) def testModuleExtensionsInDifferentBuilds(self):