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 Apr 26, 2024
1 parent 03efb76 commit 7d5efb6
Show file tree
Hide file tree
Showing 12 changed files with 46 additions and 117 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ SkyFunctions.BAZEL_LOCK_FILE, new BazelLockFileFunction(directories.getWorkspace
SkyFunctions.REGISTRY,
new RegistryFunction(
new RegistryFactoryImpl(
directories.getWorkspace(), downloadManager, clientEnvironmentSupplier)))
downloadManager, clientEnvironmentSupplier)))
.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 @@ -88,7 +88,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 @@ -17,7 +17,6 @@

import static java.nio.charset.StandardCharsets.UTF_8;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode;
Expand Down Expand Up @@ -78,7 +77,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)

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

public static BazelLockFileValue getLockfileValue(RootedPath lockfilePath, Path rootDirectory)
throws IOException {
public static BazelLockFileValue getLockfileValue(RootedPath lockfilePath) throws IOException {
BazelLockFileValue bazelLockFileValue;
try {
String json = FileSystemUtils.readContent(lockfilePath.asPath(), UTF_8);
Expand All @@ -114,8 +112,7 @@ public static BazelLockFileValue getLockfileValue(RootedPath lockfilePath, Path
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, needs to be updated
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -252,8 +252,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 @@ -527,10 +522,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 @@ -58,9 +58,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 @@ -74,13 +71,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 @@ -93,7 +88,7 @@ public IndexRegistry(

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

private String constructUrl(String base, String... segments) {
Expand Down Expand Up @@ -165,7 +160,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 @@ -246,15 +241,12 @@ public RepoSpec getRepoSpec(ModuleKey key, ExtendedEventHandler eventHandler)
throws IOException, InterruptedException {
String jsonUrl =
constructUrl(
uri.toString(),
"modules",
key.getName(),
key.getVersion().toString(),
SOURCE_JSON_FILENAME);
getUrl(), "modules", key.getName(), key.getVersion().toString(), SOURCE_JSON_FILENAME);
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 @@ -291,7 +283,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 @@ -368,7 +360,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 @@ -406,7 +398,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 @@ -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 Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager;
import com.google.devtools.build.lib.bazel.repository.downloader.HttpDownloader;
import com.google.devtools.build.lib.testutil.FoundationTestCase;
import com.google.devtools.build.lib.vfs.Path;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.IOException;
Expand Down Expand Up @@ -85,12 +84,10 @@ public ImmutableMap<String, Optional<Checksum>> getRecordedHashes() {
public void setUp() throws Exception {
eventRecorder = new EventRecorder();
eventBus.register(eventRecorder);
Path workspaceRoot = scratch.dir("/ws");
repositoryCache = new RepositoryCache();
downloadManager = new DownloadManager(repositoryCache, new HttpDownloader());
registryFactory =
new RegistryFactoryImpl(
workspaceRoot, downloadManager, Suppliers.ofInstance(ImmutableMap.of()));
new RegistryFactoryImpl(downloadManager, Suppliers.ofInstance(ImmutableMap.of()));
}

@Test
Expand Down
Loading

0 comments on commit 7d5efb6

Please sign in to comment.