Skip to content

Allow swift-build to have async entrypoint #7369

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 5 commits into from
Feb 26, 2024
Merged

Allow swift-build to have async entrypoint #7369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
443d884
Allow `swift-build` to have `async` entrypoint
MaxDesiatov Feb 24, 2024
b49a95e
Clean up `main` entrypoint of `swift-package-manager`
MaxDesiatov Feb 24, 2024
bd35a7a
Merge branch 'main' of github.com:apple/swift-package-manager into ma…
MaxDesiatov Feb 25, 2024
6fa3907
Address PR feedback
MaxDesiatov Feb 26, 2024
92f56c7
Clean up comment wording
MaxDesiatov Feb 26, 2024
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: 3 additions & 10 deletions Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,9 +681,7 @@ package.targets.append(contentsOf: [
.testTarget(
name: "FunctionalPerformanceTests",
dependencies: [
"swift-build",
"swift-package",
"swift-test",
"swift-package-manager",
"SPMTestSupport"
]
),
Expand All @@ -695,9 +693,7 @@ if ProcessInfo.processInfo.environment["SWIFTCI_DISABLE_SDK_DEPENDENT_TESTS"] ==
.testTarget(
name: "FunctionalTests",
dependencies: [
"swift-build",
"swift-package",
"swift-test",
"swift-package-manager",
"PackageModel",
"SPMTestSupport"
]
Expand All @@ -713,10 +709,7 @@ if ProcessInfo.processInfo.environment["SWIFTCI_DISABLE_SDK_DEPENDENT_TESTS"] ==
.testTarget(
name: "CommandsTests",
dependencies: [
"swift-build",
"swift-package",
"swift-test",
"swift-run",
"swift-package-manager",
"Basics",
"Build",
"Commands",
Expand Down
4 changes: 2 additions & 2 deletions Sources/Commands/SwiftBuildCommand.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ struct BuildCommandOptions: ParsableArguments {
}

/// swift-build command namespace
public struct SwiftBuildCommand: SwiftCommand {
public struct SwiftBuildCommand: AsyncSwiftCommand {
public static var configuration = CommandConfiguration(
commandName: "build",
_superCommandName: "swift",
Expand All @@ -110,7 +110,7 @@ public struct SwiftBuildCommand: SwiftCommand {
@OptionGroup()
var options: BuildCommandOptions

public func run(_ swiftCommandState: SwiftCommandState) throws {
public func run(_ swiftCommandState: SwiftCommandState) async throws {
if options.shouldPrintBinPath {
return try print(swiftCommandState.productsBuildParameters.buildPath.description)
}
Expand Down
21 changes: 16 additions & 5 deletions Sources/Commands/SwiftRunCommand.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ struct RunCommandOptions: ParsableArguments {
}

/// swift-run command namespace
public struct SwiftRunCommand: SwiftCommand {
public struct SwiftRunCommand: AsyncSwiftCommand {
public static var configuration = CommandConfiguration(
commandName: "run",
_superCommandName: "swift",
Expand All @@ -109,7 +109,7 @@ public struct SwiftRunCommand: SwiftCommand {
return .init(wantsREPLProduct: options.mode == .repl)
}

public func run(_ swiftCommandState: SwiftCommandState) throws {
public func run(_ swiftCommandState: SwiftCommandState) async throws {
if options.shouldBuildTests && options.shouldSkipBuild {
swiftCommandState.observabilityScope.emit(
.mutuallyExclusiveArgumentsError(arguments: ["--build-tests", "--skip-build"])
Expand Down Expand Up @@ -284,9 +284,20 @@ public struct SwiftRunCommand: SwiftCommand {
/// A safe wrapper of TSCBasic.exec.
private func execute(path: String, args: [String]) throws -> Never {
#if !os(Windows)
// On platforms other than Windows, signal(SIGINT, SIG_IGN) is used for handling SIGINT by DispatchSourceSignal,
// but this process is about to be replaced by exec, so SIG_IGN must be returned to default.
signal(SIGINT, SIG_DFL)
// Dispatch will disable almost all asynchronous signals on its worker threads, and this is called from `async`
// context. To correctly `exec` a freshly built binary, we will need to:
// 1. reset the signal masks
for i in 1..<NSIG {
signal(i, SIG_DFL)
}
var sig_set_all = sigset_t()
sigfillset(&sig_set_all)
sigprocmask(SIG_UNBLOCK, &sig_set_all, nil)

// 2. close all file descriptors.
for i in 3..<getdtablesize() {
close(i)
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any open fds managed by SwiftPM at this point? My concern is that this might corrupt the state of underlying libraries like libc and cause other problems for the case when exec fails for some reason. I think closing fds is not a part of this function's responsibilities but should be handled by those fds owners using O_CLOEXEC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weissi WDYT?

Copy link
Contributor Author

@MaxDesiatov MaxDesiatov Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we have any open fds managed by SwiftPM at this point?

I don't think we should, but I don't have any certainty our swift-tools-support-core or swift-corelibs-foundation APIs don't leak anything. I could see the FileHandle API or some reference or escapable type captured somewhere with no explicit close call on it.

Copy link
Member

Choose a reason for hiding this comment

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

I think the discussion point here is whether to prioritize potential state corruption after the exec fails or potential fd leak during the exec'ed program execution. IMO the former issue is more critical than the latter since we don't have a large number of open fd's here and emitting proper error diagnostics when a problem occurs is essential for further investigation.

Copy link
Contributor Author

@MaxDesiatov MaxDesiatov Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

IIUC the argument that Johannes is making is that bash is doing it this way and it works fine for them. Whatever state underlying libraries have, it'll be gone with execv call anyway https://github.com/bminor/bash/blob/f3b6bd19457e260b65d11f2712ec3da56cef463f/execute_cmd.c#L6162C1-L6173

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's what UNIX programs are expected to do if they exec: close the file descriptors that they're not intending to pass on. LGTM

Copy link
Member

Choose a reason for hiding this comment

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

Okay, that makes sense to me 👍

}
#endif

try TSCBasic.exec(path: path, args: args)
Expand Down
11 changes: 6 additions & 5 deletions Sources/SPMTestSupport/SwiftPMProduct.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public enum SwiftPM {

extension SwiftPM {
/// Executable name.
private var executableName: RelativePath {
private var executableName: String {
switch self {
case .Build:
return "swift-build"
Expand All @@ -45,7 +45,7 @@ extension SwiftPM {
}

public var xctestBinaryPath: AbsolutePath {
Self.xctestBinaryPath(for: executableName)
Self.xctestBinaryPath(for: RelativePath("swift-package-manager"))
}

public static func xctestBinaryPath(for executableName: RelativePath) -> AbsolutePath {
Expand Down Expand Up @@ -114,11 +114,12 @@ extension SwiftPM {
#endif
// FIXME: We use this private environment variable hack to be able to
// create special conditions in swift-build for swiftpm tests.
environment["SWIFTPM_TESTS_MODULECACHE"] = xctestBinaryPath.parentDirectory.pathString
environment["SWIFTPM_TESTS_MODULECACHE"] = self.xctestBinaryPath.parentDirectory.pathString

// Unset the internal env variable that allows skipping certain tests.
environment["_SWIFTPM_SKIP_TESTS_LIST"] = nil

environment["SWIFTPM_EXEC_NAME"] = self.executableName

for (key, value) in env ?? [:] {
environment[key] = value
}
Expand Down
5 changes: 4 additions & 1 deletion Sources/swift-build/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@
# See http://swift.org/CONTRIBUTORS.txt for Swift project authors

add_executable(swift-build
main.swift)
Entrypoint.swift)
target_link_libraries(swift-build PRIVATE
Commands)

target_compile_options(swift-build PRIVATE
-parse-as-library)

install(TARGETS swift-build
DESTINATION bin)
7 changes: 6 additions & 1 deletion Sources/swift-run/main.swift → Sources/swift-build/Entrypoint.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,9 @@

import Commands

SwiftRunCommand.main()
@main
struct Entrypoint {
static func main() async {
await SwiftBuildCommand.main()
}
}
20 changes: 17 additions & 3 deletions Sources/swift-package-manager/SwiftPM.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,29 +17,43 @@ import PackageCollectionsCommand
import PackageRegistryCommand

let firstArg = CommandLine.arguments[0]
let execName = (try? AbsolutePath(validating: firstArg).basenameWithoutExt) ??
let baseNameWithoutExtension = (try? AbsolutePath(validating: firstArg).basenameWithoutExt) ??
(try? RelativePath(validating: firstArg).basenameWithoutExt)

@main
struct SwiftPM {
static func main() async {
// Workaround a bug in Swift 5.9, where multiple executables with an `async` main entrypoint can't be linked
// into the same test bundle. We're then linking single `swift-package-manager` binary instead and passing
// executable name via `SWIFTPM_EXEC_NAME`.
if baseNameWithoutExtension == "swift-package-manager" {
await main(execName: EnvironmentVariables.process()["SWIFTPM_EXEC_NAME"])
} else {
await main(execName: baseNameWithoutExtension)
}
}

@discardableResult
private static func main(execName: String?) async -> Bool {
switch execName {
case "swift-package":
await SwiftPackageCommand.main()
case "swift-build":
SwiftBuildCommand.main()
await SwiftBuildCommand.main()
case "swift-experimental-sdk":
await SwiftSDKCommand.main()
case "swift-test":
SwiftTestCommand.main()
case "swift-run":
SwiftRunCommand.main()
await SwiftRunCommand.main()
case "swift-package-collection":
await PackageCollectionsCommand.main()
case "swift-package-registry":
await PackageRegistryCommand.main()
default:
fatalError("swift-package-manager launched with unexpected name: \(execName ?? "(unknown)")")
}

return true
}
}
5 changes: 4 additions & 1 deletion Sources/swift-run/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@
# See http://swift.org/CONTRIBUTORS.txt for Swift project authors

add_executable(swift-run
main.swift)
Entrypoint.swift)
target_link_libraries(swift-run PRIVATE
Commands)

target_compile_options(swift-run PRIVATE
-parse-as-library)

install(TARGETS swift-run
RUNTIME DESTINATION bin)
7 changes: 6 additions & 1 deletion Sources/swift-build/main.swift → Sources/swift-run/Entrypoint.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,9 @@

import Commands

SwiftBuildCommand.main()
@main
struct Entrypoint {
static func main() async {
await SwiftRunCommand.main()
}
}
5 changes: 5 additions & 0 deletions Tests/CommandsTests/RunCommandTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import SPMTestSupport
import XCTest

import class TSCBasic.Process
import enum TSCBasic.ProcessEnv

final class RunCommandTests: CommandsTestCase {
private func execute(
Expand Down Expand Up @@ -121,8 +122,12 @@ final class RunCommandTests: CommandsTestCase {

let sync = DispatchGroup()
let outputHandler = OutputHandler(sync: sync)

var environmentBlock = ProcessEnv.block
environmentBlock["SWIFTPM_EXEC_NAME"] = "swift-run"
let process = Process(
arguments: [SwiftPM.Run.xctestBinaryPath.pathString, "--package-path", fixturePath.pathString],
environmentBlock: environmentBlock,
outputRedirection: .stream(stdout: outputHandler.handle(bytes:), stderr: outputHandler.handle(bytes:))
)

Expand Down