From 6d8f8f9925a804b928cf6b900840353e440352cb Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 24 Sep 2024 12:15:43 -0700 Subject: [PATCH] Apply `single_version_override` patches to module files Work towards #19301 RELNOTES: Patches to the module file in `single_version_override` are now effective as long as the patch file lies in the root module. Closes #23536. PiperOrigin-RevId: 678347809 Change-Id: I6a3c4664e55cff90c8e42795f95d8ef211348ca9 --- BUILD | 8 ++ .../devtools/build/lib/bazel/bzlmod/BUILD | 3 + .../lib/bazel/bzlmod/ModuleFileFunction.java | 123 +++++++++++++++- .../lib/bazel/bzlmod/ModuleFileGlobals.java | 9 +- .../build/lib/bazel/repository/PatchUtil.java | 46 +++++- .../bazel/bzlmod/ModuleFileFunctionTest.java | 131 ++++++++++++++++++ third_party/upb/BUILD | 1 + 7 files changed, 309 insertions(+), 12 deletions(-) diff --git a/BUILD b/BUILD index b49eb116ba7f45..03480d15174f84 100644 --- a/BUILD +++ b/BUILD @@ -103,6 +103,14 @@ genrule( "MODULE.bazel", "//third_party/googleapis:MODULE.bazel", "//third_party/remoteapis:MODULE.bazel", + "//third_party:BUILD", + "//third_party:rules_jvm_external_6.0.patch", + "//third_party:protobuf_21.7.patch", + "//third_party/upb:BUILD", + "//third_party/upb:00_remove_toolchain_transition.patch", + "//third_party/upb:01_remove_werror.patch", + "//third_party/grpc:BUILD", + "//third_party/grpc:00_disable_layering_check.patch", ], outs = ["MODULE.bazel.lock.dist"], cmd = "touch BUILD && " + 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 511a6ff0eedd0b..c02e7e8658fc6a 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 @@ -241,6 +241,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:blaze_version_info", "//src/main/java/com/google/devtools/build/lib/bazel:bazel_version", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:repo_rule_value", + "//src/main/java/com/google/devtools/build/lib/bazel/repository", "//src/main/java/com/google/devtools/build/lib/bazel/repository:repository_options", "//src/main/java/com/google/devtools/build/lib/bazel/repository/cache", "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", @@ -260,6 +261,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", + "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs", "//src/main/java/com/google/devtools/build/skyframe", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/main/java/net/starlark/java/annot", @@ -270,6 +272,7 @@ java_library( "//third_party:auto_value", "//third_party:gson", "//third_party:guava", + "//third_party:java-diff-utils", "//third_party:jsr305", ], ) 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 516559c214fcd6..fa8f303938ada1 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 @@ -20,14 +20,17 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import static java.nio.charset.StandardCharsets.UTF_8; +import com.github.difflib.patch.PatchFailedException; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import com.google.common.collect.Maps; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.bazel.bzlmod.CompiledModuleFile.IncludeStatement; import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue.NonRootModuleFileValue; import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue.RootModuleFileValue; +import com.google.devtools.build.lib.bazel.repository.PatchUtil; import com.google.devtools.build.lib.bazel.repository.downloader.Checksum.MissingChecksumException; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelConstants; @@ -50,11 +53,13 @@ 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.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.RootedPath; +import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyFunctionException.Transience; @@ -613,11 +618,15 @@ private GetModuleFileResult getModuleFile( StoredEventHandler downloadEventHandler = new StoredEventHandler(); for (Registry registry : registryObjects) { try { - Optional moduleFile = registry.getModuleFile(key, downloadEventHandler); - if (moduleFile.isEmpty()) { + Optional maybeModuleFile = registry.getModuleFile(key, downloadEventHandler); + if (maybeModuleFile.isEmpty()) { continue; } - return new GetModuleFileResult(moduleFile.get(), registry, downloadEventHandler); + ModuleFile moduleFile = maybePatchModuleFile(maybeModuleFile.get(), override, env); + if (moduleFile == null) { + return null; + } + return new GetModuleFileResult(moduleFile, registry, downloadEventHandler); } catch (MissingChecksumException e) { throw new ModuleFileFunctionException( ExternalDepsException.withCause(Code.BAD_LOCKFILE, e)); @@ -630,6 +639,114 @@ private GetModuleFileResult getModuleFile( throw errorf(Code.MODULE_NOT_FOUND, "module not found in registries: %s", key); } + /** + * Applies any patches specified in registry overrides. + * + *

This allows users to modify MODULE.bazel files and thus influence resolution and visibility + * for modules via patches without having to replace the entire module via a non-registry + * override. + * + *

Note: Only patch files from the main repo are applied, all other patches are ignored. This + * is necessary as we can't load other repos during resolution (unless they are subject to a + * non-registry override). Patch commands are also not supported as they cannot be selectively + * applied to the module file only. + */ + @Nullable + private ModuleFile maybePatchModuleFile( + ModuleFile moduleFile, ModuleOverride override, Environment env) + throws InterruptedException, ModuleFileFunctionException { + if (!(override instanceof SingleVersionOverride singleVersionOverride)) { + return moduleFile; + } + var patchesInMainRepo = + singleVersionOverride.getPatches().stream() + .filter(label -> label.getRepository().isMain()) + .collect(toImmutableList()); + if (patchesInMainRepo.isEmpty()) { + return moduleFile; + } + + // Get the patch paths. + var patchPackageLookupKeys = + patchesInMainRepo.stream() + .map(Label::getPackageIdentifier) + .map(pkg -> (SkyKey) PackageLookupValue.key(pkg)) + .collect(toImmutableList()); + var patchPackageLookupResult = env.getValuesAndExceptions(patchPackageLookupKeys); + if (env.valuesMissing()) { + return null; + } + List patchPaths = new ArrayList<>(); + for (int i = 0; i < patchesInMainRepo.size(); i++) { + var pkgLookupValue = + (PackageLookupValue) patchPackageLookupResult.get(patchPackageLookupKeys.get(i)); + if (pkgLookupValue == null) { + return null; + } + if (!pkgLookupValue.packageExists()) { + Label label = patchesInMainRepo.get(i); + throw errorf( + Code.BAD_MODULE, + "unable to load package for single_version_override patch %s: %s", + label, + PackageLookupFunction.explainNoBuildFileValue(label.getPackageIdentifier(), env)); + } + patchPaths.add( + RootedPath.toRootedPath( + pkgLookupValue.getRoot(), patchesInMainRepo.get(i).toPathFragment())); + } + + // Register dependencies on the patch files and ensure that they exist. + var fileKeys = Iterables.transform(patchPaths, FileValue::key); + var fileValueResult = env.getValuesAndExceptions(fileKeys); + if (env.valuesMissing()) { + return null; + } + for (var key : fileKeys) { + var fileValue = (FileValue) fileValueResult.get(key); + if (fileValue == null) { + return null; + } + if (!fileValue.isFile()) { + throw errorf( + Code.BAD_MODULE, + "error reading single_version_override patch %s: not a regular file", + key.argument()); + } + } + + // Apply the patches to the module file only. + InMemoryFileSystem patchFs = new InMemoryFileSystem(DigestHashFunction.SHA256); + try { + Path moduleRoot = patchFs.getPath("/module"); + moduleRoot.createDirectoryAndParents(); + Path moduleFilePath = moduleRoot.getChild("MODULE.bazel"); + FileSystemUtils.writeContent(moduleFilePath, moduleFile.getContent()); + for (var patchPath : patchPaths) { + try { + PatchUtil.applyToSingleFile( + patchPath.asPath(), + singleVersionOverride.getPatchStrip(), + moduleRoot, + moduleFilePath); + } catch (PatchFailedException e) { + throw errorf( + Code.BAD_MODULE, + "error applying single_version_override patch %s to module file: %s", + patchPath.asPath(), + e.getMessage()); + } + } + return ModuleFile.create( + FileSystemUtils.readContent(moduleFilePath), moduleFile.getLocation()); + } catch (IOException e) { + throw errorf( + Code.BAD_MODULE, + "error applying single_version_override patches to module file: %s", + e.getMessage()); + } + } + private static byte[] readModuleFile(Path path) throws ModuleFileFunctionException { try { return FileSystemUtils.readWithKnownFileSize(path, path.getFileSize()); 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 f02f213d81ef5b..b3371ae77ca6e3 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 @@ -767,7 +767,10 @@ public void include(String label, StarlarkThread thread) doc = "A list of labels pointing to patch files to apply for this module. The patch files" + " must exist in the source tree of the top level project. They are applied in" - + " the list order.", + + " the list order." + + "" + + "

If a patch makes changes to the MODULE.bazel file, these changes will" + + " only be effective if the patch file is provided by the root module.", allowedTypes = {@ParamType(type = Iterable.class, generic1 = String.class)}, named = true, positional = false, @@ -775,7 +778,9 @@ public void include(String label, StarlarkThread thread) @Param( name = "patch_cmds", doc = - "Sequence of Bash commands to be applied on Linux/Macos after patches are applied.", + "Sequence of Bash commands to be applied on Linux/Macos after patches are applied." + + "" + + "

Changes to the MODULE.bazel file will not be effective.", allowedTypes = {@ParamType(type = Iterable.class, generic1 = String.class)}, named = true, positional = false, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/PatchUtil.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/PatchUtil.java index 0bafe05d9fdc73..08912bb7db89fb 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/PatchUtil.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/PatchUtil.java @@ -417,10 +417,32 @@ private static void checkFilesStatusForPatching( * * @param patchFile the patch file to apply * @param strip the number of leading components to strip from file path in the patch file - * @param outputDirectory the repository directory to apply the patch file + * @param outputDirectory the directory to apply the patch file to */ public static void apply(Path patchFile, int strip, Path outputDirectory) throws IOException, PatchFailedException { + applyInternal(patchFile, strip, outputDirectory, /* singleFile= */ null); + } + + /** + * Apply a patch file under a directory, skipping all parts of the patch file that do not apply to + * the given single file. + * + * @param patchFile the patch file to apply + * @param strip the number of leading components to strip from file path in the patch file + * @param outputDirectory the directory to apply the patch file to + * @param singleFile only apply the parts of the patch file that apply to this file. Renaming the + * file is not supported in this case. + */ + public static void applyToSingleFile( + Path patchFile, int strip, Path outputDirectory, Path singleFile) + throws IOException, PatchFailedException { + applyInternal(patchFile, strip, outputDirectory, singleFile); + } + + private static void applyInternal( + Path patchFile, int strip, Path outputDirectory, @Nullable Path singleFile) + throws IOException, PatchFailedException { if (!patchFile.exists()) { throw new PatchFailedException("Cannot find patch file: " + patchFile.getPathString()); } @@ -565,15 +587,25 @@ public static void apply(Path patchFile, int strip, Path outputDirectory) patchContent, header, oldLineCount, newLineCount, patchStartLocation); if (isRenaming) { - checkFilesStatusForRenaming( - oldFile, newFile, oldFileStr, newFileStr, patchStartLocation); + if (singleFile != null) { + if (singleFile.equals(newFile) || singleFile.equals(oldFile)) { + throw new PatchFailedException( + "Renaming %s while applying patches to it as a single file is not supported." + .formatted(singleFile)); + } + } else { + checkFilesStatusForRenaming( + oldFile, newFile, oldFileStr, newFileStr, patchStartLocation); + } } - Patch patch = UnifiedDiffUtils.parseUnifiedDiff(patchContent); - checkFilesStatusForPatching( - patch, oldFile, newFile, oldFileStr, newFileStr, patchStartLocation); + if (singleFile == null || (singleFile.equals(newFile) && singleFile.equals(oldFile))) { + Patch patch = UnifiedDiffUtils.parseUnifiedDiff(patchContent); + checkFilesStatusForPatching( + patch, oldFile, newFile, oldFileStr, newFileStr, patchStartLocation); - applyPatchToFile(patch, oldFile, newFile, isRenaming, filePermission); + applyPatchToFile(patch, oldFile, newFile, isRenaming, filePermission); + } } patchContent.clear(); 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 8fe7c00203689d..60bbeedc804e3f 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 @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.actions.FileStateValue; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; @@ -35,6 +36,7 @@ import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryModule; import com.google.devtools.build.lib.clock.BlazeClock; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; @@ -62,8 +64,11 @@ import com.google.devtools.build.lib.testutil.TestRuleClassProvider; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.FileStateKey; +import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; +import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.skyframe.EvaluationContext; import com.google.devtools.build.skyframe.EvaluationResult; @@ -1630,4 +1635,130 @@ public void testInvalidUseExtensionLabel() throws Exception { + " name 'foo/bar:extensions.bzl': repo names may contain only A-Z, a-z, 0-9, '-'," + " '_', '.' and '~' and must not start with '~'"); } + + @Test + public void testSingleVersionOverridePatches() throws Exception { + FakeRegistry registry = registryFactory.newFakeRegistry("/foo"); + ModuleFileFunction.REGISTRIES.set(differencer, ImmutableSet.of(registry.getUrl())); + ModuleKey bbb = createModuleKey("bbb", "1.0"); + registry.addModule(bbb, "module(name='bbb',version='1.0')"); + + scratch.file("BUILD"); + scratch.file( + "patch.diff", + """ + diff --git a/MODULE.bazel b/MODULE.bazel + --- a/MODULE.bazel + +++ b/MODULE.bazel + @@ -1,1 +1,1 @@ + -module(name='bbb',version='1.0') + +module(name='bbb',version='1.0',bazel_compatibility=[">=7.0.0"]) + diff --git a/not/MODULE.bazel b/not/MODULE.bazel + --- a/not/MODULE.bazel + +++ b/not/MODULE.bazel + @@ -1,1 +1,2 @@ + module(name='bbb',version='1.0',bazel_compatibility=[">=7.0.0"]) + +bazel_dep(name='ccc',version='3.0') + diff --git a/also/not/MODULE.bazel b/also/not/MODULE.bazel.bak + similarity index 55% + rename from also/not/MODULE.bazel + rename to also/not/MODULE.bazel.bak + index 3f855b5..949dd15 100644 + """); + scratch.file("other/pkg/BUILD"); + Path otherPatch = + scratch.file( + "other/pkg/other_patch.diff", + """ + --- a/MODULE.bazel + +++ b/MODULE.bazel + @@ -1,1 +1,2 @@ + module(name='bbb',version='1.0',bazel_compatibility=[">=7.0.0"]) + +bazel_dep(name='ccc',version='3.0') + """); + + var moduleFileKey = + ModuleFileValue.key( + bbb, + SingleVersionOverride.create( + Version.EMPTY, + "", + ImmutableList.of( + Label.parseCanonicalUnchecked("//:patch.diff"), + Label.parseCanonicalUnchecked("@other_repo//:patch.diff"), + Label.parseCanonicalUnchecked("//other/pkg:other_patch.diff")), + ImmutableList.of(), + 1)); + EvaluationResult result = + evaluator.evaluate(ImmutableList.of(moduleFileKey), evaluationContext); + if (result.hasError()) { + throw result.getError().getException(); + } + assertThat(result.get(moduleFileKey).getModule().getBazelCompatibility()) + .isEqualTo(ImmutableList.of(">=7.0.0")); + assertThat(result.get(moduleFileKey).getModule().getDeps()) + .containsExactly( + "ccc", InterimModule.DepSpec.fromModuleKey(ModuleKey.create("ccc", Version.parse("3.0")))); + + FileSystemUtils.writeContentAsLatin1( + otherPatch, + """ + --- a/MODULE.bazel + +++ b/MODULE.bazel + @@ -1,1 +1,2 @@ + module(name='bbb',version='1.0',bazel_compatibility=[">=7.0.0"]) + +bazel_dep(name='ccc',version='2.0') + """); + differencer.invalidate( + ImmutableList.of( + FileStateValue.key(RootedPath.toRootedPath(Root.fromPath(rootDirectory), otherPatch)))); + + result = evaluator.evaluate(ImmutableList.of(moduleFileKey), evaluationContext); + if (result.hasError()) { + throw result.getError().getException(); + } + assertThat(result.get(moduleFileKey).getModule().getBazelCompatibility()) + .isEqualTo(ImmutableList.of(">=7.0.0")); + assertThat(result.get(moduleFileKey).getModule().getDeps()) + .containsExactly( + "ccc", InterimModule.DepSpec.fromModuleKey(ModuleKey.create("ccc", Version.parse("2.0")))); + } + + @Test + public void testSingleVersionOverridePatches_failsOnRename() throws Exception { + FakeRegistry registry = registryFactory.newFakeRegistry("/foo"); + ModuleFileFunction.REGISTRIES.set(differencer, ImmutableSet.of(registry.getUrl())); + ModuleKey bbb = createModuleKey("bbb", "1.0"); + registry.addModule(bbb, "module(name='bbb',version='1.0')"); + + scratch.file("BUILD"); + scratch.file( + "patch.diff", + """ + diff --git a/MODULE.bazel b/MODULE.bazel.bak + similarity index 55% + rename from MODULE.bazel + rename to MODULE.bazel.bak + index 3f855b5..949dd15 100644 + """); + + var moduleFileKey = + ModuleFileValue.key( + bbb, + SingleVersionOverride.create( + Version.EMPTY, + "", + ImmutableList.of(Label.parseCanonicalUnchecked("//:patch.diff")), + ImmutableList.of(), + 1)); + EvaluationResult result = + evaluator.evaluate(ImmutableList.of(moduleFileKey), evaluationContext); + assertThat(result.hasError()).isTrue(); + assertThat(result.getError().getException()) + .hasMessageThat() + .isEqualTo( + "error applying single_version_override patch /workspace/patch.diff to module file:" + + " Renaming /module/MODULE.bazel while applying patches to it as a single file is" + + " not supported."); + } } diff --git a/third_party/upb/BUILD b/third_party/upb/BUILD index e584dea3004709..b02bace9ed917f 100644 --- a/third_party/upb/BUILD +++ b/third_party/upb/BUILD @@ -7,5 +7,6 @@ filegroup( ) exports_files([ + "BUILD", "01_remove_werror.patch", ])