Skip to content

Don't link runtime compatibility library in pure Clang targets. #2134

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 29, 2019
Merged

Don't link runtime compatibility library in pure Clang targets. #2134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions Sources/Build/BuildPlan.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,11 +709,13 @@ public final class ProductBuildDescription {
args += ["-module-name", product.name.spm_mangledToC99ExtendedIdentifier()]
args += dylibs.map({ "-l" + $0.product.name })

// Add arguements needed for code coverage if it is enabled.
// Add arguments needed for code coverage if it is enabled.
if buildParameters.enableCodeCoverage {
args += ["-profile-coverage-mapping", "-profile-generate"]
}

let containsSwiftTargets = product.containsSwiftTargets

switch product.type {
case .library(.automatic):
fatalError()
Expand All @@ -731,7 +733,8 @@ public final class ProductBuildDescription {
args += ["-emit-library"]
case .executable:
// Link the Swift stdlib statically if requested.
if buildParameters.shouldLinkStaticSwiftStdlib {
if containsSwiftTargets,
buildParameters.shouldLinkStaticSwiftStdlib {
// FIXME: This does not work for linux yet (SR-648).
if !buildParameters.triple.isLinux() {
args += ["-static-stdlib"]
Expand All @@ -748,16 +751,24 @@ public final class ProductBuildDescription {
args += ["@\(linkFileListPath.pathString)"]

// Embed the swift stdlib library path inside tests and executables on Darwin.
switch product.type {
case .library: break
case .test, .executable:
if buildParameters.triple.isDarwin() {
let stdlib = buildParameters.toolchain.macosSwiftStdlib
args += ["-Xlinker", "-rpath", "-Xlinker", stdlib.pathString]
}
if containsSwiftTargets {
switch product.type {
case .library: break
case .test, .executable:
if buildParameters.triple.isDarwin() {
let stdlib = buildParameters.toolchain.macosSwiftStdlib
args += ["-Xlinker", "-rpath", "-Xlinker", stdlib.pathString]
}
}
}

// Add agruments from declared build settings.
// Don't link runtime compatibility patch libraries if there are no
// Swift sources in the target.
if !containsSwiftTargets {
args += ["-runtime-compatibility-version", "none"]
}

// Add arguments from declared build settings.
args += self.buildSettingsFlags()

// User arguments (from -Xlinker and -Xswiftc) should follow generated arguments to allow user overrides
Expand Down
12 changes: 12 additions & 0 deletions Sources/PackageModel/ResolvedModels.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,18 @@ public final class ResolvedProduct: ObjectIdentifierProtocol, CustomStringConver
public var description: String {
return "<ResolvedProduct: \(name)>"
}

/// True if this product contains Swift targets.
public var containsSwiftTargets: Bool {
// C targets can't import Swift targets in SwiftPM (at least not right
// now), so we can just look at the top-level targets.
//
// If that ever changes, we'll need to do something more complex here,
// recursively checking dependencies for SwiftTargets, and considering
// dynamic library targets to be Swift targets (since the dylib could
// contain Swift code we don't know about as part of this build).
return targets.contains { $0.underlyingTarget is SwiftTarget }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is just top-level targets; should it search for any targets?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you probably want to flatMap the recursiveDependencies first. Something like:

targets.flatMap { $0.recursiveDependencies }.contains { $0.underlyingTarget is SwiftTarget }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if target.lazy.flatMap will help with performance here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no reason to build a temporary here, so lazy would work, or targets.contains { $0.recursiveDependencies.contains { ... } }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One complications here is that we should be stopping this search at the first dynamic product in the dependency graph because the top-level product could be just made up of C targets and getting Swift targets via a dylib.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing though is that C targets can't import Swift targets in SwiftPM (at least not right now), so we can just do this for the top-level targets (as you've already done) and handle the other complications when we do start supporting the other case. Can you add a comment and file a JIRA for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://bugs.swift.org/browse/SR-10791 Thanks!

}
}

extension ResolvedTarget.Dependency: CustomStringConvertible {
Expand Down
14 changes: 8 additions & 6 deletions Tests/BuildTests/BuildPlanTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,14 +310,15 @@ final class BuildPlanTests: XCTestCase {
"/fake/path/to/swiftc", "-g", "-L", "/path/to/build/debug",
"-o", "/path/to/build/debug/exe", "-module-name", "exe", "-emit-executable",
"@/path/to/build/debug/exe.product/Objects.LinkFileList",
"-Xlinker", "-rpath", "-Xlinker", "/fake/path/lib/swift/macosx",
"-runtime-compatibility-version", "none",
])
#else
XCTAssertEqual(try result.buildProduct(for: "exe").linkArguments(), [
"/fake/path/to/swiftc", "-g", "-L", "/path/to/build/debug",
"-o", "/path/to/build/debug/exe", "-module-name", "exe", "-emit-executable",
"-Xlinker", "-rpath=$ORIGIN",
"@/path/to/build/debug/exe.product/Objects.LinkFileList",
"-runtime-compatibility-version", "none",
])
#endif

Expand Down Expand Up @@ -366,14 +367,15 @@ final class BuildPlanTests: XCTestCase {
"/fake/path/to/swiftc", "-lc++", "-g", "-L", "/path/to/build/debug", "-o",
"/path/to/build/debug/exe", "-module-name", "exe", "-emit-executable",
"@/path/to/build/debug/exe.product/Objects.LinkFileList",
"-Xlinker", "-rpath", "-Xlinker", "/fake/path/lib/swift/macosx",
"-runtime-compatibility-version", "none",
])
#else
XCTAssertEqual(try result.buildProduct(for: "exe").linkArguments(), [
"/fake/path/to/swiftc", "-lstdc++", "-g", "-L", "/path/to/build/debug", "-o",
"/path/to/build/debug/exe", "-module-name", "exe", "-emit-executable",
"-Xlinker", "-rpath=$ORIGIN",
"@/path/to/build/debug/exe.product/Objects.LinkFileList",
"-runtime-compatibility-version", "none",
])
#endif

Expand Down Expand Up @@ -857,12 +859,12 @@ final class BuildPlanTests: XCTestCase {
XCTAssertEqual(lib.moduleMap, AbsolutePath("/path/to/build/debug/lib.build/module.modulemap"))

#if os(macOS)
XCTAssertEqual(try result.buildProduct(for: "lib").linkArguments(), ["/fake/path/to/swiftc", "-lc++", "-g", "-L", "/path/to/build/debug", "-o", "/path/to/build/debug/liblib.dylib", "-module-name", "lib", "-emit-library", "@/path/to/build/debug/lib.product/Objects.LinkFileList"])
XCTAssertEqual(try result.buildProduct(for: "lib").linkArguments(), ["/fake/path/to/swiftc", "-lc++", "-g", "-L", "/path/to/build/debug", "-o", "/path/to/build/debug/liblib.dylib", "-module-name", "lib", "-emit-library", "@/path/to/build/debug/lib.product/Objects.LinkFileList", "-runtime-compatibility-version", "none"])

XCTAssertEqual(try result.buildProduct(for: "exe").linkArguments(), ["/fake/path/to/swiftc", "-g", "-L", "/path/to/build/debug", "-o", "/path/to/build/debug/exe", "-module-name", "exe", "-emit-executable", "@/path/to/build/debug/exe.product/Objects.LinkFileList", "-Xlinker", "-rpath", "-Xlinker", "/fake/path/lib/swift/macosx",])
XCTAssertEqual(try result.buildProduct(for: "exe").linkArguments(), ["/fake/path/to/swiftc", "-g", "-L", "/path/to/build/debug", "-o", "/path/to/build/debug/exe", "-module-name", "exe", "-emit-executable", "@/path/to/build/debug/exe.product/Objects.LinkFileList", "-runtime-compatibility-version", "none"])
#else
XCTAssertEqual(try result.buildProduct(for: "lib").linkArguments(), ["/fake/path/to/swiftc", "-lstdc++", "-g", "-L", "/path/to/build/debug", "-o", "/path/to/build/debug/liblib.so", "-module-name", "lib", "-emit-library", "-Xlinker", "-rpath=$ORIGIN", "@/path/to/build/debug/lib.product/Objects.LinkFileList"])
XCTAssertEqual(try result.buildProduct(for: "exe").linkArguments(), ["/fake/path/to/swiftc", "-g", "-L", "/path/to/build/debug", "-o", "/path/to/build/debug/exe", "-module-name", "exe", "-emit-executable", "-Xlinker", "-rpath=$ORIGIN", "@/path/to/build/debug/exe.product/Objects.LinkFileList"])
XCTAssertEqual(try result.buildProduct(for: "lib").linkArguments(), ["/fake/path/to/swiftc", "-lstdc++", "-g", "-L", "/path/to/build/debug", "-o", "/path/to/build/debug/liblib.so", "-module-name", "lib", "-emit-library", "-Xlinker", "-rpath=$ORIGIN", "@/path/to/build/debug/lib.product/Objects.LinkFileList", "-runtime-compatibility-version", "none"])
XCTAssertEqual(try result.buildProduct(for: "exe").linkArguments(), ["/fake/path/to/swiftc", "-g", "-L", "/path/to/build/debug", "-o", "/path/to/build/debug/exe", "-module-name", "exe", "-emit-executable", "-Xlinker", "-rpath=$ORIGIN", "@/path/to/build/debug/exe.product/Objects.LinkFileList", "-runtime-compatibility-version", "none"])
#endif
}

Expand Down