Skip to content

Commit

Permalink
Refactor moveOutputs out of the SandboxedSpawn interface.
Browse files Browse the repository at this point in the history
This moves the moveOutputs method from the SandboxedSpawn interface to the
AbstractContainerizingSandboxedSpawn class.

The rationale is to make the former a pure interface, and to allow adding
more logic to moveOutputs that will require adding supporting static fields
that are not nice in an interface.

RELNOTES: None.
PiperOrigin-RevId: 230780925
  • Loading branch information
jmmv authored and weixiao-huang committed Jan 31, 2019
1 parent 105fa04 commit 36c9543
Show file tree
Hide file tree
Showing 6 changed files with 42 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,42 @@ protected void createInputs(Map<PathFragment, Path> inputs) throws IOException {

protected abstract void copyFile(Path source, Path target) throws IOException;

/**
* Moves all given outputs from a root to another.
*
* <p>This is a support function to help with the implementation of {@link #copyOutputs(Path)}.
*
* @param outputs outputs to move as relative paths to a root
* @param sourceRoot source directory from which to resolve outputs
* @param targetRoot target directory to which to move the resolved outputs from the source
* @throws IOException if any of the moves fails
*/
static void moveOutputs(SandboxOutputs outputs, Path sourceRoot, Path targetRoot)
throws IOException {
for (PathFragment output : Iterables.concat(outputs.files(), outputs.dirs())) {
Path source = sourceRoot.getRelative(output);
Path target = targetRoot.getRelative(output);
if (source.isFile() || source.isSymbolicLink()) {
// Ensure the target directory exists in the target. The directories for the action outputs
// have already been created, but the spawn outputs may be different from the overall action
// outputs. This is the case for test actions.
target.getParentDirectory().createDirectoryAndParents();
FileSystemUtils.moveFile(source, target);
} else if (source.isDirectory()) {
try {
source.renameTo(target);
} catch (IOException e) {
// Failed to move directory directly, thus move it recursively.
target.createDirectory();
FileSystemUtils.moveTreesBelow(source, target);
}
}
}
}

@Override
public void copyOutputs(Path execRoot) throws IOException {
SandboxedSpawn.moveOutputs(outputs, sandboxExecRoot, execRoot);
moveOutputs(outputs, sandboxExecRoot, execRoot);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,7 @@

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

import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs;
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.IOException;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -60,37 +56,4 @@ interface SandboxedSpawn {
* Deletes the sandbox directory.
*/
void delete();

/**
* Moves all given outputs from a root to another.
*
* <p>This is a support function to help with the implementation of {@link #copyOutputs(Path)}.
*
* @param outputs outputs to move as relative paths to a root
* @param sourceRoot source directory from which to resolve outputs
* @param targetRoot target directory to which to move the resolved outputs from the source
* @throws IOException if any of the moves fails
*/
static void moveOutputs(SandboxOutputs outputs, Path sourceRoot, Path targetRoot)
throws IOException {
for (PathFragment output : Iterables.concat(outputs.files(), outputs.dirs())) {
Path source = sourceRoot.getRelative(output);
Path target = targetRoot.getRelative(output);
if (source.isFile() || source.isSymbolicLink()) {
// Ensure the target directory exists in the target. The directories for the action outputs
// have already been created, but the spawn outputs may be different from the overall action
// outputs. This is the case for test actions.
target.getParentDirectory().createDirectoryAndParents();
FileSystemUtils.moveFile(source, target);
} else if (source.isDirectory()) {
try {
source.renameTo(target);
} catch (IOException e) {
// Failed to move directory directly, thus move it recursively.
target.createDirectory();
FileSystemUtils.moveTreesBelow(source, target);
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ public void copyOutputs(Path targetExecRoot) throws IOException {
// TODO(jmmv): If we knew the targetExecRoot when setting up the spawn, we may be able to
// configure sandboxfs so that the output files are written directly to their target locations.
// This would avoid having to move them after-the-fact.
SandboxedSpawn.moveOutputs(outputs, sandboxScratchDir, targetExecRoot);
AbstractContainerizingSandboxedSpawn.moveOutputs(outputs, sandboxScratchDir, targetExecRoot);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@

/** Tests for {@link SandboxedSpawn}. */
@RunWith(JUnit4.class)
public class SandboxedSpawnTest {
public class AbstractContainerizingSandboxedSpawnTest {

@Test
public void testMoveOutputs() throws Exception {
Expand Down Expand Up @@ -74,7 +74,8 @@ public void testMoveOutputs() throws Exception {
Path outputsDir = testRoot.getRelative("outputs");
outputsDir.createDirectory();
outputsDir.getRelative("very").createDirectory();
SandboxedSpawn.moveOutputs(SandboxOutputs.create(outputs, outputDirs), execRoot, outputsDir);
AbstractContainerizingSandboxedSpawn.moveOutputs(
SandboxOutputs.create(outputs, outputDirs), execRoot, outputsDir);

assertThat(outputsDir.getRelative("very/output.txt").isFile(Symlinks.NOFOLLOW)).isTrue();
assertThat(outputsDir.getRelative("very/output.link").isSymbolicLink()).isTrue();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@ public void testDelete() throws Exception {

@Test
public void testCopyOutputs() throws Exception {
// These tests are very simple because we just rely on SandboxedSpawnTest.testMoveOutputs to
// properly verify all corner cases.
// These tests are very simple because we just rely on
// AbstractContainerizingSandboxedSpawnTest.testMoveOutputs to properly verify all corner cases.
PathFragment outputFile = PathFragment.create("very/output.txt");

SandboxedSpawn spawn =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ public void createFileSystem() throws Exception {

@Test
public void copyOutputs() throws Exception {
// These tests are very simple because we just rely on SandboxedSpawnTest.testMoveOutputs to
// properly verify all corner cases.
// These tests are very simple because we just rely on
// AbstractContainerizingSandboxedSpawnTest.testMoveOutputs to properly verify all corner cases.
Path outputFile = execRoot.getRelative("very/output.txt");

SymlinkedSandboxedSpawn symlinkedExecRoot =
Expand Down

0 comments on commit 36c9543

Please sign in to comment.