Skip to content
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

Ensure plug-in C modules don't mix with target ones. #8083

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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
13 changes: 13 additions & 0 deletions Sources/Build/BuildDescription/ModuleBuildDescription.swift
Original file line number Diff line number Diff line change
Expand Up @@ -192,4 +192,17 @@ extension ModuleBuildDescription {
}
return dependencies
}

package func recursiveLinkDependencies(using plan: BuildPlan) -> [Dependency] {
var dependencies: [Dependency] = []
plan.traverseDependencies(of: self) {
// Filter out plugin dependencies
$0.module?.type != .plugin
} onProduct: { product, _, description in
dependencies.append(.product(product, description))
} onModule: { module, _, description in
dependencies.append(.module(module, description))
}
return dependencies
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1018,4 +1018,10 @@ extension SwiftModuleBuildDescription {
) -> [ModuleBuildDescription.Dependency] {
ModuleBuildDescription.swift(self).recursiveDependencies(using: plan)
}

package func recursiveLinkDependencies(
using plan: BuildPlan
) -> [ModuleBuildDescription.Dependency] {
ModuleBuildDescription.swift(self).recursiveLinkDependencies(using: plan)
}
}
3 changes: 2 additions & 1 deletion Sources/Build/BuildPlan/BuildPlan+Swift.swift
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,13 @@ extension BuildPlan {
func plan(swiftTarget: SwiftModuleBuildDescription) throws {
// We need to iterate recursive dependencies because Swift compiler needs to see all the targets a target
// depends on.
for case .module(let dependency, let description) in swiftTarget.recursiveDependencies(using: self) {
for case .module(let dependency, let description) in swiftTarget.recursiveLinkDependencies(using: self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think instead of a special method that is filtering out plugins what you want to do is check whether the dependency targets the same destination and if not skip it, that would filter out plugins/macros and other things for "target" destination.

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 did that originally but felt that was hiding the real issue of including plug-in dependencies in my list of recursive dependencies when I don't want them when constructing the command line arguments for my module build.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a optional destination as a parameter to recursive dependencies method then, it would be easier to understand than having to navigate multiple slightly different methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe it would be okay to adjust recursiveDependencies to always respect destination of the requesting module I don't think we ever want to have things from different destinations used anyway, it's not only plugins though it's also applicable to macros.

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've been hesitant to rely on destinations. Could a plug-in tool depend on another plug-in tool? Wouldn't both be host?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, both would be host, just like macros. There are other places in Build which filter based on the destination so it won’t be unique.

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, note that plugins actually have a special description not SwiftModuleBuildDescription.

switch dependency.underlying {
case let underlyingTarget as ClangModule where underlyingTarget.type == .library:
guard case let .clang(target)? = description else {
throw InternalError("unexpected clang target \(underlyingTarget)")
}

// Add the path to modulemap of the dependency. Currently we require that all Clang targets have a
// modulemap but we may want to remove that requirement since it is valid for a target to exist without
// one. However, in that case it will not be importable in Swift targets. We may want to emit a warning
Expand Down
2 changes: 2 additions & 0 deletions Sources/Build/BuildPlan/BuildPlan.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1142,6 +1142,7 @@ extension BuildPlan {

package func traverseDependencies(
of description: ModuleBuildDescription,
filter: (ResolvedModule.Dependency) -> Bool = { _ in true },
onProduct: (ResolvedProduct, BuildParameters.Destination, ProductBuildDescription?) -> Void,
onModule: (ResolvedModule, BuildParameters.Destination, ModuleBuildDescription?) -> Void
) {
Expand All @@ -1163,6 +1164,7 @@ extension BuildPlan {
) -> [TraversalNode] {
module
.dependencies(satisfying: description.buildParameters.buildEnvironment)
.filter({ filter($0) })
.reduce(into: [TraversalNode]()) { partial, dependency in
switch dependency {
case .product(let product, _):
Expand Down
64 changes: 64 additions & 0 deletions Tests/BuildTests/BuildPlanTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6736,4 +6736,68 @@ final class BuildPlanTests: XCTestCase {
)
}
}

func testHostTargetDontCross() async throws {
dschaefer2 marked this conversation as resolved.
Show resolved Hide resolved
let fs = InMemoryFileSystem(emptyFiles: [
"/Pkg/Plugins/CCrossingStreams/plugin.swift",
"/Pkg/Sources/CLibrary/lib.c",
"/Pkg/Sources/CLibrary/include/lib.h",
"/Pkg/Sources/Exe/main.swift",
"/Pkg/Sources/Tool/main.swift",
])

let observability = ObservabilitySystem.makeForTesting()

let graph = try loadModulesGraph(
fileSystem: fs,
manifests: [
.createRootManifest(
displayName: "Pkg",
path: "/Pkg",
targets: [
.init(name: "CLibrary"),
.init(
name: "Tool",
dependencies: [
"CLibrary"
],
type: .executable
),
.init(
name: "CCrossingStreams",
dependencies: [
.target(name: "Tool"),
],
type: .plugin,
pluginCapability: .buildTool
),
.init(
name: "Exe",
dependencies: [
.target(name: "CLibrary"),
],
type: .executable,
pluginUsages: [
.plugin(name: "CCrossingStreams", package: nil),
]
)
]
)
],
observabilityScope: observability.topScope
)

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

let exe = try result.moduleBuildDescription(for: "Exe").swift()
// Ensure the exe doesn't have the tool version of the CLibrary modulemap in it's flags
XCTAssertFalse(exe.additionalFlags.contains(where: { $0.hasSuffix("CLibrary-tool.build/module.modulemap")}))
XCTAssertTrue(exe.additionalFlags.contains(where: { $0.hasSuffix("CLibrary.build/module.modulemap")}))
// Also ensure the include path isn't there twice
XCTAssertEqual(exe.additionalFlags.filter({ $0 == "/Pkg/Sources/CLibrary/include" }).count, 1)
}
}