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");