SR-1005: [Foundation] Replace posix_spawn by NSTask #303
Conversation
|
@swift-ci Please test |
Bouke
changed the title
|
The failing OS X build doesn't appear to be related. Regarding the failing Linux build, this PR is dependent on a change in Foundation (PR). |
|
When testing this on Linux, most build-related tests fail. What I've found so far is that BEFORE: AFTER: |
NSTask does not run inside a shell, so no automatic PATH lookups are performed for binaries. Also referencing binaries by an absolute path is not desirable, so for relative paths a shell is opened and the absolute path is looked up through `which`.
No memoization has a performance penalty of ~6%.
Previous implementation used a temporary file, however NSTask doesn't allow child process to access parent's file descriptors. So stdout is used to communicate the toml output. See also: http://www.cocoabuilder.com/archive/cocoa/292758-sharing-file-descriptors-to-an-nstask.html
|
I've rebased the PR and tested against OSX and Ubuntu. Please retest. |
|
|
||
| var environment = environment | ||
| #if Xcode | ||
| let keys = ["SWIFT_EXEC", "HOME", "PATH"] |
There was a problem hiding this comment.
This lost the change to add: "TOOLCHAINS", here that I made in the past couple of days.
There was a problem hiding this comment.
Yes I missed that, I've restored those changes.
|
|
||
| private var memoized = [String: String]() | ||
| private let PATH = [POSIX.getcwd()] + (getenv("PATH") ?? "").components(separatedBy: ":") |
There was a problem hiding this comment.
We shouldn't default to looking in the cwd, that can be a security issue.
|
I'm closing this out... per my email to swift-build-dev I'm not longer convinced we should try to move all of our infrastructure onto the Foundation APIs. I think instead we should do it on a case by case basis, and in this case (posix_spawn) my feeling is that our needs match using the POSIX API directly, and I am fine with us needing to maintain the infrastructure for that (at least until there is a standard POSIX module for Swift itself). I think some of the cleanups on this branch make sense, but it didn't look like I could easily cherry pick them out. |
Currently this cannot be merged as builds fail on Linux;
NSTask.standardOutput), so no output can be captured (PR).NSFileHandle.availableData) (PR).argv[0]and stderr [NSTask] Change argv[0] to full path + fixes combining stdout+stderr swift-corelibs-foundation#353