Fix python_library so it caches correctly.
Summary:
Previously, if you had a python_binary rule that depended on a
python_library rule, and both rules were cached in cassandra,
there would be an issue executing the python_binary rule
(presumably as part of a genrule) because the files from the
python_library rule would not be in buck-out/bin/ where the
python_binary rule expected them to be.
This changes things so that the python_library writes its files
to buck-out/gen/, and those files are archived with the python_library
rule such that when it is pulled down from cassandra, it is
unpacked in the way that python_binary needs.
Test Plan: Sandcastle builds.
diff --git a/src/com/facebook/buck/python/PythonBinaryRule.java b/src/com/facebook/buck/python/PythonBinaryRule.java
index 57a19cc..a69972e 100644
--- a/src/com/facebook/buck/python/PythonBinaryRule.java
+++ b/src/com/facebook/buck/python/PythonBinaryRule.java
@@ -20,9 +20,6 @@
import com.facebook.buck.rules.AbstractBuildRuleBuilder;
import com.facebook.buck.rules.AbstractBuildRuleBuilderParams;
-import com.facebook.buck.rules.Buildable;
-import com.facebook.buck.rules.BuildableContext;
-import com.facebook.buck.rules.DoNotUseAbstractBuildable;
import com.facebook.buck.rules.AbstractDependencyVisitor;
import com.facebook.buck.rules.BinaryBuildRule;
import com.facebook.buck.rules.BuildContext;
@@ -30,19 +27,21 @@
import com.facebook.buck.rules.BuildRuleParams;
import com.facebook.buck.rules.BuildRuleResolver;
import com.facebook.buck.rules.BuildRuleType;
+import com.facebook.buck.rules.Buildable;
+import com.facebook.buck.rules.BuildableContext;
import com.facebook.buck.rules.BuildableProperties;
+import com.facebook.buck.rules.DoNotUseAbstractBuildable;
import com.facebook.buck.rules.RuleKey;
import com.facebook.buck.step.Step;
import com.facebook.buck.util.ProjectFilesystem;
-import com.google.common.base.Function;
import com.google.common.base.Joiner;
-import com.google.common.base.Optional;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import java.io.IOException;
+import java.nio.file.Path;
import java.util.List;
public class PythonBinaryRule extends DoNotUseAbstractBuildable implements BinaryBuildRule {
@@ -73,16 +72,17 @@
@Override
public String getExecutableCommand(ProjectFilesystem projectFilesystem) {
- Function<String, String> pathRelativizer = projectFilesystem.getPathRelativizer();
- String pythonPath = Joiner.on(':').join(Iterables.transform(getPythonPathEntries(),
- pathRelativizer));
+ String pythonPath = Joiner.on(':').join(
+ Iterables.transform(
+ getPythonPathEntries(),
+ projectFilesystem.getAbsolutifier()));
return String.format("PYTHONPATH=%s python %s",
pythonPath,
- pathRelativizer.apply(main));
+ projectFilesystem.getPathRelativizer().apply(main));
}
- private ImmutableSet<String> getPythonPathEntries() {
- final ImmutableSet.Builder<String> entries = ImmutableSet.builder();
+ private ImmutableSet<Path> getPythonPathEntries() {
+ final ImmutableSet.Builder<Path> entries = ImmutableSet.builder();
final PythonBinaryRule pythonBinaryRule = this;
new AbstractDependencyVisitor(this) {
@@ -93,10 +93,8 @@
if (buildable instanceof PythonLibrary) {
PythonLibrary pythonLibrary = (PythonLibrary) buildable;
- Optional<String> pythonPathEntry = pythonLibrary.getPythonPathDirectory();
- if (pythonPathEntry.isPresent()) {
- entries.add(pythonPathEntry.get());
- }
+ Path pythonPathEntry = pythonLibrary.getPythonPathDirectory();
+ entries.add(pythonPathEntry);
return true;
}
diff --git a/src/com/facebook/buck/python/PythonLibrary.java b/src/com/facebook/buck/python/PythonLibrary.java
index 889d109..ae0ee09 100644
--- a/src/com/facebook/buck/python/PythonLibrary.java
+++ b/src/com/facebook/buck/python/PythonLibrary.java
@@ -21,24 +21,26 @@
import com.facebook.buck.model.BuildTarget;
import com.facebook.buck.rules.AbstractBuildRuleBuilderParams;
import com.facebook.buck.rules.AbstractBuildable;
-import com.facebook.buck.rules.BuildableContext;
import com.facebook.buck.rules.BuildContext;
import com.facebook.buck.rules.BuildRuleParams;
import com.facebook.buck.rules.BuildRuleResolver;
import com.facebook.buck.rules.BuildRuleType;
+import com.facebook.buck.rules.BuildableContext;
import com.facebook.buck.rules.BuildableProperties;
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.SymlinkFileStep;
import com.facebook.buck.util.BuckConstant;
-import com.google.common.base.Optional;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSortedSet;
import java.io.File;
import java.io.IOException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
import java.util.List;
import javax.annotation.Nullable;
@@ -48,18 +50,18 @@
private final static BuildableProperties OUTPUT_TYPE = new BuildableProperties(LIBRARY);
private final BuildTarget buildTarget;
private final ImmutableSortedSet<String> srcs;
- private final Optional<String> pythonPathDirectory;
+ private final Path pythonPathDirectory;
protected PythonLibrary(BuildRuleParams buildRuleParams,
ImmutableSortedSet<String> srcs) {
+ Preconditions.checkArgument(!srcs.isEmpty(),
+ "Must specify srcs for %s.",
+ buildRuleParams.getBuildTarget());
+
this.buildTarget = buildRuleParams.getBuildTarget();
this.srcs = ImmutableSortedSet.copyOf(srcs);
- if (srcs.isEmpty()) {
- this.pythonPathDirectory = Optional.absent();
- } else {
- this.pythonPathDirectory = Optional.of(getPathToPythonPathDirectory());
- }
+ this.pythonPathDirectory = getPathToPythonPathDirectory();
}
@Nullable
@@ -70,16 +72,18 @@
@Override
public RuleKey.Builder appendDetailsToRuleKey(RuleKey.Builder builder) throws IOException {
- return builder
- .set("srcs", srcs)
- .set("pythonPathDirectory", pythonPathDirectory);
+ return builder.set("srcs", srcs);
}
- private String getPathToPythonPathDirectory() {
- return String.format("%s/%s/__pylib_%s/",
- BuckConstant.BIN_DIR,
+ private Path getPathToPythonPathDirectory() {
+ return Paths.get(
+ BuckConstant.GEN_DIR,
buildTarget.getBasePath(),
- buildTarget.getShortName());
+ getPathUnderGenDirectory());
+ }
+
+ private String getPathUnderGenDirectory() {
+ return "__pylib_" + buildTarget.getShortName();
}
public ImmutableSortedSet<String> getPythonSrcs() {
@@ -91,25 +95,28 @@
return srcs;
}
- public Optional<String> getPythonPathDirectory() {
+ public Path getPythonPathDirectory() {
return pythonPathDirectory;
}
@Override
- public List<Step> getBuildSteps(BuildContext context, BuildableContext buildableContext) throws IOException {
+ public List<Step> getBuildSteps(BuildContext context, BuildableContext buildableContext)
+ throws IOException {
ImmutableList.Builder<Step> commands = ImmutableList.builder();
- // Symlink all of the sources to a generated directory so that the generated directory can be
- // included as a $PYTHONPATH element.
+ // Copy all of the sources to a generated directory so that the generated directory can be
+ // included as a $PYTHONPATH element. Symlinks would be more efficient, but we need to include
+ // this structure in the artifact, which is not guaranteed to be zip-friendly.
// TODO(mbolin): Do not flatten the directory structure when creating the symlinks, as directory
// structure is significant in Python when __init__.py is used.
- if (!srcs.isEmpty()) {
- commands.add(new MakeCleanDirectoryStep(getPathToPythonPathDirectory()));
- File pythonPathDirectory = new File(getPathToPythonPathDirectory());
- for (String src : srcs) {
- File target = new File(pythonPathDirectory, new File(src).getName());
- commands.add(new SymlinkFileStep(src, target.getPath()));
- }
+ commands.add(new MakeCleanDirectoryStep(pythonPathDirectory));
+ for (String src : srcs) {
+ String srcName = new File(src).getName();
+ Path target = pythonPathDirectory.resolve(srcName);
+ commands.add(new CopyStep(src, target.toString()));
+
+ Path pathToArtifact = Paths.get(getPathUnderGenDirectory(), srcName);
+ buildableContext.recordArtifact(pathToArtifact.toString());
}
return commands.build();
diff --git a/src/com/facebook/buck/rules/RuleKey.java b/src/com/facebook/buck/rules/RuleKey.java
index c3b1aaf..e646900 100644
--- a/src/com/facebook/buck/rules/RuleKey.java
+++ b/src/com/facebook/buck/rules/RuleKey.java
@@ -36,6 +36,7 @@
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
+import java.nio.file.Path;
import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;
@@ -278,6 +279,10 @@
return setKey(key).setVal(val);
}
+ public Builder set(String key, @Nullable Path path) throws IOException {
+ return set(key, path == null ? null : path.toFile());
+ }
+
public Builder set(String key, @Nullable String val) {
return setKey(key).setVal(val);
}
diff --git a/src/com/facebook/buck/step/fs/RmStep.java b/src/com/facebook/buck/step/fs/RmStep.java
index 4194c09..c41d34f 100644
--- a/src/com/facebook/buck/step/fs/RmStep.java
+++ b/src/com/facebook/buck/step/fs/RmStep.java
@@ -91,7 +91,7 @@
}
} else {
// Delete a single file
- File file = projectFilesystem.resolve(patternToDelete).toAbsolutePath().toFile();
+ File file = projectFilesystem.resolve(patternToDelete).toFile();
if (!file.delete() && !shouldForceDeletion) {
return 1;
}
diff --git a/src/com/facebook/buck/util/ProjectFilesystem.java b/src/com/facebook/buck/util/ProjectFilesystem.java
index a5d5e8d..61f7ae1 100644
--- a/src/com/facebook/buck/util/ProjectFilesystem.java
+++ b/src/com/facebook/buck/util/ProjectFilesystem.java
@@ -51,7 +51,13 @@
private final File projectRoot;
private final Path pathToRoot;
+
+ // Over time, we hope to replace our uses of String with Path, as appropriate. When that happens,
+ // the Function<String, String> can go away, as Function<Path, Path> should be used exclusively.
+
+ private final Function<Path, Path> pathAbsolutifier;
private final Function<String, String> pathRelativizer;
+
private final ImmutableSet<String> ignorePaths;
/**
@@ -66,10 +72,16 @@
this.projectRoot = Preconditions.checkNotNull(projectRoot);
this.pathToRoot = projectRoot.toPath();
Preconditions.checkArgument(projectRoot.isDirectory());
+ this.pathAbsolutifier = new Function<Path, Path>() {
+ @Override
+ public Path apply(Path path) {
+ return resolve(path);
+ }
+ };
this.pathRelativizer = new Function<String, String>() {
@Override
public String apply(String relativePath) {
- return ProjectFilesystem.this.getFileForRelativePath(relativePath).getAbsolutePath();
+ return getFileForRelativePath(relativePath).getAbsolutePath();
}
};
this.ignorePaths = Preconditions.checkNotNull(ignorePaths);
@@ -83,9 +95,18 @@
return pathToRoot;
}
- /** @return the specified {@code path} resolved against {@link #getRootPath()}. */
+ /**
+ * @return the specified {@code path} resolved against {@link #getRootPath()} to an absolute path.
+ */
public Path resolve(Path path) {
- return pathToRoot.resolve(path);
+ return pathToRoot.resolve(path).toAbsolutePath();
+ }
+
+ /**
+ * @return A {@link Function} that applies {@link #resolve(Path)} to its parameter.
+ */
+ public Function<Path, Path> getAbsolutifier() {
+ return pathAbsolutifier;
}
public File getProjectRoot() {
diff --git a/test/com/facebook/buck/android/ApkGenruleTest.java b/test/com/facebook/buck/android/ApkGenruleTest.java
index b450ea2..8362f74 100644
--- a/test/com/facebook/buck/android/ApkGenruleTest.java
+++ b/test/com/facebook/buck/android/ApkGenruleTest.java
@@ -61,6 +61,7 @@
import java.io.File;
import java.io.IOException;
+import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.List;
import java.util.Map;
@@ -169,7 +170,7 @@
ImmutableList.of(
"rm",
"-f",
- "./" + apkGenrule.getPathToOutputFile()),
+ apkGenrule.getPathToOutputFile()),
rmCommand.getShellCommand(executionContext));
Step secondStep = steps.get(1);
@@ -177,7 +178,7 @@
MkdirStep mkdirCommand = (MkdirStep) secondStep;
assertEquals(
"Second command should make sure the output directory exists.",
- Paths.get("./" + GEN_DIR + "/src/com/facebook/"),
+ Paths.get(GEN_DIR + "/src/com/facebook/"),
mkdirCommand.getPath(executionContext));
Step thirdStep = steps.get(2);
@@ -235,6 +236,11 @@
public Function<String, String> getPathRelativizer() {
return Functions.identity();
}
+
+ @Override
+ public Path resolve(Path path) {
+ return path;
+ }
})
.setEventBus(BuckEventBusFactory.newInstance())
.setPlatform(Platform.LINUX) // Fix platform to Linux to use bash in genrule.
diff --git a/test/com/facebook/buck/android/GenAidlTest.java b/test/com/facebook/buck/android/GenAidlTest.java
index 6351d60..978e3c5 100644
--- a/test/com/facebook/buck/android/GenAidlTest.java
+++ b/test/com/facebook/buck/android/GenAidlTest.java
@@ -88,7 +88,12 @@
ExecutionContext executionContext = createMock(ExecutionContext.class);
expect(executionContext.getAndroidPlatformTarget()).andReturn(androidPlatformTarget);
expect(executionContext.getProjectFilesystem()).andReturn(
- new ProjectFilesystem(new File(".")))
+ new ProjectFilesystem(new File(".")) {
+ @Override
+ public Path resolve(Path path) {
+ return path;
+ }
+ })
.times(2);
replay(androidPlatformTarget,
aidlExecutable,
@@ -96,7 +101,6 @@
executionContext);
Path outputDirectory = Paths.get(
- ".",
BuckConstant.BIN_DIR,
"/java/com/example/base/.IWhateverService.aidl");
MkdirStep mkdirStep = (MkdirStep) steps.get(0);
diff --git a/test/com/facebook/buck/shell/GenruleTest.java b/test/com/facebook/buck/shell/GenruleTest.java
index c5c6b6e..1dfab56 100644
--- a/test/com/facebook/buck/shell/GenruleTest.java
+++ b/test/com/facebook/buck/shell/GenruleTest.java
@@ -70,6 +70,7 @@
import java.io.File;
import java.io.IOException;
+import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.List;
import java.util.Map;
@@ -175,7 +176,7 @@
ImmutableList.of(
"rm",
"-f",
- "./" + GEN_DIR + "/src/com/facebook/katana/AndroidManifest.xml"),
+ GEN_DIR + "/src/com/facebook/katana/AndroidManifest.xml"),
rmCommand.getShellCommand(executionContext));
Step secondStep = steps.get(1);
@@ -183,7 +184,7 @@
MkdirStep mkdirCommand = (MkdirStep) secondStep;
assertEquals(
"Second command should make sure the output directory exists.",
- Paths.get("./" + GEN_DIR + "/src/com/facebook/katana"),
+ Paths.get(GEN_DIR + "/src/com/facebook/katana"),
mkdirCommand.getPath(executionContext));
Step mkTmpDir = steps.get(2);
@@ -245,6 +246,11 @@
public Function<String, String> getPathRelativizer() {
return Functions.identity();
}
+
+ @Override
+ public Path resolve(Path path) {
+ return path;
+ }
})
.setEventBus(BuckEventBusFactory.newInstance())
.setPlatform(platform)
