[6.3.0] Fix absolute file paths showing up in lockfiles (#18993)

* Redact absolute root module file path in `MODULE.bazel.lock` The absolute path of the root module file in Starlark `Location`s is replaced with a constant when writing to the lock file. Work towards #18936 Closes #18949. PiperOrigin-RevId: 549118781 Change-Id: Ie689f7b5edf92296772c605845d694d872074214 * Use a label as the location of the MODULE.bazel file of non-registry overrides Fixes #18936 PiperOrigin-RevId: 549310245 Change-Id: I852570fbc81c1592c2fc0b3848d4b58e1c9ffb7d
bazelbuild · Jul 19, 2023 · 762c470 · 762c470
1 parent b13091a
commit 762c470
Show file tree

Hide file tree

Showing 6 changed files with 446 additions and 29 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileFunction.java
@@ -15,7 +15,6 @@
 
 package com.google.devtools.build.lib.bazel.bzlmod;
 
-import static com.google.devtools.build.lib.bazel.bzlmod.GsonTypeAdapterUtil.LOCKFILE_GSON;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.collect.ImmutableList;
@@ -93,7 +92,13 @@ public static BazelLockFileValue getLockfileValue(RootedPath lockfilePath) throw
     BazelLockFileValue bazelLockFileValue;
     try {
       String json = FileSystemUtils.readContent(lockfilePath.asPath(), UTF_8);
-      bazelLockFileValue = LOCKFILE_GSON.fromJson(json, BazelLockFileValue.class);
+      bazelLockFileValue =
+          GsonTypeAdapterUtil.createLockFileGson(
+                  lockfilePath
+                      .asPath()
+                      .getParentDirectory()
+                      .getRelative(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME))
+              .fromJson(json, BazelLockFileValue.class);
     } catch (FileNotFoundException e) {
       bazelLockFileValue = EMPTY_LOCKFILE;
     }

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java
@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.bazel.bzlmod;
 
-import static com.google.devtools.build.lib.bazel.bzlmod.GsonTypeAdapterUtil.LOCKFILE_GSON;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.collect.ImmutableMap;
@@ -136,7 +135,14 @@ private ImmutableMap<ModuleExtensionId, LockFileModuleExtension> combineModuleEx
   public static void updateLockfile(RootedPath lockfilePath, BazelLockFileValue updatedLockfile) {
     try {
       FileSystemUtils.writeContent(
-          lockfilePath.asPath(), UTF_8, LOCKFILE_GSON.toJson(updatedLockfile));
+          lockfilePath.asPath(),
+          UTF_8,
+          GsonTypeAdapterUtil.createLockFileGson(
+                  lockfilePath
+                      .asPath()
+                      .getParentDirectory()
+                      .getRelative(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME))
+              .toJson(updatedLockfile));
     } catch (IOException e) {
       logger.atSevere().withCause(e).log(
           "Error while updating MODULE.bazel.lock file: %s", e.getMessage());

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
@@ -20,11 +20,13 @@
 import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_MAP;
 import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_SET;
 
+import com.google.auto.value.AutoValue;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Splitter;
 import com.google.devtools.build.lib.bazel.bzlmod.Version.ParseException;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.vfs.Path;
 import com.google.gson.Gson;
 import com.google.gson.GsonBuilder;
 import com.google.gson.JsonParseException;
@@ -42,6 +44,7 @@
 import java.util.List;
 import java.util.Optional;
 import javax.annotation.Nullable;
+import net.starlark.java.syntax.Location;
 
 /**
  * Utility class to hold type adapters and helper methods to get gson registered with type adapters
@@ -188,24 +191,102 @@ public Optional<T> read(JsonReader jsonReader) throws IOException {
     }
   }
 
-  public static final Gson LOCKFILE_GSON =
-      new GsonBuilder()
-          .setPrettyPrinting()
-          .disableHtmlEscaping()
-          .enableComplexMapKeySerialization()
-          .registerTypeAdapterFactory(GenerateTypeAdapter.FACTORY)
-          .registerTypeAdapterFactory(DICT)
-          .registerTypeAdapterFactory(IMMUTABLE_MAP)
-          .registerTypeAdapterFactory(IMMUTABLE_LIST)
-          .registerTypeAdapterFactory(IMMUTABLE_BIMAP)
-          .registerTypeAdapterFactory(IMMUTABLE_SET)
-          .registerTypeAdapterFactory(OPTIONAL)
-          .registerTypeAdapter(Version.class, VERSION_TYPE_ADAPTER)
-          .registerTypeAdapter(ModuleKey.class, MODULE_KEY_TYPE_ADAPTER)
-          .registerTypeAdapter(ModuleExtensionId.class, MODULE_EXTENSION_ID_TYPE_ADAPTER)
-          .registerTypeAdapter(AttributeValues.class, new AttributeValuesAdapter())
-          .registerTypeAdapter(byte[].class, BYTE_ARRAY_TYPE_ADAPTER)
-          .create();
+  /**
+   * A variant of {@link Location} that converts the absolute path to the root module file to a
+   * constant and back.
+   */
+  // protected only for @AutoValue
+  @GenerateTypeAdapter
+  @AutoValue
+  protected abstract static class RootModuleFileEscapingLocation {
+    // This marker string is neither a valid absolute path nor a valid URL and thus cannot conflict
+    // with any real module file location.
+    private static final String ROOT_MODULE_FILE_LABEL = "@@//:MODULE.bazel";
+
+    public abstract String file();
+
+    public abstract int line();
+
+    public abstract int column();
+
+    public Location toLocation(String moduleFilePath) {
+      String file;
+      if (file().equals(ROOT_MODULE_FILE_LABEL)) {
+        file = moduleFilePath;
+      } else {
+        file = file();
+      }
+      return Location.fromFileLineColumn(file, line(), column());
+    }
+
+    public static RootModuleFileEscapingLocation fromLocation(
+        Location location, String moduleFilePath) {
+      String file;
+      if (location.file().equals(moduleFilePath)) {
+        file = ROOT_MODULE_FILE_LABEL;
+      } else {
+        file = location.file();
+      }
+      return new AutoValue_GsonTypeAdapterUtil_RootModuleFileEscapingLocation(
+          file, location.line(), location.column());
+    }
+  }
+
+  private static final class LocationTypeAdapterFactory implements TypeAdapterFactory {
+
+    private final String moduleFilePath;
+
+    public LocationTypeAdapterFactory(Path moduleFilePath) {
+      this.moduleFilePath = moduleFilePath.getPathString();
+    }
+
+    @Nullable
+    @Override
+    @SuppressWarnings("unchecked")
+    public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> typeToken) {
+      if (typeToken.getRawType() != Location.class) {
+        return null;
+      }
+      TypeAdapter<RootModuleFileEscapingLocation> relativizedLocationTypeAdapter =
+          gson.getAdapter(RootModuleFileEscapingLocation.class);
+      return (TypeAdapter<T>)
+          new TypeAdapter<Location>() {
+
+            @Override
+            public void write(JsonWriter jsonWriter, Location location) throws IOException {
+              relativizedLocationTypeAdapter.write(
+                  jsonWriter,
+                  RootModuleFileEscapingLocation.fromLocation(location, moduleFilePath));
+            }
+
+            @Override
+            public Location read(JsonReader jsonReader) throws IOException {
+              return relativizedLocationTypeAdapter.read(jsonReader).toLocation(moduleFilePath);
+            }
+          };
+    }
+  }
+
+  public static Gson createLockFileGson(Path moduleFilePath) {
+    return new GsonBuilder()
+        .setPrettyPrinting()
+        .disableHtmlEscaping()
+        .enableComplexMapKeySerialization()
+        .registerTypeAdapterFactory(GenerateTypeAdapter.FACTORY)
+        .registerTypeAdapterFactory(DICT)
+        .registerTypeAdapterFactory(IMMUTABLE_MAP)
+        .registerTypeAdapterFactory(IMMUTABLE_LIST)
+        .registerTypeAdapterFactory(IMMUTABLE_BIMAP)
+        .registerTypeAdapterFactory(IMMUTABLE_SET)
+        .registerTypeAdapterFactory(OPTIONAL)
+        .registerTypeAdapterFactory(new LocationTypeAdapterFactory(moduleFilePath))
+        .registerTypeAdapter(Version.class, VERSION_TYPE_ADAPTER)
+        .registerTypeAdapter(ModuleKey.class, MODULE_KEY_TYPE_ADAPTER)
+        .registerTypeAdapter(ModuleExtensionId.class, MODULE_EXTENSION_ID_TYPE_ADAPTER)
+        .registerTypeAdapter(AttributeValues.class, new AttributeValuesAdapter())
+        .registerTypeAdapter(byte[].class, BYTE_ARRAY_TYPE_ADAPTER)
+        .create();
+  }
 
   private GsonTypeAdapterUtil() {}
 }
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
@@ -24,7 +24,9 @@
 import com.google.devtools.build.lib.actions.FileValue;
 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.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelConstants;
+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.packages.StarlarkExportable;
@@ -37,6 +39,7 @@
 import com.google.devtools.build.lib.util.Fingerprint;
 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.skyframe.SkyFunction;
@@ -199,11 +202,11 @@ private SkyValue computeForRootModule(StarlarkSemantics starlarkSemantics, Envir
     if (env.getValue(FileValue.key(moduleFilePath)) == null) {
       return null;
     }
-    ModuleFile moduleFile = readModuleFile(moduleFilePath.asPath());
-    String moduleFileHash = new Fingerprint().addBytes(moduleFile.getContent()).hexDigestAndReset();
+    byte[] moduleFileContents = readModuleFile(moduleFilePath.asPath());
+    String moduleFileHash = new Fingerprint().addBytes(moduleFileContents).hexDigestAndReset();
     ModuleFileGlobals moduleFileGlobals =
         execModuleFile(
-            moduleFile,
+            ModuleFile.create(moduleFileContents, moduleFilePath.asPath().toString()),
             /* registry= */ null,
             ModuleKey.ROOT,
             /* ignoreDevDeps= */ Objects.requireNonNull(IGNORE_DEV_DEPS.get(env)),
@@ -335,8 +338,15 @@ private GetModuleFileResult getModuleFile(
       if (env.getValue(FileValue.key(moduleFilePath)) == null) {
         return null;
       }
+      Label moduleFileLabel =
+          Label.createUnvalidated(
+              PackageIdentifier.create(key.getCanonicalRepoName(), PathFragment.EMPTY_FRAGMENT),
+              LabelConstants.MODULE_DOT_BAZEL_FILE_NAME.getBaseName());
       GetModuleFileResult result = new GetModuleFileResult();
-      result.moduleFile = readModuleFile(moduleFilePath.asPath());
+      result.moduleFile =
+          ModuleFile.create(
+              readModuleFile(moduleFilePath.asPath()),
+              moduleFileLabel.getUnambiguousCanonicalForm());
       return result;
     }
 
@@ -392,10 +402,9 @@ private GetModuleFileResult getModuleFile(
     throw errorf(Code.MODULE_NOT_FOUND, "module not found in registries: %s", key);
   }
 
-  private static ModuleFile readModuleFile(Path path) throws ModuleFileFunctionException {
+  private static byte[] readModuleFile(Path path) throws ModuleFileFunctionException {
     try {
-      return ModuleFile.create(
-          FileSystemUtils.readWithKnownFileSize(path, path.getFileSize()), path.getPathString());
+      return FileSystemUtils.readWithKnownFileSize(path, path.getFileSize());
     } catch (IOException e) {
       throw errorf(Code.MODULE_NOT_FOUND, "MODULE.bazel expected but not found at %s", path);
     }

diff --git a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py
@@ -505,6 +505,70 @@ def testRemoveModuleExtensionsNotUsed(self):
       lockfile = json.loads(f.read().strip())
       self.assertEqual(len(lockfile['moduleExtensions']), 0)
 
+  def testNoAbsoluteRootModuleFilePath(self):
+    self.ScratchFile(
+        'MODULE.bazel',
+        [
+            'ext = use_extension("extension.bzl", "ext")',
+            'ext.dep(generate = True)',
+            'use_repo(ext, ext_hello = "hello")',
+            'other_ext = use_extension("extension.bzl", "other_ext")',
+            'other_ext.dep(generate = False)',
+            'use_repo(other_ext, other_ext_hello = "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):',
+            '    for mod in ctx.modules:',
+            '        for dep in mod.tags.dep:',
+            '            if dep.generate:',
+            '                repo_rule(name="hello")',
+            '',
+            '_dep = tag_class(attrs={"generate": attr.bool()})',
+            'ext = module_extension(',
+            '    implementation=_module_ext_impl,',
+            '    tag_classes={"dep": _dep},',
+            ')',
+            'other_ext = module_extension(',
+            '    implementation=_module_ext_impl,',
+            '    tag_classes={"dep": _dep},',
+            ')',
+        ],
+    )
+
+    # Paths to module files in error message always use forward slashes as
+    # separators, even on Windows.
+    module_file_path = self.Path('MODULE.bazel').replace('\\', '/')
+
+    self.RunBazel(['build', '--nobuild', '@ext_hello//:all'])
+    with open(self.Path('MODULE.bazel.lock'), 'r') as f:
+      self.assertNotIn(module_file_path, f.read())
+
+    self.RunBazel(['shutdown'])
+    exit_code, _, stderr = self.RunBazel(
+        ['build', '--nobuild', '@other_ext_hello//:all'], allow_failure=True
+    )
+    self.AssertNotExitCode(exit_code, 0, stderr)
+    self.assertIn(
+        (
+            'ERROR: module extension "other_ext" from "//:extension.bzl" does '
+            'not generate repository "hello", yet it is imported as '
+            '"other_ext_hello" in the usage at '
+            + module_file_path
+            + ':4:26'
+        ),
+        stderr,
+    )
+
 
 if __name__ == '__main__':
   unittest.main()