[SR-1613][SourceKit] Require blocks runtime #2703
Conversation
| @@ -235,7 +238,7 @@ function(_add_variant_link_flags) | |||
| RESULT_VAR_NAME result) | |||
|
|
|||
| if("${LFLAGS_SDK}" STREQUAL "LINUX") | |||
| list(APPEND result "-lpthread" "-ldl") | |||
| list(APPEND result "-lpthread" "-ldl" "-lBlocksRuntime") | |||
There was a problem hiding this comment.
This creates a new dependency for new ports that don't need SourceKit to start bootstrapping; this will also link it into the standard library, which does not need it. Could we limit this to only SourceKit binaries and libraries?
There was a problem hiding this comment.
Yes, I was wondering about this as well. Can do! I'll add these flags to add_sourcekit_library() instead of here.
SourceKit makes heavy use of blocks. In order to port SourceKit to Linux, we either need to rewrite much of it to use function pointers, or we must require a blocks runtime. This commit requires a blocks runtime, but only when SourceKit is being built. Currently, SourceKit is not built on Linux, so this should not affect anyone.
modocache
force-pushed
the
sr-1613-sourcekit-linux-blocks
branch
from
f68ccb8 to
d7e2e1f
Compare
|
@gribozavr Thanks for the tips! I updated the patch to only link a blocks runtime on Linux. @swift-ci please test |
|
I'm not opposed to this patch, but a nicer solution is to use C++ lambdas. C function pointers are falling off the cliff too far. |
|
Hopefully, in the future, we could build blocks runtime and distribute it with swiftRuntime? |
To clarify, blocks are primarily used for the C API and code around it (tools using the C API, implementation of the C API, etc.), otherwise C++ lambdas are used liberally. |
|
@akyrtzi Would you be alright with me merging this change? I think it's in agreement with your comments on SR-1613:
It also gets us a lot closer to SourceKit on Linux, which we need in order to generate lists of XCTest test methods. |
|
Change LGTM, thanks for working on this! |
|
Excellent, thanks @akyrtzi! I'll cc you on future pull requests as well. :) |
|
It's worth pointing out here that there are already two Blocks runtimes in the public repos under the Apple GitHub org. There's the original one in compiler-rt which isn't hooked into the build system at all AFAICT, and there's the one in Foundation which ends up linked into To @rintaro's point, it would be great to eventually remove this dependency on an external Blocks runtime package and instead provide one as a part of the Swift build. |
| @@ -84,6 +84,11 @@ function(add_sourcekit_default_compiler_flags target) | |||
| ANALYZE_CODE_COVERAGE "${analyze_code_coverage}" | |||
| RESULT_VAR_NAME link_flags) | |||
|
|
|||
| if("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux") | |||
There was a problem hiding this comment.
I realize this has been merged already, but should we be considering other non-Darwin platforms here?
There was a problem hiding this comment.
Absolutely. I'm worried about SR-710 getting done in time for Swift 3.0, so I'm reducing scope a bit here. Ideally, I'd like SourceKit to run everywhere. Were it able to do so, we'd probably want to change this to handle more than just Linux.
|
With #3835 (e.g., ClangImporter: enable -fblocks on non-Darwin platforms) appearing as a PR (yet to land) and swift-corelibs-libdispatch #139: SR-2309: embed BlocksRuntime in libdispatch to eliminate external dep… (already landed) would now be a good time to revisit @rintaro and @briancroom's suggestion? Namely to stop depending on an external BlocksRuntime? I point this out because a pre-packed BlocksRuntime does not exist on all distros, as it does on Ubuntu. Also, it would seem that with or without SourceKit in mind this is going to become on issue on a number of distros due to #4203 progressing. Also just to be clear I'm not asking anyone to solve this per-se. I'm trying to get a feel for what the likely hood of such a PR getting reviewed/accepted should I attempt to make one. |
What's in this pull request?
SourceKit makes heavy use of blocks. In order to port SourceKit to Linux, we either need to rewrite much of it to use function pointers, or we must require a blocks runtime. This commit requires a blocks runtime, but only when SourceKit is being built. Currently, SourceKit is not built on Linux, so this should not affect anyone.
You may attempt to build SourceKit on Linux by applying this patch.
Resolved bug number: (SR-1613)
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.