Skip to content

Commit

Permalink
[8.0.1] Also apply --experimental_inprocess_symlink_creation to files…
Browse files Browse the repository at this point in the history 
…ets. (#24918)

This makes it possible to hardcode in-process and get rid of
out-of-process (in a future CL).

This change doesn't affect Bazel, as filesets only exist in Blaze.

PiperOrigin-RevId: 695279110
Change-Id: I404fa9a60b285f46e68697ec3342bf61831da36e
  • Loading branch information
@tjgq
tjgq authored Jan 14, 2025
1 parent 429a056 commit 6f63d52
Show file tree
Hide file tree
Showing 4 changed files with 109 additions and 66 deletions.
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ private void updateRunfilesTree(
case SKIP -> helper.clearRunfilesDirectory();
case EXTERNAL -> helper.createSymlinksUsingCommand(binTools, env, outErr);
case INTERNAL -> {
helper.createSymlinksDirectly(tree.getMapping());
helper.createRunfilesSymlinksDirectly(tree.getMapping());
outputManifest.createSymbolicLink(inputManifest);
}
}
Expand Down
79 changes: 47 additions & 32 deletions src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,32 @@ private Path getOutputManifest() {
return symlinkTreeRoot.getChild("MANIFEST");
}

interface TargetPathFunction<T> {
/** Obtains a symlink target path from a T. */
PathFragment get(T target) throws IOException;
}

/** Creates a symlink tree for a fileset by making VFS calls. */
public void createFilesetSymlinksDirectly(Map<PathFragment, PathFragment> symlinkMap)
throws IOException {
createSymlinksDirectly(symlinkMap, (path) -> path);
}

/** Creates a symlink tree for a runfiles by making VFS calls. */
public void createRunfilesSymlinksDirectly(Map<PathFragment, Artifact> symlinkMap)
throws IOException {
createSymlinksDirectly(
symlinkMap,
(artifact) ->
artifact.isSymlink()
// Unresolved symlinks are created textually.
? artifact.getPath().readSymbolicLink()
: artifact.getPath().asFragment());
}

/** Creates a symlink tree by making VFS calls. */
public void createSymlinksDirectly(Map<PathFragment, Artifact> symlinkMap) throws IOException {
private <T> void createSymlinksDirectly(
Map<PathFragment, T> symlinkMap, TargetPathFunction<T> targetPathFn) throws IOException {
// Our strategy is to minimize mutating file system operations as much as possible. Ideally, if
// there is an existing symlink tree with the expected contents, we don't make any changes. Our
// algorithm goes as follows:
Expand Down Expand Up @@ -129,14 +153,13 @@ public void createSymlinksDirectly(Map<PathFragment, Artifact> symlinkMap) throw
// 3. For every remaining entry in the node, create the corresponding file, symlink, or
// directory on disk. If it is a directory, recurse into that directory.
try (SilentCloseable c = Profiler.instance().profile("Create symlink tree in-process")) {
Preconditions.checkState(!filesetTree);
Directory root = new Directory();
for (Map.Entry<PathFragment, Artifact> entry : symlinkMap.entrySet()) {
Directory<T> root = new Directory<>();
for (Map.Entry<PathFragment, T> entry : symlinkMap.entrySet()) {
// This creates intermediate directory nodes as a side effect.
Directory parentDir = root.walk(entry.getKey().getParentDirectory());
Directory<T> parentDir = root.walk(entry.getKey().getParentDirectory());
parentDir.addSymlink(entry.getKey().getBaseName(), entry.getValue());
}
root.syncTreeRecursively(symlinkTreeRoot);
root.syncTreeRecursively(symlinkTreeRoot, targetPathFn);
createWorkspaceSubdirectory();
}
}
Expand Down Expand Up @@ -255,23 +278,23 @@ static ImmutableMap<PathFragment, PathFragment> processFilesetLinks(
return symlinks.buildOrThrow();
}

private static final class Directory {
private final Map<String, Artifact> symlinks = new HashMap<>();
private final Map<String, Directory> directories = new HashMap<>();
private static final class Directory<T> {
private final Map<String, T> symlinks = new HashMap<>();
private final Map<String, Directory<T>> directories = new HashMap<>();

void addSymlink(String basename, @Nullable Artifact artifact) {
symlinks.put(basename, artifact);
void addSymlink(String basename, @Nullable T target) {
symlinks.put(basename, target);
}

Directory walk(PathFragment dir) {
Directory result = this;
Directory<T> walk(PathFragment dir) {
Directory<T> result = this;
for (String segment : dir.segments()) {
result = result.directories.computeIfAbsent(segment, unused -> new Directory());
result = result.directories.computeIfAbsent(segment, unused -> new Directory<>());
}
return result;
}

void syncTreeRecursively(Path at) throws IOException {
void syncTreeRecursively(Path at, TargetPathFunction<T> targetPathFn) throws IOException {
// This is a reimplementation of the C++ code in build-runfiles.cc. This avoids having to ship
// a separate native tool to create a few runfiles.
// TODO(ulfjack): Measure performance.
Expand All @@ -294,7 +317,7 @@ void syncTreeRecursively(Path at) throws IOException {
String basename = dirent.getName();
Path next = at.getChild(basename);
if (symlinks.containsKey(basename)) {
Artifact value = symlinks.remove(basename);
T value = symlinks.remove(basename);
if (value == null) {
if (dirent.getType() != Dirent.Type.FILE) {
next.deleteTree();
Expand All @@ -307,39 +330,31 @@ void syncTreeRecursively(Path at) throws IOException {
if (dirent.getType() != Dirent.Type.SYMLINK) {
next.deleteTree();
}
FileSystemUtils.ensureSymbolicLink(next, getSymlinkTargetPath(value));
FileSystemUtils.ensureSymbolicLink(next, targetPathFn.get(value));
}
} else if (directories.containsKey(basename)) {
Directory nextDir = directories.remove(basename);
Directory<T> nextDir = directories.remove(basename);
if (dirent.getType() != Dirent.Type.DIRECTORY) {
next.deleteTree();
}
nextDir.syncTreeRecursively(at.getChild(basename));
nextDir.syncTreeRecursively(at.getChild(basename), targetPathFn);
} else {
at.getChild(basename).deleteTree();
}
}

for (Map.Entry<String, Artifact> entry : symlinks.entrySet()) {
for (Map.Entry<String, T> entry : symlinks.entrySet()) {
Path next = at.getChild(entry.getKey());
Artifact value = entry.getValue();
T value = entry.getValue();
if (value == null) {
FileSystemUtils.createEmptyFile(next);
} else {
FileSystemUtils.ensureSymbolicLink(next, getSymlinkTargetPath(value));
FileSystemUtils.ensureSymbolicLink(next, targetPathFn.get(value));
}
}
for (Map.Entry<String, Directory> entry : directories.entrySet()) {
entry.getValue().syncTreeRecursively(at.getChild(entry.getKey()));
for (Map.Entry<String, Directory<T>> entry : directories.entrySet()) {
entry.getValue().syncTreeRecursively(at.getChild(entry.getKey()), targetPathFn);
}
}
}

private static PathFragment getSymlinkTargetPath(Artifact artifact) throws IOException {
if (artifact.isSymlink()) {
// Unresolved symlinks are created textually.
return artifact.getPath().readSymbolicLink();
}
return artifact.getPath().asFragment();
}
}
60 changes: 34 additions & 26 deletions src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Function;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionExecutionException;
Expand Down Expand Up @@ -74,28 +74,14 @@ public void createSymlinks(
try {
if (outputService.canCreateSymlinkTree()) {
Path inputManifest = actionExecutionContext.getInputPath(action.getInputManifest());

Map<PathFragment, PathFragment> symlinks;
if (action.getRunfiles() != null) {
symlinks = Maps.transformValues(runfilesToMap(action), TO_PATH);
if (action.isFilesetTree()) {
symlinks = getFilesetMap(action, actionExecutionContext);
} else {
Preconditions.checkState(action.isFilesetTree());

ImmutableList<FilesetOutputSymlink> filesetLinks;
try {
filesetLinks =
actionExecutionContext
.getArtifactExpander()
.expandFileset(action.getInputManifest())
.symlinks();
} catch (MissingExpansionException e) {
throw new IllegalStateException(e);
}

symlinks =
SymlinkTreeHelper.processFilesetLinks(
filesetLinks,
action.getWorkspaceNameForFileset(),
actionExecutionContext.getExecRoot().asFragment());
// TODO(tjgq): This produces an incorrect path for unresolved symlinks, which should be
// created textually.
symlinks = Maps.transformValues(getRunfilesMap(action), TO_PATH);
}

outputService.createSymlinkTree(
Expand All @@ -107,11 +93,14 @@ public void createSymlinks(
// This is required because only the output manifest is considered an action output, so
// Skyframe does not clear the directory for us.
createSymlinkTreeHelper(action, actionExecutionContext).clearRunfilesDirectory();
} else if (action.getRunfileSymlinksMode() == RunfileSymlinksMode.INTERNAL
&& !action.isFilesetTree()) {
} else if (action.getRunfileSymlinksMode() == RunfileSymlinksMode.INTERNAL) {
try {
createSymlinkTreeHelper(action, actionExecutionContext)
.createSymlinksDirectly(runfilesToMap(action));
SymlinkTreeHelper helper = createSymlinkTreeHelper(action, actionExecutionContext);
if (action.isFilesetTree()) {
helper.createFilesetSymlinksDirectly(getFilesetMap(action, actionExecutionContext));
} else {
helper.createRunfilesSymlinksDirectly(getRunfilesMap(action));
}
} catch (IOException e) {
throw ActionExecutionException.fromExecException(
new EnvironmentalExecException(e, Code.SYMLINK_TREE_CREATION_IO_EXCEPTION), action);
Expand All @@ -132,7 +121,26 @@ public void createSymlinks(
}
}

private static Map<PathFragment, Artifact> runfilesToMap(SymlinkTreeAction action) {
private static ImmutableMap<PathFragment, PathFragment> getFilesetMap(
SymlinkTreeAction action, ActionExecutionContext actionExecutionContext) {
ImmutableList<FilesetOutputSymlink> filesetLinks;
try {
filesetLinks =
actionExecutionContext
.getArtifactExpander()
.expandFileset(action.getInputManifest())
.symlinks();
} catch (MissingExpansionException e) {
throw new IllegalStateException(e);
}

return SymlinkTreeHelper.processFilesetLinks(
filesetLinks,
action.getWorkspaceNameForFileset(),
actionExecutionContext.getExecRoot().asFragment());
}

private static Map<PathFragment, Artifact> getRunfilesMap(SymlinkTreeAction action) {
// This call outputs warnings about overlapping symlinks. However, since this has already been
// called by the SourceManifestAction, we silence the warnings here.
return action
Expand Down
34 changes: 27 additions & 7 deletions src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeHelperTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@
@RunWith(TestParameterInjector.class)
public final class SymlinkTreeHelperTest {

private enum TreeType {
RUNFILES,
FILESET
}

private final FileSystem fs = new InMemoryFileSystem(DigestHashFunction.SHA256);
private final Path execRoot = fs.getPath("/execroot");
private final ArtifactRoot outputRoot =
Expand Down Expand Up @@ -125,7 +130,8 @@ public void readMultilineManifest() {
}

@Test
public void createSymlinksDirectly(@TestParameter boolean replace) throws Exception {
public void createSymlinksDirectly(
@TestParameter TreeType treeType, @TestParameter boolean replace) throws Exception {
Path treeRoot = execRoot.getRelative("foo.runfiles");
Path inputManifestPath = execRoot.getRelative("foo.runfiles_manifest");
SymlinkTreeHelper helper =
Expand All @@ -137,11 +143,6 @@ public void createSymlinksDirectly(@TestParameter boolean replace) throws Except
FileSystemUtils.writeContent(file.getPath(), UTF_8, "content");
FileSystemUtils.ensureSymbolicLink(symlink.getPath(), "/path/to/target");

HashMap<PathFragment, Artifact> symlinkMap = new HashMap<>();
symlinkMap.put(PathFragment.create(WORKSPACE_NAME + "/empty"), null);
symlinkMap.put(PathFragment.create(WORKSPACE_NAME + "/file"), file);
symlinkMap.put(PathFragment.create(WORKSPACE_NAME + "/symlink"), symlink);

Path treeWorkspace = treeRoot.getRelative(WORKSPACE_NAME);
Path treeEmpty = treeWorkspace.getRelative("empty");
Path treeFile = treeWorkspace.getRelative("file");
Expand All @@ -156,7 +157,26 @@ public void createSymlinksDirectly(@TestParameter boolean replace) throws Except
treeWorkspace.chmod(000);
}

helper.createSymlinksDirectly(symlinkMap);
switch (treeType) {
case RUNFILES -> {
HashMap<PathFragment, Artifact> symlinkMap = new HashMap<>();
symlinkMap.put(PathFragment.create(WORKSPACE_NAME + "/empty"), null);
symlinkMap.put(PathFragment.create(WORKSPACE_NAME + "/file"), file);
symlinkMap.put(PathFragment.create(WORKSPACE_NAME + "/symlink"), symlink);

helper.createRunfilesSymlinksDirectly(symlinkMap);
}
case FILESET -> {
HashMap<PathFragment, PathFragment> symlinkMap = new HashMap<>();
symlinkMap.put(PathFragment.create(WORKSPACE_NAME + "/empty"), null);
symlinkMap.put(PathFragment.create(WORKSPACE_NAME + "/file"), file.getPath().asFragment());
symlinkMap.put(
PathFragment.create(WORKSPACE_NAME + "/symlink"),
PathFragment.create("/path/to/target"));

helper.createFilesetSymlinksDirectly(symlinkMap);
}
}

assertThat(treeRoot.isDirectory()).isTrue();
assertThat(treeWorkspace.isDirectory()).isTrue();
Expand Down

0 comments on commit 6f63d52

Please sign in to comment.