-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
[Concurrency] Fix task-to-thread model linking. #80195
base: main
Are you sure you want to change the base?
[Concurrency] Fix task-to-thread model linking. #80195
Conversation
We need to provide some of the additional functions, as stubs, for task-to-thread model. rdar://141348916
@swift-ci Please smoke test |
Hopefully we can take this instead of #80194. |
@swift-ci Please smoke test macOS platform |
Looks like a new CMakelists.txt got added to the new build system while I was working on this.
@swift-ci Please smoke test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new runtimes side of the changes for the build seem good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New runtime build changes look fine to me. I may come back and conditionalize including the platform-specific executors based on the platform to reduce filesystem traffic.
Just the one question on whether we want to include ExecutorImpl.swift
in the list of sources where the swift_Concurrency
library is declared instead of adding it afterward in each of the executor backend implementations.
DispatchGlobalExecutor.cpp | ||
DispatchExecutor.swift | ||
CFExecutor.swift | ||
ExecutorImpl.swift) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is ExecutorImpl.swift
going to be used for all executors? If so, it might make sense to include the file wholesale on the main add_library(swift_Concurrency)
invocation instead of adding it separately for each executor backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily, no. The Embedded Swift build doesn't use ExecutorImpl.swift
, currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'd like it to, but… there are issues relating to existing users and how they're using it that we need to work out first.)
@swift-ci Please smoke test macOS platform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build system changes look fine to me
macOS PR testing timed-out that time. |
@swift-ci Please smoke test macOS |
Now Windows timed out :-( |
@swift-ci Please smoke test Windows platform |
@swift-ci Please smoke test macOS |
@swift-ci please test Windows platform |
@swift-ci please smoke test macOS platform |
|
I'm going to split out some of the build changes to get the CI unblocked. |
Seems that attempt crashes the compiler - fortunately @glessard did put up a revert for the issue. I've triggered the auto-merge to get CI healthy again. |
We need to provide some of the additional functions, as stubs, for task-to-thread model.
rdar://141348916