Use symlinks when creating PythonLibrary environment.
Summary: Use symbolic links instead of copying. The links needed to be
relative in order to allow the keep cached data correct across repositories.
diff --git a/src/com/facebook/buck/python/PythonLibrary.java b/src/com/facebook/buck/python/PythonLibrary.java
index b3d08b1..6cc536b 100644
--- a/src/com/facebook/buck/python/PythonLibrary.java
+++ b/src/com/facebook/buck/python/PythonLibrary.java
@@ -30,9 +30,9 @@
import com.facebook.buck.rules.RuleKey;
import com.facebook.buck.rules.SrcsAttributeBuilder;
import com.facebook.buck.step.Step;
-import com.facebook.buck.step.fs.CopyStep;
import com.facebook.buck.step.fs.MakeCleanDirectoryStep;
import com.facebook.buck.step.fs.MkdirStep;
+import com.facebook.buck.step.fs.SymlinkFileStep;
import com.facebook.buck.util.BuckConstant;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
@@ -111,17 +111,19 @@
commands.add(new MakeCleanDirectoryStep(pythonPathDirectory));
ImmutableSortedSet.Builder<Path> directories = ImmutableSortedSet.naturalOrder();
- ImmutableList.Builder<Step> copySteps = ImmutableList.builder();
+ ImmutableList.Builder<Step> symlinkSteps = ImmutableList.builder();
for (String src : srcs) {
Path srcPath = Paths.get(src);
- Path relativeSrc = Paths.get(buildTarget.getBasePath()).relativize(srcPath);
- Path target = pythonPathDirectory.resolve(relativeSrc);
+ Path targetPath = pythonPathDirectory.resolve(srcPath);
- directories.add(target.getParent());
- copySteps.add(new CopyStep(srcPath, target));
+ directories.add(targetPath.getParent());
+ symlinkSteps.add(
+ new SymlinkFileStep(srcPath.toString(),
+ targetPath.toString(),
+ /* useAbsolutePaths */ false));
- Path pathToArtifact = Paths.get(getPathUnderGenDirectory()).resolve(relativeSrc);
+ Path pathToArtifact = Paths.get(getPathUnderGenDirectory()).resolve(srcPath);
buildableContext.recordArtifact(pathToArtifact);
}
@@ -129,7 +131,7 @@
commands.add(new MkdirStep(path));
}
- commands.addAll(copySteps.build());
+ commands.addAll(symlinkSteps.build());
return commands.build();
}
diff --git a/src/com/facebook/buck/step/fs/MkdirAndSymlinkFileStep.java b/src/com/facebook/buck/step/fs/MkdirAndSymlinkFileStep.java
index 1157f61..2e7e92c 100644
--- a/src/com/facebook/buck/step/fs/MkdirAndSymlinkFileStep.java
+++ b/src/com/facebook/buck/step/fs/MkdirAndSymlinkFileStep.java
@@ -35,7 +35,7 @@
public MkdirAndSymlinkFileStep(String source, String target) {
super(ImmutableList.of(
new MkdirStep(Paths.get(target).getParent().toString()),
- new SymlinkFileStep(source, target)
+ new SymlinkFileStep(source, target, /* useAbsolutePaths */ true)
));
this.source = Preconditions.checkNotNull(source);
this.target = Preconditions.checkNotNull(target);
diff --git a/src/com/facebook/buck/step/fs/SymlinkFileStep.java b/src/com/facebook/buck/step/fs/SymlinkFileStep.java
index 4efae07..46b14c5 100644
--- a/src/com/facebook/buck/step/fs/SymlinkFileStep.java
+++ b/src/com/facebook/buck/step/fs/SymlinkFileStep.java
@@ -18,21 +18,45 @@
import com.facebook.buck.step.ExecutionContext;
import com.facebook.buck.step.Step;
-import com.google.common.base.Function;
+import com.facebook.buck.util.MorePaths;
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import java.io.IOException;
import java.nio.file.Path;
+import java.nio.file.Paths;
public class SymlinkFileStep implements Step {
- private final String source;
- private final String target;
+ private final String existingFile;
+ private final String desiredLink;
+ private final boolean useAbsolutePaths;
- public SymlinkFileStep(String source, String target) {
- this.source = Preconditions.checkNotNull(source);
- this.target = Preconditions.checkNotNull(target);
+ public SymlinkFileStep(String existingFile, String desiredLink, boolean useAbsolutePaths) {
+ this.existingFile = Preconditions.checkNotNull(existingFile);
+ this.desiredLink = Preconditions.checkNotNull(desiredLink);
+ this.useAbsolutePaths = useAbsolutePaths;
+ }
+
+ /**
+ * Get the path to the existing file that should be linked.
+ */
+ private Path getExistingFilePath(ExecutionContext context) {
+ // This could be either an absolute or relative path.
+ // TODO ideally all symbolic links should be relative, consider eliminating the absolute option.
+ return (useAbsolutePaths ? getAbsolutePath(existingFile, context) :
+ MorePaths.getRelativePath(Paths.get(existingFile), Paths.get(desiredLink).getParent()));
+ }
+
+ /**
+ * Get the path to the desired link that should be created.
+ */
+ private Path getDesiredLinkPath(ExecutionContext context) {
+ return getAbsolutePath(desiredLink, context);
+ }
+
+ private static Path getAbsolutePath(String path, ExecutionContext context) {
+ return context.getProjectFilesystem().getPathRelativizer().apply(path);
}
@Override
@@ -42,23 +66,23 @@
@Override
public String getDescription (ExecutionContext context) {
- Function<String, Path> pathRelativizer = context.getProjectFilesystem().getPathRelativizer();
- // Always symlink to an absolute path so the symlink is sure to be read correctly.
return Joiner.on(" ").join(
"ln",
"-f",
"-s",
- pathRelativizer.apply(source),
- pathRelativizer.apply(target));
+ getExistingFilePath(context),
+ getDesiredLinkPath(context));
}
@Override
public int execute(ExecutionContext context) {
- Function<String, Path> pathRelativizer = context.getProjectFilesystem().getPathRelativizer();
- Path targetPath = pathRelativizer.apply(target);
- Path sourcePath = pathRelativizer.apply(source);
+ Path existingFilePath = getExistingFilePath(context);
+ Path desiredLinkPath = getDesiredLinkPath(context);
try {
- context.getProjectFilesystem().createSymLink(sourcePath, targetPath, true);
+ context.getProjectFilesystem().createSymLink(
+ existingFilePath,
+ desiredLinkPath,
+ /* force */ true);
return 0;
} catch (IOException e) {
e.printStackTrace(context.getStdErr());
diff --git a/src/com/facebook/buck/util/MorePaths.java b/src/com/facebook/buck/util/MorePaths.java
index fcaca0a..a147fdf 100644
--- a/src/com/facebook/buck/util/MorePaths.java
+++ b/src/com/facebook/buck/util/MorePaths.java
@@ -17,7 +17,6 @@
package com.facebook.buck.util;
import com.google.common.base.Preconditions;
-import com.google.common.base.Strings;
import com.google.common.collect.ImmutableSortedSet;
import java.io.File;
@@ -26,6 +25,8 @@
import java.nio.file.Path;
import java.nio.file.Paths;
+import javax.annotation.Nullable;
+
public class MorePaths {
/** Utility class: do not instantiate. */
@@ -65,6 +66,27 @@
}
/**
+ * Get the path of a file relative to a base directory.
+ *
+ * @param path must reference a file, not a directory.
+ * @param baseDir must reference a directory that is relative to a common directory with the path.
+ * may be null if referencing the same directory as the path.
+ * @return the relative path of path from the directory baseDir.
+ */
+ public static Path getRelativePath(Path path, @Nullable Path baseDir) {
+ if (baseDir == null) {
+ // This allows callers to use this method with "file.parent()" for files from the project
+ // root dir.
+ baseDir = Paths.get("");
+ }
+ Preconditions.checkArgument(!path.isAbsolute(),
+ "Path must be relative: %s.", path);
+ Preconditions.checkArgument(!baseDir.isAbsolute(),
+ "Path must be relative: %s.", baseDir);
+ return baseDir.relativize(path);
+ }
+
+ /**
* Creates a symlink at
* {@code projectFilesystem.getRootPath().resolve(pathToDesiredLinkUnderProjectRoot)} that
* points to {@code projectFilesystem.getRootPath().resolve(pathToExistingFileUnderProjectRoot)}
@@ -86,7 +108,7 @@
/**
* Creates a symlink at {@code pathToProjectRoot.resolve(pathToDesiredLinkUnderProjectRoot)} that
* points to {@code pathToProjectRoot.resolve(pathToExistingFileUnderProjectRoot)} using a
- * relative symlink.
+ * relative symlink. Both params must be relative to the project root.
*
* @param pathToDesiredLinkUnderProjectRoot must reference a file, not a directory.
* @param pathToExistingFileUnderProjectRoot must reference a file, not a directory.
@@ -96,16 +118,9 @@
Path pathToDesiredLinkUnderProjectRoot,
Path pathToExistingFileUnderProjectRoot,
Path pathToProjectRoot) throws IOException {
- Preconditions.checkArgument(!pathToDesiredLinkUnderProjectRoot.isAbsolute(),
- "Path must be relative to project root: %s.",
- pathToDesiredLinkUnderProjectRoot);
- Preconditions.checkArgument(!pathToExistingFileUnderProjectRoot.isAbsolute(),
- "Path must be relative to project root: %s.",
- pathToExistingFileUnderProjectRoot);
-
- Path parent = pathToDesiredLinkUnderProjectRoot.getParent();
- String base = Strings.repeat("../", parent == null ? 0 : parent.getNameCount());
- Path target = Paths.get(base, pathToExistingFileUnderProjectRoot.toString());
+ Path target = getRelativePath(
+ pathToExistingFileUnderProjectRoot,
+ pathToDesiredLinkUnderProjectRoot.getParent());
Files.createSymbolicLink(pathToProjectRoot.resolve(pathToDesiredLinkUnderProjectRoot), target);
return target;
}
diff --git a/test/com/facebook/buck/python/PythonLibraryTest.java b/test/com/facebook/buck/python/PythonLibraryTest.java
index 1bffd84..dd3802c 100644
--- a/test/com/facebook/buck/python/PythonLibraryTest.java
+++ b/test/com/facebook/buck/python/PythonLibraryTest.java
@@ -40,7 +40,9 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedSet;
+import org.junit.Rule;
import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
import java.io.IOException;
import java.io.File;
@@ -52,6 +54,8 @@
* Unit test for {@link PythonLibrary}.
*/
public class PythonLibraryTest {
+ @Rule
+ public final TemporaryFolder projectRootDir = new TemporaryFolder();
@Test
public void testGetters() {
@@ -82,7 +86,7 @@
PythonLibrary rule = (PythonLibrary)ruleResolver.get(pyLibraryTarget).getBuildable();
List<Step> steps = rule.getBuildSteps(buildContext, buildableContext);
- final String projectRoot = "/";
+ final String projectRoot = projectRootDir.getRoot().getAbsolutePath();
final String pylibpath = "__pylib_py_library";
ProjectFilesystem projectFilesystem = new ProjectFilesystem(new File(projectRoot));
@@ -91,32 +95,35 @@
.build();
MoreAsserts.assertSteps(
- "python_library() should ensure each file is copied and has its destination directory made",
+ "python_library() should ensure each file is linked and has its destination directory made",
ImmutableList.of(
String.format(
- "mkdir -p %s%s/%s",
+ "mkdir -p %s/%s/%s",
projectRoot,
BuckConstant.GEN_DIR,
pylibpath
),
String.format(
- "mkdir -p %s%s/%s/foo",
+ "mkdir -p %s/%s/%s/foo",
projectRoot,
BuckConstant.GEN_DIR,
pylibpath
),
String.format(
- "cp baz.py %s/%s/baz.py",
+ "ln -f -s ../../../baz.py %s/%s/%s/baz.py",
+ projectRoot,
BuckConstant.GEN_DIR,
pylibpath
),
String.format(
- "cp foo/__init__.py %s/%s/foo/__init__.py",
+ "ln -f -s ../../../../foo/__init__.py %s/%s/%s/foo/__init__.py",
+ projectRoot,
BuckConstant.GEN_DIR,
pylibpath
),
String.format(
- "cp foo/bar.py %s/%s/foo/bar.py",
+ "ln -f -s ../../../../foo/bar.py %s/%s/%s/foo/bar.py",
+ projectRoot,
BuckConstant.GEN_DIR,
pylibpath
)
diff --git a/test/com/facebook/buck/step/fs/SymlinkFileStepTest.java b/test/com/facebook/buck/step/fs/SymlinkFileStepTest.java
index ba56760..e567742 100644
--- a/test/com/facebook/buck/step/fs/SymlinkFileStepTest.java
+++ b/test/com/facebook/buck/step/fs/SymlinkFileStepTest.java
@@ -42,7 +42,16 @@
public final TemporaryFolder tmpDir = new TemporaryFolder();
@Test
- public void testSymlinkFiles() throws IOException {
+ public void testAbsoluteSymlinkFiles() throws IOException {
+ internalTestSymlinkFiles(/* useAbsolutePaths */ true);
+ }
+
+ @Test
+ public void testRelativeSymlinkFiles() throws IOException {
+ internalTestSymlinkFiles(/* useAbsolutePaths */ false);
+ }
+
+ public void internalTestSymlinkFiles(boolean useAbsolutePaths) throws IOException {
ExecutionContext context = TestExecutionContext.newBuilder()
.setProjectFilesystem(new ProjectFilesystem(tmpDir.getRoot()))
.build();
@@ -53,8 +62,10 @@
File target = tmpDir.newFile();
target.delete();
- SymlinkFileStep step = new SymlinkFileStep(source.getName(),
- target.getName());
+ SymlinkFileStep step = new SymlinkFileStep(
+ /* source */ source.getName(),
+ /* target */ target.getName(),
+ useAbsolutePaths);
step.execute(context);
// Run twice to ensure we can overwrite an existing symlink
step.execute(context);
@@ -95,7 +106,8 @@
tmpDir.newFile("dummy");
SymlinkFileStep symlinkStep = new SymlinkFileStep(
/* source */ "dummy",
- /* target */ "my_symlink");
+ /* target */ "my_symlink",
+ /* useAbsolutePaths*/ true);
int exitCode = symlinkStep.execute(executionContext);
assertEquals(0, exitCode);
assertTrue(java.nio.file.Files.isSymbolicLink(symlink));
diff --git a/test/com/facebook/buck/util/MorePathsTest.java b/test/com/facebook/buck/util/MorePathsTest.java
index 03513a7..452ecab 100644
--- a/test/com/facebook/buck/util/MorePathsTest.java
+++ b/test/com/facebook/buck/util/MorePathsTest.java
@@ -35,6 +35,29 @@
public DebuggableTemporaryFolder tmp = new DebuggableTemporaryFolder();
@Test
+ public void testGetRelativePath() {
+ // Path on base directory.
+ assertEquals(
+ Paths.get("file"),
+ MorePaths.getRelativePath(Paths.get("file"), Paths.get("")));
+
+ // Path on base directory (using null).
+ assertEquals(
+ Paths.get("file"),
+ MorePaths.getRelativePath(Paths.get("file"), null));
+
+ // Path internal to base directory.
+ assertEquals(
+ Paths.get("dir/file"),
+ MorePaths.getRelativePath(Paths.get("base/dir/file"), Paths.get("base")));
+
+ // Path external to base directory.
+ assertEquals(
+ Paths.get("../dir1/file"),
+ MorePaths.getRelativePath(Paths.get("dir1/file"), Paths.get("base")));
+ }
+
+ @Test
public void testCreateRelativeSymlinkToFilesInRoot() throws IOException {
ProjectFilesystem projectFilesystem = new ProjectFilesystem(tmp.getRoot());
tmp.newFile("biz.txt");
