Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Commit

Permalink
python: fix extension locations in merged link strategy
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@andrewjcg
andrewjcg authored and facebook-github-bot-2 committed Dec 27, 2015
1 parent bb2e5b5 commit 82edf0b
Show file tree
Hide file tree
Showing 6 changed files with 157 additions and 26 deletions.
41 changes: 33 additions & 8 deletions src/com/facebook/buck/cxx/Omnibus.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Arg> createUndefinedSymbolsArgs(
Expand Down Expand Up @@ -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));
}

/**
Expand All @@ -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<String, SourcePath> getSharedLibraries(
public static OmnibusLibraries getSharedLibraries(
BuildRuleParams params,
BuildRuleResolver ruleResolver,
SourcePathResolver pathResolver,
Expand All @@ -447,7 +448,7 @@ public static ImmutableMap<String, SourcePath> getSharedLibraries(
Iterable<? extends NativeLinkable> nativeLinkableRoots)
throws NoSuchBuildTargetException {

ImmutableMap.Builder<String, SourcePath> libs = ImmutableMap.builder();
OmnibusLibraries.Builder libs = OmnibusLibraries.builder();

OmnibusSpec spec = buildSpec(cxxPlatform, nativeLinkTargetRoots, nativeLinkableRoots);

Expand All @@ -460,19 +461,22 @@ public static ImmutableMap<String, SourcePath> 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<String, SourcePath> ent :
nativeLinkable.getSharedLibraries(cxxPlatform).entrySet()) {
libs.addLibraries(SharedLibrary.of(ent.getKey(), ent.getValue()));
}
}
}

Expand Down Expand Up @@ -523,7 +527,8 @@ public void verify() {
}

@Value.Immutable
interface SharedLibrary {
@BuckStyleImmutable
interface AbstractSharedLibrary {

@Value.Parameter
String getSoname();
Expand All @@ -533,4 +538,24 @@ interface SharedLibrary {

}

@Value.Immutable
@BuckStyleImmutable
abstract static class AbstractOmnibusLibraries {

@Value.Parameter
public abstract ImmutableMap<BuildTarget, SharedLibrary> getRoots();

@Value.Parameter
public abstract ImmutableList<SharedLibrary> getLibraries();

public ImmutableMap<String, SourcePath> toSonameMap() {
ImmutableMap.Builder<String, SourcePath> libs = ImmutableMap.builder();
for (SharedLibrary lib : Iterables.concat(getRoots().values(), getLibraries())) {
libs.put(lib.getSoname(), lib.getPath());
}
return libs.build();
}

}

}
4 changes: 4 additions & 0 deletions src/com/facebook/buck/python/CxxPythonExtension.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -40,6 +42,8 @@ protected abstract BuildRule getExtension(
CxxPlatform cxxPlatform)
throws NoSuchBuildTargetException;

public abstract Path getModule();

@Override
public abstract PythonPackageComponents getPythonPackageComponents(
PythonPlatform pythonPlatform,
Expand Down
5 changes: 5 additions & 0 deletions src/com/facebook/buck/python/CxxPythonExtensionDescription.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,11 @@ protected BuildRule getExtension(
CxxDescriptionEnhancer.SHARED_FLAVOR));
}

@Override
public Path getModule() {
return module;
}

@Override
public PythonPackageComponents getPythonPackageComponents(
PythonPlatform pythonPlatform,
Expand Down
50 changes: 37 additions & 13 deletions src/com/facebook/buck/python/PythonUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -172,9 +174,6 @@ public Iterable<BuildRule> visit(BuildRule rule) throws NoSuchBuildTargetExcepti
}
}.start();

// Collect up native libraries.
ImmutableMap<String, SourcePath> sharedLibs;

// For the merged strategy, build up the lists of included native linkable roots, and the
// excluded native linkable roots.
if (nativeLinkStrategy == NativeLinkStrategy.MERGED) {
Expand All @@ -188,18 +187,43 @@ public Iterable<BuildRule> visit(BuildRule rule) throws NoSuchBuildTargetExcepti
}

// All C/C++ python extensions are included native link targets.
Map<BuildTarget, CxxPythonExtension> 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,
pathResolver,
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<BuildTarget, SharedLibrary> 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.
Expand All @@ -209,20 +233,20 @@ public Iterable<BuildRule> visit(BuildRule rule) throws NoSuchBuildTargetExcepti
entry.getKey());
}

sharedLibs =
// Add all the native libraries.
ImmutableMap<String, SourcePath> libs =
NativeLinkables.getTransitiveSharedLibraries(
cxxPlatform,
params.getDeps(),
Predicates.instanceOf(PythonPackagable.class));
for (Map.Entry<String, SourcePath> ent : libs.entrySet()) {
allComponents.addNativeLibraries(
Paths.get(ent.getKey()),
ent.getValue(),
params.getBuildTarget());
}
}

// Add all the native libraries.
for (Map.Entry<String, SourcePath> ent : sharedLibs.entrySet()) {
allComponents.addNativeLibraries(
Paths.get(ent.getKey()),
ent.getValue(),
params.getBuildTarget());
}

return allComponents.build();
}
Expand Down
15 changes: 10 additions & 5 deletions test/com/facebook/buck/cxx/OmnibusTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ public void includedDeps() throws NoSuchBuildTargetException {
pathResolver,
CxxPlatformUtils.DEFAULT_PLATFORM,
ImmutableList.of(root),
ImmutableList.<NativeLinkable>of());
ImmutableList.<NativeLinkable>of())
.toSonameMap();
assertThat(
libs.keySet(),
Matchers.containsInAnyOrder(root.getBuildTarget().toString(), "libomnibus.so"));
Expand Down Expand Up @@ -139,7 +140,8 @@ public void excludedAndIncludedDeps() throws NoSuchBuildTargetException {
pathResolver,
CxxPlatformUtils.DEFAULT_PLATFORM,
ImmutableList.of(root),
ImmutableList.<NativeLinkable>of());
ImmutableList.<NativeLinkable>of())
.toSonameMap();
assertThat(
libs.keySet(),
Matchers.containsInAnyOrder(
Expand Down Expand Up @@ -202,7 +204,8 @@ public void excludedDepExcludesTransitiveDep() throws NoSuchBuildTargetException
pathResolver,
CxxPlatformUtils.DEFAULT_PLATFORM,
ImmutableList.of(root),
ImmutableList.<NativeLinkable>of());
ImmutableList.<NativeLinkable>of())
.toSonameMap();
assertThat(
libs.keySet(),
Matchers.containsInAnyOrder(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down
68 changes: 68 additions & 0 deletions test/com/facebook/buck/python/PythonBinaryDescriptionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -447,4 +458,61 @@ public NativeLinkStrategy getNativeLinkStrategy() {
Matchers.containsInAnyOrder("libtransitive_dep.so", "libdep.so", "libcxx.so"));
}

@Test
public void extensionDepUsingMergedNativeLinkStrategy() throws Exception {
FlavorDomain<PythonPlatform> 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.<Map.Entry<Path, SourcePath>>empty());
assertThat(
Iterables.transform(
binary.getComponents().getModules().keySet(),
Functions.toStringFunction()),
Matchers.containsInAnyOrder("hello/extension.so"));
}

}

0 comments on commit 82edf0b

Please sign in to comment.