Skip to content

Add cxx interop support to symbolgraph-extract #7610

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.

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 4 commits into from
Jun 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,20 @@ public final class ClangTargetBuildDescription {
}
}

/// Determines the arguments needed to run `swift-symbolgraph-extract` for
/// this module.
public func symbolGraphExtractArguments() throws -> [String] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public func symbolGraphExtractArguments() throws -> [String] {
package func symbolGraphExtractArguments() throws -> [String] {

var args = [String]()

if self.clangTarget.isCXX {
args += ["-cxx-interoperability-mode=default"]
}
if let cxxLanguageStandard = self.clangTarget.cxxLanguageStandard {
args += ["-Xcc", "-std=\(cxxLanguageStandard)"]
}
return args
}

/// Builds up basic compilation arguments for a source file in this target; these arguments may be different for C++
/// vs non-C++.
/// NOTE: The parameter to specify whether to get C++ semantics is currently optional, but this is only for revlock
Expand Down
83 changes: 63 additions & 20 deletions Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift
Original file line number Diff line number Diff line change
Expand Up @@ -547,26 +547,8 @@ public final class SwiftTargetBuildDescription {
args += ["-color-diagnostics"]
}

// If this is a generated test discovery target or a test entry point, it might import a test
// target that is built with C++ interop enabled. In that case, the test
// discovery target must enable C++ interop as well
switch testTargetRole {
case .discovery, .entryPoint:
for dependency in try self.target.recursiveTargetDependencies() {
let dependencyScope = self.buildParameters.createScope(for: dependency)
let dependencySwiftFlags = dependencyScope.evaluate(.OTHER_SWIFT_FLAGS)
if let interopModeFlag = dependencySwiftFlags.first(where: { $0.hasPrefix("-cxx-interoperability-mode=") }) {
args += [interopModeFlag]
if interopModeFlag != "-cxx-interoperability-mode=off" {
if let cxxStandard = self.package.manifest.cxxLanguageStandard {
args += ["-Xcc", "-std=\(cxxStandard)"]
}
}
break
}
}
default: break
}
args += try self.cxxInteroperabilityModeArguments(
propagateFromCurrentModuleOtherSwiftFlags: false)

// Add arguments from declared build settings.
args += try self.buildSettingsFlags()
Expand Down Expand Up @@ -644,6 +626,67 @@ public final class SwiftTargetBuildDescription {

return args
}

/// Determines the arguments needed to run `swift-symbolgraph-extract` for
/// this module.
public func symbolGraphExtractArguments() throws -> [String] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public func symbolGraphExtractArguments() throws -> [String] {
package func symbolGraphExtractArguments() throws -> [String] {

var args = [String]()
args += try self.cxxInteroperabilityModeArguments(
propagateFromCurrentModuleOtherSwiftFlags: true)
return args
Comment on lines +633 to +636
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be simplified to just return self.cxxInteroperabilityModeArguments(propagateFromCurrentModuleOtherSwiftFlags: true)?

Copy link
Member Author

Choose a reason for hiding this comment

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

A follow up PR adds to args so I'd like to leave this as is

}

// FIXME: this function should operation on a strongly typed buildSetting
// Move logic from PackageBuilder here.
/// Determines the arguments needed for cxx interop for this module.
func cxxInteroperabilityModeArguments(
// FIXME: Remove argument
// This argument is added as a stop gap to support generating arguments
// for tools which currently don't leverage "OTHER_SWIFT_FLAGS". In the
// fullness of time this function should operate on a strongly typed
// "interopMode" property of SwiftTargetBuildDescription instead of
// digging through "OTHER_SWIFT_FLAGS" manually.
propagateFromCurrentModuleOtherSwiftFlags: Bool
) throws -> [String] {
func cxxInteroperabilityModeAndStandard(
for module: ResolvedModule
) -> [String]? {
let scope = self.buildParameters.createScope(for: module)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct to use buildParameters of the context here for dependency scopes? What if if this i.e. executable target that has a plugin or a macro dependency and that in turn has other dependencies, we'd use "destination" parameters for their scope, which seems counter-intuitive?

It seems like we need to avoid adding new uses of recursiveTargetDependencies() in this context when build parameters are involved, because they are strongly connected to a destination, and instead traverse a build plan graph and use TargetDependencyDescription.

Copy link
Member Author

@rauhul rauhul Jun 3, 2024

Choose a reason for hiding this comment

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

I'd rather not change this logic in this PR, im simply moving the existing code such that symbol graph can use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I'm mentioning this because of the comment - Move logic from PackageBuilder here., I don't think that would be possible to do because of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the point of that comment is to say, we should not throw away the typed interop information at that layer and try to recover it here by parsing flags, we should instead determine the flags at this layer

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could do the same thing as .SWIFT_VERSION and add a new declaration for assigments like that which means that you won't need to deal with prefixes here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mean it has to be in this PR, just an idea how to avoid having to prefix match things...

let flags = scope.evaluate(.OTHER_SWIFT_FLAGS)
let mode = flags.first { $0.hasPrefix("-cxx-interoperability-mode=") }
guard let mode else { return nil }
// FIXME: Use a stored self.cxxLanguageStandard property
// It definitely should _never_ reach back into the manifest
if let cxxStandard = self.package.manifest.cxxLanguageStandard {
return [mode, "-Xcc", "-std=\(cxxStandard)"]
} else {
return [mode]
}
}

if propagateFromCurrentModuleOtherSwiftFlags {
// Look for cxx interop mode in the current module, if set exit early,
// the flag is already present.
if let args = cxxInteroperabilityModeAndStandard(for: self.target) {
return args
}
}

// Implicitly propagate cxx interop flags for generated test targets.
// If the current module doesn't have cxx interop mode set, search
// through the module's dependencies looking for the a module that
// enables cxx interop and copy it's flag.
switch self.testTargetRole {
case .discovery, .entryPoint:
for module in try self.target.recursiveTargetDependencies() {
if let args = cxxInteroperabilityModeAndStandard(for: module) {
return args
}
}
default: break
}
return []
}

/// When `scanInvocation` argument is set to `true`, omit the side-effect producing arguments
/// such as emitting a module or supplementary outputs.
Expand Down
9 changes: 9 additions & 0 deletions Sources/Build/BuildDescription/TargetBuildDescription.swift
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,13 @@ public enum TargetBuildDescription {
return clangTargetBuildDescription.toolsVersion
}
}

/// Determines the arguments needed to run `swift-symbolgraph-extract` for
/// this module.
public func symbolGraphExtractArguments() throws -> [String] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public func symbolGraphExtractArguments() throws -> [String] {
package func symbolGraphExtractArguments() throws -> [String] {

switch self {
case .swift(let target): try target.symbolGraphExtractArguments()
case .clang(let target): try target.symbolGraphExtractArguments()
}
}
}
7 changes: 7 additions & 0 deletions Sources/Build/BuildPlan/BuildPlan.swift
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,13 @@ public class BuildPlan: SPMBuildCore.BuildPlan {
try binaryTarget.parseXCFrameworks(for: triple, fileSystem: self.fileSystem)
}
}

public func symbolGraphExtractArguments(for module: ResolvedModule) throws -> [String] {
guard let description = self.targetMap[module.id] else {
throw InternalError("Expected description for module \(module)")
}
return try description.symbolGraphExtractArguments()
}
}

extension Basics.Diagnostic {
Expand Down
3 changes: 3 additions & 0 deletions Sources/Commands/Utilities/SymbolGraphExtract.swift
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ public struct SymbolGraphExtract {

// Construct arguments for extracting symbols for a single target.
var commandLine = [self.tool.pathString]
commandLine += try buildPlan.symbolGraphExtractArguments(for: module)

// FIXME: everything here should be in symbolGraphExtractArguments
commandLine += ["-module-name", module.c99name]
commandLine += try buildParameters.tripleArgs(for: module)
commandLine += try buildPlan.createAPIToolCommonArgs(includeLibrarySearchPaths: true)
Expand Down
2 changes: 2 additions & 0 deletions Sources/SPMBuildCore/BuildSystem/BuildSystem.swift
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ public protocol BuildPlan {

func createAPIToolCommonArgs(includeLibrarySearchPaths: Bool) throws -> [String]
func createREPLArguments() throws -> [String]

func symbolGraphExtractArguments(for module: ResolvedModule) throws -> [String]
}

public protocol BuildSystemFactory {
Expand Down
105 changes: 105 additions & 0 deletions Tests/BuildTests/BuildPlanTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1715,13 +1715,36 @@ final class BuildPlanTests: XCTestCase {
)
)

// Assert compile args for swift modules importing cxx modules
let swiftInteropLib = try result.target(for: "swiftInteropLib").swiftTarget().compileArguments()
XCTAssertMatch(
swiftInteropLib,
[.anySequence, "-cxx-interoperability-mode=default", "-Xcc", "-std=c++1z", .anySequence]
)
let swiftLib = try result.target(for: "swiftLib").swiftTarget().compileArguments()
XCTAssertNoMatch(swiftLib, [.anySequence, "-Xcc", "-std=c++1z", .anySequence])

// Assert symbolgraph-extract args for swift modules importing cxx modules
do {
let swiftInteropLib = try result.target(for: "swiftInteropLib").swiftTarget().compileArguments()
XCTAssertMatch(
swiftInteropLib,
[.anySequence, "-cxx-interoperability-mode=default", "-Xcc", "-std=c++1z", .anySequence]
)
let swiftLib = try result.target(for: "swiftLib").swiftTarget().compileArguments()
XCTAssertNoMatch(swiftLib, [.anySequence, "-Xcc", "-std=c++1z", .anySequence])
}

// Assert symbolgraph-extract args for cxx modules
do {
let swiftInteropLib = try result.target(for: "swiftInteropLib").swiftTarget().compileArguments()
XCTAssertMatch(
swiftInteropLib,
[.anySequence, "-cxx-interoperability-mode=default", "-Xcc", "-std=c++1z", .anySequence]
)
let swiftLib = try result.target(for: "swiftLib").swiftTarget().compileArguments()
XCTAssertNoMatch(swiftLib, [.anySequence, "-Xcc", "-std=c++1z", .anySequence])
}
}

func testSwiftCMixed() throws {
Expand Down Expand Up @@ -1894,6 +1917,88 @@ final class BuildPlanTests: XCTestCase {
])
}

func testSwiftSettings_interoperabilityMode_cxx() throws {
let Pkg: AbsolutePath = "/Pkg"

let fs: FileSystem = InMemoryFileSystem(
emptyFiles:
Pkg.appending(components: "Sources", "cxxLib", "lib.cpp").pathString,
Pkg.appending(components: "Sources", "cxxLib", "include", "lib.h").pathString,
Pkg.appending(components: "Sources", "swiftLib", "lib.swift").pathString,
Pkg.appending(components: "Sources", "swiftLib2", "lib2.swift").pathString
)

let observability = ObservabilitySystem.makeForTesting()
let graph = try loadModulesGraph(
fileSystem: fs,
manifests: [
Manifest.createRootManifest(
displayName: "Pkg",
path: .init(validating: Pkg.pathString),
cxxLanguageStandard: "c++20",
targets: [
TargetDescription(name: "cxxLib", dependencies: []),
TargetDescription(
name: "swiftLib",
dependencies: ["cxxLib"],
settings: [.init(tool: .swift, kind: .interoperabilityMode(.Cxx))]
),
TargetDescription(name: "swiftLib2", dependencies: ["swiftLib"]),
]
),
],
observabilityScope: observability.topScope
)
XCTAssertNoDiagnostics(observability.diagnostics)

let plan = try mockBuildPlan(
graph: graph,
fileSystem: fs,
observabilityScope: observability.topScope
)
let result = try BuildPlanResult(plan: plan)

// Cxx module
do {
try XCTAssertMatch(
result.target(for: "cxxLib").clangTarget().symbolGraphExtractArguments(),
[.anySequence, "-cxx-interoperability-mode=default", "-Xcc", "-std=c++20", .anySequence]
)
}

// Swift module directly importing cxx module
do {
try XCTAssertMatch(
result.target(for: "swiftLib").swiftTarget().compileArguments(),
[.anySequence, "-cxx-interoperability-mode=default", "-Xcc", "-std=c++20", .anySequence]
)
try XCTAssertMatch(
result.target(for: "swiftLib").swiftTarget().symbolGraphExtractArguments(),
[.anySequence, "-cxx-interoperability-mode=default", "-Xcc", "-std=c++20", .anySequence]
)
}

// Swift module transitively importing cxx module
do {
try XCTAssertNoMatch(
result.target(for: "swiftLib2").swiftTarget().compileArguments(),
[.anySequence, "-cxx-interoperability-mode=default", .anySequence]
)
try XCTAssertNoMatch(
result.target(for: "swiftLib2").swiftTarget().compileArguments(),
[.anySequence, "-Xcc", "-std=c++20", .anySequence]
)
try XCTAssertNoMatch(
result.target(for: "swiftLib2").swiftTarget().symbolGraphExtractArguments(),
[.anySequence, "-cxx-interoperability-mode=default", .anySequence]
)
try XCTAssertNoMatch(
result.target(for: "swiftLib2").swiftTarget().symbolGraphExtractArguments(),
[.anySequence, "-Xcc", "-std=c++20", .anySequence]
)
}
}

func testREPLArguments() throws {
let Dep = AbsolutePath("/Dep")
let fs = InMemoryFileSystem(
Expand Down