Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Redact absolute root module file path in MODULE.bazel.lock #18949

Closed
wants to merge 2 commits into from
+171 −22
Closed

Redact absolute root module file path in MODULE.bazel.lock #18949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a9aa79a
Relativize absolute root module file path in `MODULE.bazel.lock`
fmeum Jul 14, 2023
ad98a7a
Merge branch 'master' into 18936-absolute-paths-in-lock-file
Wyverald Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down
10 changes: 8 additions & 2 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand Down
115 changes: 97 additions & 18 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -188,24 +191,100 @@ 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";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for the fix, but I was thinking that we could just actually use the label in error messages too (i.e. change the logic in ModuleFileFunction). IMO it's pretty clear which file @@//:MODULE.bazel (or better, if we use Label#getDisplayForm(), //:MODULE.bazel) is referring to. That would also remove this special-casing at lockfile read/write time. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that absolute paths still provide the best usability here: You can often just copy&paste the path together with the line number to go to the correct place in your editor. All other Starlark stack traces also contain absolute paths in this place. Building //:MODULE.bazel or querying for it also results in an error about the file not being exported.

I view the switch to labels as a tradeoff that reduces implementation complexity at the cost of usability. This is definitely warranted for non-registry overrides, which are a bit more of an "advanced" concept anyway. But if we can keep absolute paths for the root module file, which I expect to cause most errors arising in practice, with a reasonable amount of complexity, then we should consider doing that.

I am actually not sure anymore whether the display form is better for non-root modules: The canonical form has the advantage that it more directly corresponds to the external path on disk, which is ultimately what the user wants to find if they face an error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely warranted for non-registry overrides, which are a bit more of an "advanced" concept anyway.

To be clear, is your preference to use labels for non-registry overrides in error messages?

I am actually not sure anymore whether the display form is better for non-root modules: The canonical form has the advantage that it more directly corresponds to the external path on disk, which is ultimately what the user wants to find if they face an error.

I don't really mind it either way. The difference would only be @foo//:MODULE.bazel vs @@foo~override//:MODULE.bazel, which is not a huge deal IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely warranted for non-registry overrides, which are a bit more of an "advanced" concept anyway.

To be clear, is your preference to use labels for non-registry overrides in error messages?

Ideally we would also keep absolute paths for them. But I can see how that would introduce too much complexity (tracking and replacing the output base) for too little gain (non-registry overrides were explicitly added by the user, so they should have a better idea where their module files live). That's why I consider labels a decent tradeoff in this case.

I am actually not sure anymore whether the display form is better for non-root modules: The canonical form has the advantage that it more directly corresponds to the external path on disk, which is ultimately what the user wants to find if they face an error.

I don't really mind it either way. The difference would only be @foo//:MODULE.bazel vs @@foo~override//:MODULE.bazel, which is not a huge deal IMO.

Couldn't it be @my_gazelle//:MODULE.bazel vs @@gazelle~override//:MODULE.bazel? In that case, the former could make it harder to locate the directory in external. But of course it's also more easily recognized by the user, so I'm not completely sure which I like better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright. This is holding up 6.3.0, and while I'm not a huge fan of the special de/serialization logic, I don't have a better idea right now. So let's get this submitted (and I'll fast track the label fix for non-registry overrides). Thanks!


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();
}

@Override
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() {}
}
59 changes: 59 additions & 0 deletions src/test/py/bazel/bzlmod/bazel_lockfile_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,65 @@ 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)

def testModuleExtensionEnvVariable(self):
self.ScratchFile(
'MODULE.bazel',
Expand Down
Loading