Skip to content

Conversation

kkebo
Copy link
Contributor

@kkebo kkebo commented Mar 18, 2022

This is a hot fix for Windows build broken by #4224. #4231 also needs this change.

Motivation:

#4224 broke Windows build as @compnerd pointed out at #4224 (comment). The global function exec added in #4224 is actually unnecessary for Windows because signal(SIGINT, SIG_IGN) is not compiled for Windows target.

            #if os(Windows)
            // set shutdown handler to terminate sub-processes, etc
            SwiftTool.cancellator = cancellator
            _ = SetConsoleCtrlHandler({ _ in
                // Terminate all processes on receiving an interrupt signal.
                try? SwiftTool.cancellator?.cancel(deadline: .now() + .seconds(30))

                // Reset the handler.
                _ = SetConsoleCtrlHandler(nil, false)

                // Exit as if by signal()
                TerminateProcess(GetCurrentProcess(), 3)

                return true
            }, true)
            #else
            // trap SIGINT to terminate sub-processes, etc
            signal(SIGINT, SIG_IGN)
            ...
            #endif

Modifications:

  • Enclosed exec with #if !(os(Windows)) and #endif
  • Changed comments to be clearer

Result:

Combined with this change, #4224 no longer affects Windows.

@compnerd
Copy link
Member

@swift-ci please smoke test

@compnerd
Copy link
Member

compnerd commented Mar 18, 2022

I think that we should really revert the change - its incomplete - this should have an analogous counterpart to Windows. (Having a separate exec is likely going to cause further conflicts as things evolve ... this should handle the Windows path internally).

@kkebo
Copy link
Contributor Author

kkebo commented Mar 18, 2022

@compnerd

(Having a separate exec is likely going to cause further conflicts as things evolve ... this should handle the Windows path internally).

I initially considered alternatives such as SwiftTool.exec. However, there is a concern that when changes are made to add TSCBasic's exec in the future, the author may forget to use SwiftTool.exec.

In light of that, it might be better to revert and reimplement #3815 in other ways.

this should have an analogous counterpart to Windows.

I think that would be the most ideal too.

@compnerd
Copy link
Member

I initially considered alternatives such as SwiftTool.exec. However, there is a concern that when changes are made to add TSCBasic's exec in the future, the author may forget to use SwiftTool.exec.

I think that we could just bring over exec from TSCBasic. I haven't gotten around to removing the last of the TSCUtility from swift-driver, which would then allow us to focus on removing the uses of TSCBasic. Doing that more immediately is reasonable. We should be able to entirely remove the use of TSCBasic.exec and use the internal exec only.

@kkebo
Copy link
Contributor Author

kkebo commented Mar 18, 2022

TSCBasic provides other types and operators as well, so it's difficult for me to determine if TSCBasic should be removed.

By any chance, could your concern be addressed with code such as the following?

func exec(...) {
    #if !(os(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)
    #endif

    TSCBasic.exec(...)
}

...

public class SwiftTool {
    ...

    // marked internal for testing
    internal init(outputStream: OutputByteStream, options: GlobalOptions) throws {
        ...

        do {
            ...

            #if os(Windows)
            ...
            #else
            ...
            signal(SIGINT, SIG_IGN)
            ...
            #endif

            ...
        } catch {
            ...
        }

        ...
    }

    ...
}

...

@tomerd
Copy link
Contributor

tomerd commented Mar 18, 2022

@swift-ci please smoke test macOS

@compnerd
Copy link
Member

Yeah, I think that would be better than the current version.

@kkebo kkebo force-pushed the fix-windows-build-4224 branch from 26967ce to e857589 Compare March 18, 2022 18:41
@kkebo
Copy link
Contributor Author

kkebo commented Mar 18, 2022

@compnerd I pushed that change.

@tomerd Sorry for the change after you approve. Please could you check it again?

@tomerd
Copy link
Contributor

tomerd commented Mar 18, 2022

@swift-ci smoke test

@tomerd
Copy link
Contributor

tomerd commented Mar 18, 2022

no problem @KKK669 please bring this commit over to the 5.6 PR once merged

@kkebo
Copy link
Contributor Author

kkebo commented Mar 18, 2022

Yes sure. Thank you.

@tomerd
Copy link
Contributor

tomerd commented Mar 18, 2022

@swift-ci smoke test linux

@compnerd
Copy link
Member

@swift-ci please smoke test Linux platform

@tomerd
Copy link
Contributor

tomerd commented Mar 18, 2022

@swift-ci smoke test linux

2 similar comments
@tomerd
Copy link
Contributor

tomerd commented Mar 19, 2022

@swift-ci smoke test linux

@tomerd
Copy link
Contributor

tomerd commented Mar 19, 2022

@swift-ci smoke test linux

@compnerd
Copy link
Member

@swift-ci please smoke test Linux platform

@compnerd compnerd merged commit e29c65d into swiftlang:main Mar 19, 2022
@kkebo kkebo deleted the fix-windows-build-4224 branch March 19, 2022 07:58
tomerd pushed a commit that referenced this pull request Mar 24, 2022
…4235, #4250) (#4231)

* Fix SIGINT handling during `swift run` (#4224)
* Fix SIGINT handling during `swift run`
* Create a safe wrapper of TSCBasic.exec
* add test fixture for handling SIGINT on "swift run" (#4250)

Co-authored-by: tomer doron <tomer@apple.com>

rdar://90574791
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants