OutputService: simplify interface
Originally, OutputService.createSymlinkTree had to read the MANIFEST file
in order to get the list of symlinks. This was problematic, because the
file is not round-trip safe for paths containing whitespace characters.
In addition, it was responsible for creating a symlink from the output
manifest location to the input manifest location, which all subclasses
implemented in the same way.
Instead, it now receives a map representation of the symlinks, and the
creation of the output manifest symlink is the responsibility of the
action.
The current change is primarily a cleanup to reduce code duplication.
However, this change also makes it possible to globally change the
contents of the output manifest. By making it a non-symlink (e.g.,
a hash of the symlink map), we can decouple the symlink tree action
from the manifest writing action. We could run both in parallel, or
even skip writing the input manifest entirely (though note that the
input manifest is currently still necessary in some cases because
we use an external subprocess to create the symlink tree).
PiperOrigin-RevId: 282564612
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
index d96d408..e5e2124 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
@@ -13,6 +13,8 @@
// limitations under the License.
package com.google.devtools.build.lib.exec;
+import static java.nio.charset.StandardCharsets.ISO_8859_1;
+
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.Lists;
@@ -26,7 +28,11 @@
import com.google.devtools.build.lib.util.io.OutErr;
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 java.io.BufferedReader;
import java.io.IOException;
+import java.io.InputStreamReader;
+import java.util.HashMap;
import java.util.List;
import java.util.Map;
@@ -129,4 +135,32 @@
.setEnv(shellEnvironment)
.build();
}
+
+ static Map<PathFragment, PathFragment> readSymlinksFromFilesetManifest(Path manifest)
+ throws IOException {
+ Map<PathFragment, PathFragment> result = new HashMap<>();
+ try (BufferedReader reader =
+ new BufferedReader(
+ new InputStreamReader(
+ // ISO_8859 is used to write the manifest in {Runfiles,Fileset}ManifestAction.
+ manifest.getInputStream(), ISO_8859_1))) {
+ String line;
+ int lineNumber = 0;
+ while ((line = reader.readLine()) != null) {
+ // If the input has metadata (for fileset), they appear in every other line.
+ if (++lineNumber % 2 == 0) {
+ continue;
+ }
+ int spaceIndex = line.indexOf(' ');
+ result.put(
+ PathFragment.create(line.substring(0, spaceIndex)),
+ PathFragment.create(line.substring(spaceIndex + 1)));
+ }
+ if (lineNumber % 2 != 0) {
+ throw new IOException(
+ "Possibly corrupted manifest file '" + manifest.getPathString() + "'");
+ }
+ }
+ return result;
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java
index 93caaea..bf3f824 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java
@@ -15,6 +15,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Function;
+import com.google.common.base.Preconditions;
import com.google.common.collect.Maps;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionExecutionException;
@@ -43,8 +44,8 @@
private static final Logger logger = Logger.getLogger(SymlinkTreeStrategy.class.getName());
@VisibleForTesting
- static final Function<Artifact, Path> TO_PATH =
- (artifact) -> artifact == null ? null : artifact.getPath();
+ static final Function<Artifact, PathFragment> TO_PATH =
+ (artifact) -> artifact == null ? null : artifact.getPath().asFragment();
private final OutputService outputService;
private final BinTools binTools;
@@ -64,7 +65,8 @@
"running " + action.prettyPrint(), logger, /*minTimeForLoggingInMilliseconds=*/ 100)) {
try {
if (outputService != null && outputService.canCreateSymlinkTree()) {
- Map<PathFragment, Path> symlinks = null;
+ Path inputManifest = actionExecutionContext.getInputPath(action.getInputManifest());
+ Map<PathFragment, PathFragment> symlinks;
if (action.getRunfiles() != null) {
try {
symlinks =
@@ -79,13 +81,30 @@
} catch (IOException e) {
throw new EnvironmentalExecException(e);
}
+ } else {
+ Preconditions.checkState(action.isFilesetTree());
+ try {
+ symlinks = SymlinkTreeHelper.readSymlinksFromFilesetManifest(inputManifest);
+ } catch (IOException e) {
+ throw new EnvironmentalExecException(
+ "Failed to read from manifest '" + inputManifest.getPathString() + "'", e);
+ }
}
+
outputService.createSymlinkTree(
- actionExecutionContext.getInputPath(action.getInputManifest()),
symlinks,
- actionExecutionContext.getInputPath(action.getOutputManifest()),
- action.isFilesetTree(),
action.getOutputManifest().getExecPath().getParentDirectory());
+
+ // Link output manifest on success. We avoid a file copy as these manifests may be large.
+ // Note that this step has to come last because the OutputService may delete any
+ // pre-existing symlink tree before creating a new one.
+ Path outputManifest = actionExecutionContext.getInputPath(action.getOutputManifest());
+ try {
+ outputManifest.createSymbolicLink(inputManifest);
+ } catch (IOException e) {
+ throw new EnvironmentalExecException(
+ "Failed to link output manifest '" + outputManifest.getPathString() + "'", e);
+ }
} else if (!action.enableRunfiles()) {
createSymlinkTreeHelper(action, actionExecutionContext).copyManifest();
} else {
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java
index da925e6..93b4675 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java
@@ -107,11 +107,7 @@
@Override
public void createSymlinkTree(
- Path inputManifest,
- @Nullable Map<PathFragment, Path> symlinks,
- Path outputManifest,
- boolean filesetTree,
- PathFragment symlinkTreeRoot) {
+ Map<PathFragment, PathFragment> symlinks, PathFragment symlinkTreeRoot) {
throw new UnsupportedOperationException();
}
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/OutputService.java b/src/main/java/com/google/devtools/build/lib/vfs/OutputService.java
index 8f604a5..3bb5234 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/OutputService.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/OutputService.java
@@ -119,20 +119,12 @@
/**
* Creates the symlink tree
*
- * @param inputPath the input manifest
* @param symlinks the symlinks to create
- * @param outputPath the output manifest
- * @param filesetTree is true iff we're constructing a Fileset
* @param symlinkTreeRoot the symlink tree root, relative to the execRoot
* @throws ExecException on failure
* @throws InterruptedException
*/
- void createSymlinkTree(
- Path inputPath,
- @Nullable Map<PathFragment, Path> symlinks,
- Path outputPath,
- boolean filesetTree,
- PathFragment symlinkTreeRoot)
+ void createSymlinkTree(Map<PathFragment, PathFragment> symlinks, PathFragment symlinkTreeRoot)
throws ExecException, InterruptedException;
/**
diff --git a/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeHelperTest.java b/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeHelperTest.java
index e7314a9..8e81445 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeHelperTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeHelperTest.java
@@ -15,13 +15,19 @@
package com.google.devtools.build.lib.exec;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.shell.Command;
import com.google.devtools.build.lib.vfs.FileSystem;
+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.inmemoryfs.InMemoryFileSystem;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.util.Map;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -48,4 +54,52 @@
assertThat(commandLine[1]).isEqualTo("input_manifest");
assertThat(commandLine[2]).isEqualTo("output/MANIFEST");
}
+
+ @Test
+ public void readManifest() throws Exception {
+ Path execRoot = fs.getPath("/my/workspace");
+ execRoot.createDirectoryAndParents();
+ Path inputManifestPath = execRoot.getRelative("input_manifest");
+ FileSystemUtils.writeContentAsLatin1(inputManifestPath, "from to\nmetadata");
+ Map<PathFragment, PathFragment> symlinks =
+ SymlinkTreeHelper.readSymlinksFromFilesetManifest(inputManifestPath);
+ assertThat(symlinks).containsExactly(PathFragment.create("from"), PathFragment.create("to"));
+ }
+
+ @Test
+ public void readMultilineManifest() throws Exception {
+ Path execRoot = fs.getPath("/my/workspace");
+ execRoot.createDirectoryAndParents();
+ Path inputManifestPath = execRoot.getRelative("input_manifest");
+ FileSystemUtils.writeContentAsLatin1(
+ inputManifestPath, "from to\nmetadata\n/foo /bar\nmetadata");
+ Map<PathFragment, PathFragment> symlinks =
+ SymlinkTreeHelper.readSymlinksFromFilesetManifest(inputManifestPath);
+ assertThat(symlinks)
+ .containsExactly(
+ PathFragment.create("from"),
+ PathFragment.create("to"),
+ PathFragment.create("/foo"),
+ PathFragment.create("/bar"));
+ }
+
+ @Test
+ public void readCorruptManifest() throws Exception {
+ Path execRoot = fs.getPath("/my/workspace");
+ execRoot.createDirectoryAndParents();
+ Path inputManifestPath = execRoot.getRelative("input_manifest");
+ FileSystemUtils.writeContentAsLatin1(inputManifestPath, "from to");
+ assertThrows(
+ IOException.class,
+ () -> SymlinkTreeHelper.readSymlinksFromFilesetManifest(inputManifestPath));
+ }
+
+ @Test
+ public void readNonExistentManifestFails() {
+ Path execRoot = fs.getPath("/my/workspace");
+ Path inputManifestPath = execRoot.getRelative("input_manifest");
+ assertThrows(
+ FileNotFoundException.class,
+ () -> SymlinkTreeHelper.readSymlinksFromFilesetManifest(inputManifestPath));
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java
index b81ad7a..b355035 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java
@@ -16,7 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
@@ -34,7 +34,6 @@
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.vfs.OutputService;
-import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Map;
import org.junit.Test;
@@ -48,7 +47,8 @@
@Test
public void testArtifactToPathConversion() {
Artifact artifact = getBinArtifactWithNoOwner("dir/foo");
- assertThat(SymlinkTreeStrategy.TO_PATH.apply(artifact)).isEqualTo(artifact.getPath());
+ assertThat(SymlinkTreeStrategy.TO_PATH.apply(artifact))
+ .isEqualTo(artifact.getPath().asFragment());
assertThat(SymlinkTreeStrategy.TO_PATH.apply(null)).isEqualTo(null);
}
@@ -68,6 +68,13 @@
Artifact inputManifest = getBinArtifactWithNoOwner("dir/manifest.in");
Artifact outputManifest = getBinArtifactWithNoOwner("dir/MANIFEST");
Artifact runfile = getBinArtifactWithNoOwner("dir/runfile");
+ doAnswer(
+ (i) -> {
+ outputManifest.getPath().getParentDirectory().createDirectoryAndParents();
+ return null;
+ })
+ .when(outputService)
+ .createSymlinkTree(any(), any());
Runfiles runfiles =
new Runfiles.Builder("TESTING", false)
@@ -87,13 +94,12 @@
action.execute(context);
@SuppressWarnings("unchecked")
- ArgumentCaptor<Map<PathFragment, Path>> capture = ArgumentCaptor.forClass(Map.class);
- verify(outputService, times(1))
- .createSymlinkTree(any(), capture.capture(), any(), anyBoolean(), any());
+ ArgumentCaptor<Map<PathFragment, PathFragment>> capture = ArgumentCaptor.forClass(Map.class);
+ verify(outputService, times(1)).createSymlinkTree(capture.capture(), any());
assertThat(capture.getValue())
.containsExactly(
PathFragment.create("TESTING/dir/runfile"),
- runfile.getPath(),
+ runfile.getPath().asFragment(),
PathFragment.create("TESTING/dir/empty"),
null);
}
