Skip to content

Fix breakage on Windows, which doesn't have ProcessResult.ExitStatus.signalled. #4299

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 1 commit into from
Apr 16, 2022

Conversation

abertelrud
Copy link
Contributor

This fixes a build break on Windows from #4290.

@abertelrud
Copy link
Contributor Author

@swift-ci please smoke test

@abertelrud
Copy link
Contributor Author

abertelrud commented Apr 15, 2022

Confirmed by code inspection that's the only use of signalled in the original commit.

@abertelrud
Copy link
Contributor Author

For the future, it's also a bit suboptimal to have such platform specific APIs in Process, where each client has to conditionalize. Maybe we can find a way of abstracting these concepts, even if not all kinds of results are applicable to all platforms. I'm also thinking of cases such as maybe wanting to serialize this information, e.g. a unit test result that can end in an .abnormal condition on Windows but where it makes sense to reason about that in a cross-platform way.

@compnerd
Copy link
Member

compnerd commented Apr 15, 2022

For the future, it's also a bit suboptimal to have such platform specific APIs in Process, where each client has to conditionalize. Maybe we can find a way of abstracting these concepts, even if not all kinds of results are applicable to all platforms. I'm also thinking of cases such as maybe wanting to serialize this information, e.g. a unit test result that can end in an .abnormal condition on Windows but where it makes sense to reason about that in a cross-platform way.

That would be nice. I don't see how to unify that path immediately. The concepts are different (there is no signal equivalent on Windows, and an abnormal exit is a crash really - on unix, the signal may be a SIGTERM, SIGINT, SIGQUIT, SIGHUP none of which are a crash).

@abertelrud
Copy link
Contributor Author

For the future, it's also a bit suboptimal to have such platform specific APIs in Process, where each client has to conditionalize. Maybe we can find a way of abstracting these concepts, even if not all kinds of results are applicable to all platforms. I'm also thinking of cases such as maybe wanting to serialize this information, e.g. a unit test result that can end in an .abnormal condition on Windows but where it makes sense to reason about that in a cross-platform way.

That would be nice. I don't see how to unify that path immediately. The concepts are different (there is no signal equivalent on Windows, and an abnormal exit is a crash really - on unix, the signal may be a SIGTERM, SIGINT, SIGQUIT, SIGHUP none of which are a crash).

I was actually thinking that the union of all possible states could be represented on all platforms. If it was Codable, then for example a unit test failure from a signal coded on a Unix box could be decoded on a Windows box with the concept of a Unix signal kept intact.

@abertelrud
Copy link
Contributor Author

Meanwhile, I'm preparing a cross-repo test run with a DNM branch in the Swift repo. If we must revert quickly we can do that, then I would convert this to putting it back with the fix.

@abertelrud
Copy link
Contributor Author

swiftlang/swift#42405

@abertelrud abertelrud force-pushed the eng/fix-windows-breakage branch from 2cbec2d to 098341e Compare April 16, 2022 00:36
@abertelrud
Copy link
Contributor Author

@swift-ci please smoke test

@abertelrud abertelrud force-pushed the eng/fix-windows-breakage branch from 098341e to 29ee652 Compare April 16, 2022 02:01
@abertelrud
Copy link
Contributor Author

@swift-ci please smoke test

@abertelrud
Copy link
Contributor Author

Windows toolchain build at swiftlang/swift#42405 succeeded, @compnerd OK to merge?

@abertelrud abertelrud merged commit f9f74f1 into swiftlang:main Apr 16, 2022
@abertelrud abertelrud deleted the eng/fix-windows-breakage branch April 16, 2022 03:53
abertelrud added a commit to abertelrud/swift-package-manager that referenced this pull request Apr 19, 2022
abertelrud added a commit that referenced this pull request Apr 19, 2022
… improve persistence of cached compiler outputs (#4290 & #4299) (#4309)

* Clean up the code and some of the API of compiling plugins, and improve persistence of cached compiler outputs (#4290)

Clients now get the command line and compiler output even in the case of failure, and also when the plugin doesn't have to be recompiled because nothing changed (previously this information was lost).  This is done by serializing a proper struct to JSON rather than just storing the hexadecimal representation of the configuration hash.

Callers also have control over the name of the temporary directory to represent the plugin, rather than always having it be the last path component of the sources directory.

(cherry picked from commit 09d77a7)

* Fix breakage on Windows, which doesn't have `ProcessResult.ExitStatus.signalled`. (#4299)

(cherry picked from commit f9f74f1)
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