From 82edf0bdbe63ef99cff17114458d4bd442a55fd7 Mon Sep 17 00:00:00 2001 From: Andrew Gallagher Date: Sun, 27 Dec 2015 03:40:43 -0800 Subject: [PATCH] python: fix extension locations in merged link strategy Summary: Previously, using omnibus linking grouped the linked roots with all other native libraries namespaced using the shared library name. However, in languages such as Python, the roots may be C/C++ extensions, and therefore need to be loaded via the module search path via their modules names. This makes the omnibus linker return the generated root libraries separately from the others, so that the caller can special case their layout and handling. This is used in the Python code, so that C/C++ extensions which are roots in the link get properly placed in the module tree. Test Plan: added test Reviewed By: beefon fb-gh-sync-id: 82bed8f --- src/com/facebook/buck/cxx/Omnibus.java | 41 ++++++++--- .../buck/python/CxxPythonExtension.java | 4 ++ .../python/CxxPythonExtensionDescription.java | 5 ++ src/com/facebook/buck/python/PythonUtil.java | 50 ++++++++++---- test/com/facebook/buck/cxx/OmnibusTest.java | 15 ++-- .../python/PythonBinaryDescriptionTest.java | 68 +++++++++++++++++++ 6 files changed, 157 insertions(+), 26 deletions(-) diff --git a/src/com/facebook/buck/cxx/Omnibus.java b/src/com/facebook/buck/cxx/Omnibus.java index 325ae8d6427..34393afadd1 100644 --- a/src/com/facebook/buck/cxx/Omnibus.java +++ b/src/com/facebook/buck/cxx/Omnibus.java @@ -33,6 +33,7 @@ import com.facebook.buck.rules.SourcePathResolver; import com.facebook.buck.rules.args.Arg; import com.facebook.buck.rules.args.SourcePathArg; +import com.facebook.buck.util.immutables.BuckStyleImmutable; import com.google.common.base.Functions; import com.google.common.base.Preconditions; import com.google.common.base.Predicates; @@ -300,7 +301,7 @@ protected static SharedLibrary createRoot( rootSoname, argsBuilder.build())); - return ImmutableSharedLibrary.of(rootSoname, new BuildTargetSourcePath(rootTarget)); + return SharedLibrary.of(rootSoname, new BuildTargetSourcePath(rootTarget)); } private static ImmutableList createUndefinedSymbolsArgs( @@ -423,7 +424,7 @@ protected static SharedLibrary createOmnibus( omnibusSoname, argsBuilder.build())); - return ImmutableSharedLibrary.of(omnibusSoname, new BuildTargetSourcePath(omnibusTarget)); + return SharedLibrary.of(omnibusSoname, new BuildTargetSourcePath(omnibusTarget)); } /** @@ -438,7 +439,7 @@ protected static SharedLibrary createOmnibus( * @return a map of shared library names to their containing {@link SourcePath}s. * @throws NoSuchBuildTargetException */ - public static ImmutableMap getSharedLibraries( + public static OmnibusLibraries getSharedLibraries( BuildRuleParams params, BuildRuleResolver ruleResolver, SourcePathResolver pathResolver, @@ -447,7 +448,7 @@ public static ImmutableMap getSharedLibraries( Iterable nativeLinkableRoots) throws NoSuchBuildTargetException { - ImmutableMap.Builder libs = ImmutableMap.builder(); + OmnibusLibraries.Builder libs = OmnibusLibraries.builder(); OmnibusSpec spec = buildSpec(cxxPlatform, nativeLinkTargetRoots, nativeLinkableRoots); @@ -460,19 +461,22 @@ public static ImmutableMap getSharedLibraries( for (SharedNativeLinkTarget root : spec.getRoots().values()) { SharedLibrary lib = createRoot(params, ruleResolver, pathResolver, cxxPlatform, spec, dummyOmnibus, root); - libs.put(lib.getSoname(), lib.getPath()); + libs.putRoots(root.getBuildTarget(), lib); } // If there are any body nodes, generate the giant merged omnibus library. if (!spec.getBody().isEmpty()) { SharedLibrary omnibus = createOmnibus(params, ruleResolver, pathResolver, cxxPlatform, spec); - libs.put(omnibus.getSoname(), omnibus.getPath()); + libs.addLibraries(omnibus); } // Lastly, add in any shared libraries from excluded nodes the normal way. for (NativeLinkable nativeLinkable : spec.getExcluded().values()) { if (nativeLinkable.getPreferredLinkage(cxxPlatform) != NativeLinkable.Linkage.STATIC) { - libs.putAll(nativeLinkable.getSharedLibraries(cxxPlatform)); + for (Map.Entry ent : + nativeLinkable.getSharedLibraries(cxxPlatform).entrySet()) { + libs.addLibraries(SharedLibrary.of(ent.getKey(), ent.getValue())); + } } } @@ -523,7 +527,8 @@ public void verify() { } @Value.Immutable - interface SharedLibrary { + @BuckStyleImmutable + interface AbstractSharedLibrary { @Value.Parameter String getSoname(); @@ -533,4 +538,24 @@ interface SharedLibrary { } + @Value.Immutable + @BuckStyleImmutable + abstract static class AbstractOmnibusLibraries { + + @Value.Parameter + public abstract ImmutableMap getRoots(); + + @Value.Parameter + public abstract ImmutableList getLibraries(); + + public ImmutableMap toSonameMap() { + ImmutableMap.Builder libs = ImmutableMap.builder(); + for (SharedLibrary lib : Iterables.concat(getRoots().values(), getLibraries())) { + libs.put(lib.getSoname(), lib.getPath()); + } + return libs.build(); + } + + } + } diff --git a/src/com/facebook/buck/python/CxxPythonExtension.java b/src/com/facebook/buck/python/CxxPythonExtension.java index fb76fbf4bde..7ecf501417b 100644 --- a/src/com/facebook/buck/python/CxxPythonExtension.java +++ b/src/com/facebook/buck/python/CxxPythonExtension.java @@ -26,6 +26,8 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Function; +import java.nio.file.Path; + public abstract class CxxPythonExtension extends NoopBuildRule implements PythonPackagable { public CxxPythonExtension( @@ -40,6 +42,8 @@ protected abstract BuildRule getExtension( CxxPlatform cxxPlatform) throws NoSuchBuildTargetException; + public abstract Path getModule(); + @Override public abstract PythonPackageComponents getPythonPackageComponents( PythonPlatform pythonPlatform, diff --git a/src/com/facebook/buck/python/CxxPythonExtensionDescription.java b/src/com/facebook/buck/python/CxxPythonExtensionDescription.java index 541764fe193..425838520a3 100644 --- a/src/com/facebook/buck/python/CxxPythonExtensionDescription.java +++ b/src/com/facebook/buck/python/CxxPythonExtensionDescription.java @@ -310,6 +310,11 @@ protected BuildRule getExtension( CxxDescriptionEnhancer.SHARED_FLAVOR)); } + @Override + public Path getModule() { + return module; + } + @Override public PythonPackageComponents getPythonPackageComponents( PythonPlatform pythonPlatform, diff --git a/src/com/facebook/buck/python/PythonUtil.java b/src/com/facebook/buck/python/PythonUtil.java index b2c744d9874..c0f11ceaf95 100644 --- a/src/com/facebook/buck/python/PythonUtil.java +++ b/src/com/facebook/buck/python/PythonUtil.java @@ -18,6 +18,8 @@ import com.facebook.buck.cxx.CxxPlatform; import com.facebook.buck.cxx.NativeLinkables; +import com.facebook.buck.cxx.OmnibusLibraries; +import com.facebook.buck.cxx.SharedLibrary; import com.facebook.buck.cxx.SharedNativeLinkTarget; import com.facebook.buck.cxx.NativeLinkable; import com.facebook.buck.cxx.Omnibus; @@ -172,9 +174,6 @@ public Iterable visit(BuildRule rule) throws NoSuchBuildTargetExcepti } }.start(); - // Collect up native libraries. - ImmutableMap sharedLibs; - // For the merged strategy, build up the lists of included native linkable roots, and the // excluded native linkable roots. if (nativeLinkStrategy == NativeLinkStrategy.MERGED) { @@ -188,11 +187,14 @@ public Iterable visit(BuildRule rule) throws NoSuchBuildTargetExcepti } // All C/C++ python extensions are included native link targets. + Map includedExtensions = new LinkedHashMap<>(); for (CxxPythonExtension extension : extensions.values()) { - includedNativeLinkTargetRoots.add(extension.getNativeLinkTarget(pythonPlatform)); + SharedNativeLinkTarget target = extension.getNativeLinkTarget(pythonPlatform); + includedExtensions.put(target.getBuildTarget(), extension); + includedNativeLinkTargetRoots.add(target); } - sharedLibs = + OmnibusLibraries libraries = Omnibus.getSharedLibraries( params, ruleResolver, @@ -200,6 +202,28 @@ public Iterable visit(BuildRule rule) throws NoSuchBuildTargetExcepti cxxPlatform, includedNativeLinkTargetRoots, nativeLinkableRoots.values()); + + // Add all the roots from the omnibus link. If it's an extension, add it as a module. + // Otherwise, add it as a native library. + for (Map.Entry root : libraries.getRoots().entrySet()) { + CxxPythonExtension extension = includedExtensions.get(root.getKey()); + if (extension != null) { + allComponents.addModule(extension.getModule(), root.getValue().getPath(), root.getKey()); + } else { + allComponents.addNativeLibraries( + Paths.get(root.getValue().getSoname()), + root.getValue().getPath(), + root.getKey()); + } + } + + // Add all remaining libraries as native libraries. + for (SharedLibrary library : libraries.getLibraries()) { + allComponents.addNativeLibraries( + Paths.get(library.getSoname()), + library.getPath(), + params.getBuildTarget()); + } } else { // For regular linking, add all extensions via the package components interface. @@ -209,20 +233,20 @@ public Iterable visit(BuildRule rule) throws NoSuchBuildTargetExcepti entry.getKey()); } - sharedLibs = + // Add all the native libraries. + ImmutableMap libs = NativeLinkables.getTransitiveSharedLibraries( cxxPlatform, params.getDeps(), Predicates.instanceOf(PythonPackagable.class)); + for (Map.Entry ent : libs.entrySet()) { + allComponents.addNativeLibraries( + Paths.get(ent.getKey()), + ent.getValue(), + params.getBuildTarget()); + } } - // Add all the native libraries. - for (Map.Entry ent : sharedLibs.entrySet()) { - allComponents.addNativeLibraries( - Paths.get(ent.getKey()), - ent.getValue(), - params.getBuildTarget()); - } return allComponents.build(); } diff --git a/test/com/facebook/buck/cxx/OmnibusTest.java b/test/com/facebook/buck/cxx/OmnibusTest.java index 85a9589db93..c0121437a63 100644 --- a/test/com/facebook/buck/cxx/OmnibusTest.java +++ b/test/com/facebook/buck/cxx/OmnibusTest.java @@ -81,7 +81,8 @@ public void includedDeps() throws NoSuchBuildTargetException { pathResolver, CxxPlatformUtils.DEFAULT_PLATFORM, ImmutableList.of(root), - ImmutableList.of()); + ImmutableList.of()) + .toSonameMap(); assertThat( libs.keySet(), Matchers.containsInAnyOrder(root.getBuildTarget().toString(), "libomnibus.so")); @@ -139,7 +140,8 @@ public void excludedAndIncludedDeps() throws NoSuchBuildTargetException { pathResolver, CxxPlatformUtils.DEFAULT_PLATFORM, ImmutableList.of(root), - ImmutableList.of()); + ImmutableList.of()) + .toSonameMap(); assertThat( libs.keySet(), Matchers.containsInAnyOrder( @@ -202,7 +204,8 @@ public void excludedDepExcludesTransitiveDep() throws NoSuchBuildTargetException pathResolver, CxxPlatformUtils.DEFAULT_PLATFORM, ImmutableList.of(root), - ImmutableList.of()); + ImmutableList.of()) + .toSonameMap(); assertThat( libs.keySet(), Matchers.containsInAnyOrder( @@ -269,7 +272,8 @@ public void depOfExcludedRoot() throws NoSuchBuildTargetException { pathResolver, CxxPlatformUtils.DEFAULT_PLATFORM, ImmutableList.of(root), - ImmutableList.of(excludedRoot)); + ImmutableList.of(excludedRoot)) + .toSonameMap(); assertThat( libs.keySet(), Matchers.containsInAnyOrder( @@ -334,7 +338,8 @@ public void commondDepOfIncludedAndExcludedRoots() throws NoSuchBuildTargetExcep pathResolver, CxxPlatformUtils.DEFAULT_PLATFORM, ImmutableList.of(root), - ImmutableList.of(excludedRoot)); + ImmutableList.of(excludedRoot)) + .toSonameMap(); assertThat( libs.keySet(), Matchers.containsInAnyOrder( diff --git a/test/com/facebook/buck/python/PythonBinaryDescriptionTest.java b/test/com/facebook/buck/python/PythonBinaryDescriptionTest.java index 422aa36b2aa..42395ec7699 100644 --- a/test/com/facebook/buck/python/PythonBinaryDescriptionTest.java +++ b/test/com/facebook/buck/python/PythonBinaryDescriptionTest.java @@ -21,8 +21,10 @@ import com.facebook.buck.cli.BuildTargetNodeToBuildRuleTransformer; import com.facebook.buck.cli.FakeBuckConfig; import com.facebook.buck.cxx.CxxBinaryBuilder; +import com.facebook.buck.cxx.CxxBuckConfig; import com.facebook.buck.cxx.CxxLibraryBuilder; import com.facebook.buck.cxx.CxxPlatformUtils; +import com.facebook.buck.cxx.PrebuiltCxxLibraryBuilder; import com.facebook.buck.io.AlwaysFoundExecutableFinder; import com.facebook.buck.model.BuildTarget; import com.facebook.buck.model.BuildTargetFactory; @@ -61,9 +63,18 @@ import java.nio.file.Path; import java.nio.file.Paths; +import java.util.Map; public class PythonBinaryDescriptionTest { + private static final BuildTarget PYTHON2_DEP_TARGET = + BuildTargetFactory.newInstance("//:python2_dep"); + private static final PythonPlatform PY2 = + PythonPlatform.of( + ImmutableFlavor.of("py2"), + new PythonEnvironment(Paths.get("python2"), PythonVersion.of("2.6")), + Optional.of(PYTHON2_DEP_TARGET)); + @Test public void thatComponentSourcePathDepsPropagateProperly() throws Exception { BuildRuleResolver resolver = @@ -447,4 +458,61 @@ public NativeLinkStrategy getNativeLinkStrategy() { Matchers.containsInAnyOrder("libtransitive_dep.so", "libdep.so", "libcxx.so")); } + @Test + public void extensionDepUsingMergedNativeLinkStrategy() throws Exception { + FlavorDomain pythonPlatforms = + new FlavorDomain<>( + "Python Platform", + ImmutableMap.of(PY2.getFlavor(), PY2)); + + PrebuiltCxxLibraryBuilder python2Builder = + new PrebuiltCxxLibraryBuilder(PYTHON2_DEP_TARGET) + .setHeaderOnly(true) + .setExportedLinkerFlags(ImmutableList.of("-lpython2")); + + CxxPythonExtensionBuilder extensionBuilder = + new CxxPythonExtensionBuilder( + BuildTargetFactory.newInstance("//:extension"), + pythonPlatforms, + new CxxBuckConfig(FakeBuckConfig.builder().build()), + CxxPlatformUtils.DEFAULT_PLATFORMS); + extensionBuilder.setBaseModule("hello"); + + PythonBuckConfig config = + new PythonBuckConfig(FakeBuckConfig.builder().build(), new AlwaysFoundExecutableFinder()) { + @Override + public NativeLinkStrategy getNativeLinkStrategy() { + return NativeLinkStrategy.MERGED; + } + }; + PythonBinaryBuilder binaryBuilder = + new PythonBinaryBuilder( + BuildTargetFactory.newInstance("//:bin"), + config, + pythonPlatforms, + CxxPlatformUtils.DEFAULT_PLATFORM, + CxxPlatformUtils.DEFAULT_PLATFORMS); + binaryBuilder.setMainModule("main"); + binaryBuilder.setDeps(ImmutableSortedSet.of(extensionBuilder.getTarget())); + + BuildRuleResolver resolver = + new BuildRuleResolver( + TargetGraphFactory.newInstance( + python2Builder.build(), + extensionBuilder.build(), + binaryBuilder.build()), + new BuildTargetNodeToBuildRuleTransformer()); + python2Builder.build(resolver); + extensionBuilder.build(resolver); + PythonBinary binary = (PythonBinary) binaryBuilder.build(resolver); + assertThat( + binary.getComponents().getNativeLibraries().entrySet(), + Matchers.>empty()); + assertThat( + Iterables.transform( + binary.getComponents().getModules().keySet(), + Functions.toStringFunction()), + Matchers.containsInAnyOrder("hello/extension.so")); + } + }