python_library should not flatten libraries.
Summary:
Multi-file python libraries are often contained in a directory and
imported by that directory's name. Flattening the hierarchy will render that
module unimportable (or worse, possibly import only part of it). By copying the
unflattened subtree, we preserve the semantics of Python's module system while
retaining the isolation property.
diff --git a/src/com/facebook/buck/python/PythonLibrary.java b/src/com/facebook/buck/python/PythonLibrary.java
index 8ba084c..b3d08b1 100644
--- a/src/com/facebook/buck/python/PythonLibrary.java
+++ b/src/com/facebook/buck/python/PythonLibrary.java
@@ -32,12 +32,12 @@
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.util.BuckConstant;
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;
@@ -106,20 +106,31 @@
ImmutableList.Builder<Step> commands = ImmutableList.builder();
// 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.
+ // included as a $PYTHONPATH element. TODO(mbolin): Symlinks would be more efficient, but we
+ // need to include this structure in the artifact, which is not guaranteed to be zip-friendly.
commands.add(new MakeCleanDirectoryStep(pythonPathDirectory));
- for (String src : srcs) {
- String srcName = new File(src).getName();
- Path target = pythonPathDirectory.resolve(srcName);
- commands.add(new CopyStep(Paths.get(src), target));
- Path pathToArtifact = Paths.get(getPathUnderGenDirectory(), srcName);
+ ImmutableSortedSet.Builder<Path> directories = ImmutableSortedSet.naturalOrder();
+ ImmutableList.Builder<Step> copySteps = ImmutableList.builder();
+
+ for (String src : srcs) {
+ Path srcPath = Paths.get(src);
+ Path relativeSrc = Paths.get(buildTarget.getBasePath()).relativize(srcPath);
+ Path target = pythonPathDirectory.resolve(relativeSrc);
+
+ directories.add(target.getParent());
+ copySteps.add(new CopyStep(srcPath, target));
+
+ Path pathToArtifact = Paths.get(getPathUnderGenDirectory()).resolve(relativeSrc);
buildableContext.recordArtifact(pathToArtifact);
}
+ for (Path path : directories.build()) {
+ commands.add(new MkdirStep(path));
+ }
+
+ commands.addAll(copySteps.build());
+
return commands.build();
}
diff --git a/test/com/facebook/buck/python/BUCK b/test/com/facebook/buck/python/BUCK
index 9b22033..f8cc751 100644
--- a/test/com/facebook/buck/python/BUCK
+++ b/test/com/facebook/buck/python/BUCK
@@ -5,6 +5,7 @@
'//src/com/facebook/buck/python:rules',
],
deps = [
+ '//lib:easymock',
'//lib:guava',
'//lib:junit',
'//src/com/facebook/buck/java:rules',
@@ -12,8 +13,12 @@
'//src/com/facebook/buck/python:rules',
'//src/com/facebook/buck/rules:build_rule',
'//src/com/facebook/buck/rules:rules',
+ '//src/com/facebook/buck/step:step',
+ '//src/com/facebook/buck/util:constants',
'//src/com/facebook/buck/util:io',
'//test/com/facebook/buck/model:BuildTargetFactory',
'//test/com/facebook/buck/rules:testutil',
+ '//test/com/facebook/buck/step:testutil',
+ '//test/com/facebook/buck/testutil:testutil',
],
)
diff --git a/test/com/facebook/buck/python/PythonLibraryTest.java b/test/com/facebook/buck/python/PythonLibraryTest.java
index 014a58a..1bffd84 100644
--- a/test/com/facebook/buck/python/PythonLibraryTest.java
+++ b/test/com/facebook/buck/python/PythonLibraryTest.java
@@ -17,16 +17,37 @@
package com.facebook.buck.python;
import static com.facebook.buck.rules.BuildableProperties.Kind.LIBRARY;
+import static org.easymock.EasyMock.createMock;
+import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
import com.facebook.buck.model.BuildTarget;
+import com.facebook.buck.model.BuildTargetFactory;
+import com.facebook.buck.rules.BuildContext;
import com.facebook.buck.rules.BuildRuleParams;
+import com.facebook.buck.rules.BuildRuleResolver;
+import com.facebook.buck.rules.FakeAbstractBuildRuleBuilderParams;
+import com.facebook.buck.rules.FakeBuildableContext;
import com.facebook.buck.rules.FakeBuildRuleParams;
+import com.facebook.buck.step.ExecutionContext;
+import com.facebook.buck.step.Step;
+import com.facebook.buck.step.TestExecutionContext;
+import com.facebook.buck.util.BuckConstant;
+import com.facebook.buck.util.ProjectFilesystem;
+import com.facebook.buck.testutil.MoreAsserts;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedSet;
import org.junit.Test;
+import java.io.IOException;
+import java.io.File;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.List;
+
/**
* Unit test for {@link PythonLibrary}.
*/
@@ -44,4 +65,72 @@
assertTrue(pythonLibrary.getProperties().is(LIBRARY));
assertSame(srcs, pythonLibrary.getPythonSrcs());
}
+
+ @Test
+ public void testFlattening() throws IOException {
+ BuildRuleResolver ruleResolver = new BuildRuleResolver();
+
+ BuildTarget pyLibraryTarget = BuildTargetFactory.newInstance("//:py_library");
+ ruleResolver.buildAndAddToIndex(
+ PythonLibrary.newPythonLibraryBuilder(new FakeAbstractBuildRuleBuilderParams())
+ .addSrc("baz.py")
+ .addSrc("foo/__init__.py")
+ .addSrc("foo/bar.py")
+ .setBuildTarget(pyLibraryTarget));
+ FakeBuildableContext buildableContext = new FakeBuildableContext();
+ BuildContext buildContext = createMock(BuildContext.class);
+ PythonLibrary rule = (PythonLibrary)ruleResolver.get(pyLibraryTarget).getBuildable();
+ List<Step> steps = rule.getBuildSteps(buildContext, buildableContext);
+
+ final String projectRoot = "/";
+ final String pylibpath = "__pylib_py_library";
+
+ ProjectFilesystem projectFilesystem = new ProjectFilesystem(new File(projectRoot));
+ ExecutionContext executionContext = TestExecutionContext.newBuilder()
+ .setProjectFilesystem(projectFilesystem)
+ .build();
+
+ MoreAsserts.assertSteps(
+ "python_library() should ensure each file is copied and has its destination directory made",
+ ImmutableList.of(
+ String.format(
+ "mkdir -p %s%s/%s",
+ projectRoot,
+ BuckConstant.GEN_DIR,
+ pylibpath
+ ),
+ String.format(
+ "mkdir -p %s%s/%s/foo",
+ projectRoot,
+ BuckConstant.GEN_DIR,
+ pylibpath
+ ),
+ String.format(
+ "cp baz.py %s/%s/baz.py",
+ BuckConstant.GEN_DIR,
+ pylibpath
+ ),
+ String.format(
+ "cp foo/__init__.py %s/%s/foo/__init__.py",
+ BuckConstant.GEN_DIR,
+ pylibpath
+ ),
+ String.format(
+ "cp foo/bar.py %s/%s/foo/bar.py",
+ BuckConstant.GEN_DIR,
+ pylibpath
+ )
+ ),
+ steps.subList(1, 6),
+ executionContext);
+
+ ImmutableSet<Path> artifacts = buildableContext.getRecordedArtifacts();
+ assertEquals(
+ ImmutableSet.of(
+ Paths.get(pylibpath, "baz.py"),
+ Paths.get(pylibpath, "foo/__init__.py"),
+ Paths.get(pylibpath, "foo/bar.py")
+ ),
+ artifacts);
+ }
}
diff --git a/test/com/facebook/buck/rules/FakeBuildableContext.java b/test/com/facebook/buck/rules/FakeBuildableContext.java
index e5f2b4d..1a90c48 100644
--- a/test/com/facebook/buck/rules/FakeBuildableContext.java
+++ b/test/com/facebook/buck/rules/FakeBuildableContext.java
@@ -17,6 +17,7 @@
package com.facebook.buck.rules;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
@@ -53,4 +54,8 @@
public ImmutableMap<String, String> getRecordedMetadata() {
return ImmutableMap.copyOf(metadata);
}
+
+ public ImmutableSet<Path> getRecordedArtifacts() {
+ return ImmutableSet.copyOf(artifacts);
+ }
}
