Skip to content

Commit

Permalink
Revert "user-specific path in lockfile" changes
Browse files Browse the repository at this point in the history
Revert "Remove user specific path from the lockfile (Fixes bazelbuild#19621)"

This reverts commit 173bd2b.

Revert "Remove user specific absolute path from lockfile"

This reverts commit 736a068.
  • Loading branch information
fmeum committed May 7, 2024
1 parent a077a02 commit ac71f10
Show file tree
Hide file tree
Showing 22 changed files with 81 additions and 123 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -270,8 +270,8 @@ SkyFunctions.BAZEL_LOCK_FILE, new BazelLockFileFunction(directories.getWorkspace
.addSkyFunction(
SkyFunctions.REGISTRY,
new RegistryFunction(
new RegistryFactoryImpl(
directories.getWorkspace(), downloadManager, clientEnvironmentSupplier)))
new RegistryFactoryImpl(downloadManager, clientEnvironmentSupplier),
directories.getWorkspace()))
.addSkyFunction(SkyFunctions.REPO_SPEC, new RepoSpecFunction())
.addSkyFunction(SkyFunctions.YANKED_VERSIONS, new YankedVersionsFunction())
.addSkyFunction(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/util:os",
"//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/skyframe:skyframe-objects",
"//third_party:gson",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
}

try (SilentCloseable c = Profiler.instance().profile(ProfilerTask.BZLMOD, "parse lockfile")) {
return getLockfileValue(lockfilePath, rootDirectory, LOCKFILE_MODE.get(env));
return getLockfileValue(lockfilePath, LOCKFILE_MODE.get(env));
} catch (IOException | JsonSyntaxException | NullPointerException e) {
throw new BazelLockfileFunctionException(
ExternalDepsException.withMessage(
Expand All @@ -83,7 +83,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
}

public static BazelLockFileValue getLockfileValue(
RootedPath lockfilePath, Path rootDirectory, LockfileMode lockfileMode)
RootedPath lockfilePath, LockfileMode lockfileMode)
throws IOException, BazelLockfileFunctionException {
try {
String json = FileSystemUtils.readContent(lockfilePath.asPath(), UTF_8);
Expand All @@ -94,8 +94,7 @@ public static BazelLockFileValue getLockfileValue(
lockfilePath
.asPath()
.getParentDirectory()
.getRelative(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME),
rootDirectory)
.getRelative(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME))
.fromJson(json, BazelLockFileValue.class);
} else {
// This is an old version, its information can't be used.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -255,8 +255,7 @@ private static void updateLockfile(Path workspaceRoot, BazelLockFileValue update
lockfilePath
.asPath()
.getParentDirectory()
.getRelative(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME),
workspaceRoot)
.getRelative(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME))
.toJson(updatedLockfile)
+ "\n");
} catch (IOException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -384,23 +384,23 @@ protected abstract static class RootModuleFileEscapingLocation {

public abstract int column();

public Location toLocation(String moduleFilePath, String workspaceRoot) {
public Location toLocation(String moduleFilePath) {
String file;
if (file().equals(ROOT_MODULE_FILE_LABEL)) {
file = moduleFilePath;
} else {
file = file().replace("%workspace%", workspaceRoot);
file = file();
}
return Location.fromFileLineColumn(file, line(), column());
}

public static RootModuleFileEscapingLocation fromLocation(
Location location, String moduleFilePath, String workspaceRoot) {
Location location, String moduleFilePath) {
String file;
if (location.file().equals(moduleFilePath)) {
file = ROOT_MODULE_FILE_LABEL;
} else {
file = location.file().replace(workspaceRoot, "%workspace%");
file = location.file();
}
return new AutoValue_GsonTypeAdapterUtil_RootModuleFileEscapingLocation(
file, location.line(), location.column());
Expand All @@ -410,11 +410,9 @@ public static RootModuleFileEscapingLocation fromLocation(
private static final class LocationTypeAdapterFactory implements TypeAdapterFactory {

private final String moduleFilePath;
private final String workspaceRoot;

public LocationTypeAdapterFactory(Path moduleFilePath, Path workspaceRoot) {
public LocationTypeAdapterFactory(Path moduleFilePath) {
this.moduleFilePath = moduleFilePath.getPathString();
this.workspaceRoot = workspaceRoot.getPathString();
}

@Nullable
Expand All @@ -433,15 +431,12 @@ public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> typeToken) {
public void write(JsonWriter jsonWriter, Location location) throws IOException {
relativizedLocationTypeAdapter.write(
jsonWriter,
RootModuleFileEscapingLocation.fromLocation(
location, moduleFilePath, workspaceRoot));
RootModuleFileEscapingLocation.fromLocation(location, moduleFilePath));
}

@Override
public Location read(JsonReader jsonReader) throws IOException {
return relativizedLocationTypeAdapter
.read(jsonReader)
.toLocation(moduleFilePath, workspaceRoot);
return relativizedLocationTypeAdapter.read(jsonReader).toLocation(moduleFilePath);
}
};
}
Expand Down Expand Up @@ -528,10 +523,10 @@ public Optional<Checksum> read(JsonReader jsonReader) throws IOException {
}
}

public static Gson createLockFileGson(Path moduleFilePath, Path workspaceRoot) {
public static Gson createLockFileGson(Path moduleFilePath) {
return newGsonBuilder()
.setPrettyPrinting()
.registerTypeAdapterFactory(new LocationTypeAdapterFactory(moduleFilePath, workspaceRoot))
.registerTypeAdapterFactory(new LocationTypeAdapterFactory(moduleFilePath))
.registerTypeAdapterFactory(new OptionalChecksumTypeAdapterFactory())
.create();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,6 @@ public enum KnownFileHashesMode {
ENFORCE
}

/** The unresolved version of the url. Ex: has %workspace% placeholder */
private final String unresolvedUri;

private final URI uri;
private final DownloadManager downloadManager;
private final Map<String, String> clientEnv;
Expand All @@ -80,13 +77,11 @@ public enum KnownFileHashesMode {

public IndexRegistry(
URI uri,
String unresolvedUri,
DownloadManager downloadManager,
Map<String, String> clientEnv,
ImmutableMap<String, Optional<Checksum>> knownFileHashes,
KnownFileHashesMode knownFileHashesMode) {
this.uri = uri;
this.unresolvedUri = unresolvedUri;
this.downloadManager = downloadManager;
this.clientEnv = clientEnv;
this.gson =
Expand All @@ -99,7 +94,7 @@ public IndexRegistry(

@Override
public String getUrl() {
return unresolvedUri;
return uri.toString();
}

private String constructUrl(String base, String... segments) {
Expand Down Expand Up @@ -176,7 +171,7 @@ public Optional<ModuleFile> getModuleFile(ModuleKey key, ExtendedEventHandler ev
throws IOException, InterruptedException {
String url =
constructUrl(
uri.toString(), "modules", key.getName(), key.getVersion().toString(), "MODULE.bazel");
getUrl(), "modules", key.getName(), key.getVersion().toString(), "MODULE.bazel");
Optional<byte[]> maybeContent = grabFile(url, eventHandler, /* useChecksum= */ true);
return maybeContent.map(content -> ModuleFile.create(content, url));
}
Expand Down Expand Up @@ -259,7 +254,8 @@ public RepoSpec getRepoSpec(ModuleKey key, ExtendedEventHandler eventHandler)
Optional<String> jsonString = grabJsonFile(jsonUrl, eventHandler, /* useChecksum= */ true);
if (jsonString.isEmpty()) {
throw new FileNotFoundException(
String.format("Module %s's %s not found in registry %s", key, SOURCE_JSON_FILENAME, uri));
String.format(
"Module %s's %s not found in registry %s", key, SOURCE_JSON_FILENAME, getUrl()));
}
SourceJson sourceJson = parseJson(jsonString.get(), jsonUrl, SourceJson.class);
switch (sourceJson.type) {
Expand Down Expand Up @@ -300,7 +296,7 @@ private Optional<BazelRegistryJson> getBazelRegistryJson(ExtendedEventHandler ev
var storedEventHandler = new StoredEventHandler();
bazelRegistryJson =
grabJson(
constructUrl(uri.toString(), "bazel_registry.json"),
constructUrl(getUrl(), "bazel_registry.json"),
BazelRegistryJson.class,
storedEventHandler,
/* useChecksum= */ true);
Expand Down Expand Up @@ -377,7 +373,7 @@ private RepoSpec createArchiveRepoSpec(
for (Map.Entry<String, String> entry : sourceJson.patches.entrySet()) {
remotePatches.put(
constructUrl(
unresolvedUri,
getUrl(),
"modules",
key.getName(),
key.getVersion().toString(),
Expand Down Expand Up @@ -415,7 +411,7 @@ public Optional<ImmutableMap<Version, String>> getYankedVersions(
throws IOException, InterruptedException {
Optional<MetadataJson> metadataJson =
grabJson(
constructUrl(uri.toString(), "modules", moduleName, "metadata.json"),
constructUrl(getUrl(), "modules", moduleName, "metadata.json"),
MetadataJson.class,
eventHandler,
// metadata.json is not immutable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode;
import com.google.devtools.build.lib.bazel.repository.downloader.Checksum;
import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager;
import com.google.devtools.build.lib.vfs.Path;
import java.net.URI;
import java.net.URISyntaxException;
import java.util.Map;
Expand All @@ -29,26 +28,22 @@

/** Prod implementation of {@link RegistryFactory}. */
public class RegistryFactoryImpl implements RegistryFactory {
private final Path workspacePath;
private final DownloadManager downloadManager;
private final Supplier<Map<String, String>> clientEnvironmentSupplier;

public RegistryFactoryImpl(
Path workspacePath,
DownloadManager downloadManager,
Supplier<Map<String, String>> clientEnvironmentSupplier) {
this.workspacePath = workspacePath;
DownloadManager downloadManager, Supplier<Map<String, String>> clientEnvironmentSupplier) {
this.downloadManager = downloadManager;
this.clientEnvironmentSupplier = clientEnvironmentSupplier;
}

@Override
public Registry createRegistry(
String unresolvedUrl,
String url,
ImmutableMap<String, Optional<Checksum>> knownFileHashes,
LockfileMode lockfileMode)
throws URISyntaxException {
URI uri = new URI(unresolvedUrl.replace("%workspace%", workspacePath.getPathString()));
URI uri = new URI(url);
if (uri.getScheme() == null) {
throw new URISyntaxException(
uri.toString(),
Expand All @@ -73,7 +68,6 @@ public Registry createRegistry(
};
return new IndexRegistry(
uri,
unresolvedUrl,
downloadManager,
clientEnvironmentSupplier.get(),
knownFileHashes,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode;
import com.google.devtools.build.lib.server.FailureDetails;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
import com.google.devtools.build.skyframe.SkyKey;
Expand All @@ -27,9 +28,11 @@
/** A simple SkyFunction that creates a {@link Registry} with a given URL. */
public class RegistryFunction implements SkyFunction {
private final RegistryFactory registryFactory;
private final Path workspaceRoot;

public RegistryFunction(RegistryFactory registryFactory) {
public RegistryFunction(RegistryFactory registryFactory, Path workspaceRoot) {
this.registryFactory = registryFactory;
this.workspaceRoot = workspaceRoot;
}

@Override
Expand All @@ -46,7 +49,9 @@ public SkyValue compute(SkyKey skyKey, Environment env)
RegistryKey key = (RegistryKey) skyKey.argument();
try {
return registryFactory.createRegistry(
key.getUrl(), lockfile.getRegistryFileHashes(), lockfileMode);
key.getUrl().replace("%workspace%", workspaceRoot.getPathString()),
lockfile.getRegistryFileHashes(),
lockfileMode);
} catch (URISyntaxException e) {
throw new RegistryException(
ExternalDepsException.withCauseAndMessage(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept
Event.warn(
String.format(
"Could not read metadata file for module %s from registry %s: %s",
key.getModuleName(), key.getRegistryUrl(), e.getMessage())));
key.getModuleKey().getName(), key.getRegistryUrl(), e.getMessage())));
// This is failing open: If we can't read the metadata file, we allow yanked modules to be
// fetched.
return YankedVersionsValue.create(Optional.empty());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,13 @@

package com.google.devtools.build.lib.skyframe;

import static com.google.common.collect.ImmutableMap.toImmutableMap;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.bazel.bzlmod.ArchiveRepoSpecBuilder;
import com.google.devtools.build.lib.bazel.bzlmod.AttributeValues;
import com.google.devtools.build.lib.bazel.bzlmod.BazelDepGraphValue;
import com.google.devtools.build.lib.bazel.bzlmod.BzlmodRepoRuleCreator;
import com.google.devtools.build.lib.bazel.bzlmod.BzlmodRepoRuleValue;
Expand Down Expand Up @@ -51,7 +49,6 @@
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.Set;
Expand Down Expand Up @@ -193,7 +190,7 @@ private BzlmodRepoRuleValue createRuleFromSpec(

var attributes =
ImmutableMap.<String, Object>builder()
.putAll(resolveRemotePatchesUrl(repoSpec).attributes())
.putAll(repoSpec.attributes().attributes())
.put("name", repositoryName.getName())
.buildOrThrow();
try {
Expand All @@ -217,35 +214,6 @@ private BzlmodRepoRuleValue createRuleFromSpec(
}
}

/* Resolves repo specs containing remote patches that are stored with %workspace% place holder*/
@SuppressWarnings("unchecked")
private AttributeValues resolveRemotePatchesUrl(RepoSpec repoSpec) {
if (repoSpec
.getRuleClass()
.equals(ArchiveRepoSpecBuilder.HTTP_ARCHIVE_PATH + "%http_archive")) {
return AttributeValues.create(
repoSpec.attributes().attributes().entrySet().stream()
.collect(
toImmutableMap(
Map.Entry::getKey,
e -> {
if (e.getKey().equals("remote_patches")) {
Map<String, String> remotePatches = (Map<String, String>) e.getValue();
return remotePatches.keySet().stream()
.collect(
toImmutableMap(
key ->
key.replace(
"%workspace%",
directories.getWorkspace().getPathString()),
remotePatches::get));
}
return e.getValue();
})));
}
return repoSpec.attributes();
}

// Starlark rules loaded from bazel_tools that may define Bazel module repositories and thus must
// be loaded without relying on any other modules.
private static final Set<String> BOOTSTRAP_RULE_CLASSES =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,7 @@ public BazelPackageLoader buildImpl() {
HttpDownloader httpDownloader = new HttpDownloader();
DownloadManager downloadManager = new DownloadManager(repositoryCache, httpDownloader);
RegistryFactory registryFactory =
new RegistryFactoryImpl(
directories.getWorkspace(), downloadManager, Suppliers.ofInstance(ImmutableMap.of()));
new RegistryFactoryImpl(downloadManager, Suppliers.ofInstance(ImmutableMap.of()));

// Allow tests to override the following functions to use fake registry or custom built-in
// modules
Expand All @@ -160,7 +159,9 @@ public BazelPackageLoader buildImpl() {
}
if (!this.extraSkyFunctions.containsKey(SkyFunctions.REGISTRY)) {
addExtraSkyFunctions(
ImmutableMap.of(SkyFunctions.REGISTRY, new RegistryFunction(registryFactory)));
ImmutableMap.of(
SkyFunctions.REGISTRY,
new RegistryFunction(registryFactory, directories.getWorkspace())));
}

addExtraSkyFunctions(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,9 @@ public ImmutableMap<SkyFunctionName, SkyFunction> getSkyFunctions(BlazeDirectori
SkyFunctions.SINGLE_EXTENSION_EVAL,
new SingleExtensionEvalFunction(directories, ImmutableMap::of, downloadManager))
.put(SkyFunctions.SINGLE_EXTENSION_USAGES, new SingleExtensionUsagesFunction())
.put(SkyFunctions.REGISTRY, new RegistryFunction(FakeRegistry.DEFAULT_FACTORY))
.put(
SkyFunctions.REGISTRY,
new RegistryFunction(FakeRegistry.DEFAULT_FACTORY, directories.getWorkspace()))
.put(SkyFunctions.REPO_SPEC, new RepoSpecFunction())
.put(SkyFunctions.YANKED_VERSIONS, new YankedVersionsFunction())
.put(
Expand Down
Loading

0 comments on commit ac71f10

Please sign in to comment.