-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Enable SourceKit building by default on Linux #5903
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
Conversation
OUTPUT "${SWIFT_PATH_TO_LIBDISPATCH_BUILD}/src/.libs/libdispatch.so" | ||
COMMAND /bin/sh ${SWIFT_PATH_TO_LIBDISPATCH_SOURCE}/autogen.sh | ||
COMMAND /bin/sh ${SWIFT_PATH_TO_LIBDISPATCH_SOURCE}/configure | ||
COMMAND /usr/bin/make |
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.
Just want to make sure that there is no way the libdispatch.so you are building here is going to get mixed into the "real" build of libdispatch.so that probably happens later. The configure command is missing the --with-swift-toolchain --prefix and --with-build-variant arguments that are supplied when it is build from swift/utils/build-script-impl. Is also missing the setting of CC and CXX environment variables to the clang that is being built as part of building swift.
The "make distclean" at the end should ensure that when the real libdispatch is built there are no problems with the overlap. Not sure what other variables need to be passed through or if there is a better way to do this though. |
The Linux build has a dependency on the libdispatch library, which is needed by the various native libraries for sourcekitd. On macOS, the dependency for libdispatch is satisfied directly through the base OS, but on Linux no such dependency exists. Modify this so that if the SourceKit library is built, and the libdispatch library is already present, then we shell out to make the libdispatch binary project when the SourceKit is built. Issue: SR-1676
How about removing need for libdispatch on linux by having a pthread or std::thread implementation of |
The preferred approach for concurrency is using libdispatch on Linux; given that we already have it, it seems overkill to re-implement libdispatch than fix the build order. |
Ok, this is fair. From the two options I would find "SourceKit to its own repository" rather undesirable. |
The dependency already exists. This PR makes I think this PR is the least-bad way to build SourceKit on Linux, and agree with all of @alblue's comments on the different options to build SourceKit and how they're all considerably less desirable and/or more work, despite being more "correct" in terms of CMake usage. There is one more option, which I'm including for the sake of completeness rather than think it's a preferable alternative, and that's to build libdispatch's Swift overlay at a later phase, removing its dependency on Swift, which would let it be built before Swift, and therefore be available when SourceKit is built. |
Can someone kick off the tests for this because they seem to be stuck. @akyrtzi This change just adds an explicit dependency in the build script such that libdispatch.so is built as a pre-requisite for sourcekit's use. While there could be improvements (such as passing CC flags through to the nested make module) I'm not sure what the right way to achieve that would be. Other solutions to this problem almost certainly require reorganising the repository for sourcekit, libdispatch, or both. |
@swift-ci Please smoke test |
@akyrtzi @slavapestov do we think this is ready to merge? |
@swift-ci Please smoke test |
@shahmishal Do you know what's going on with this PR? |
@akyrtzi Can we merge this? |
@swift-ci smoke test |
This appears to have broke the linux 14_04 bot. https://ci.swift.org/job/oss-swift-incremental-RA-linux-ubuntu-14_04/517/ |
I will speculatively revert. |
It looks like there was an issue with the 14.04 build - it picked up a local gcc instead of clang. Interestingly the 16.04 build didn't have that problem. I'll look at what's going on with the build on 14.04 and provide an updated patch. |
I've resubmitted this change in pull #6807 - can someone kick off the CI build on that one? I don't know if the CI build kicks off the build on the 14.04 box but we need to verify this fixes the issue seen earlier. |
Resubmitted change as pull #6858 |
SourceKit on Linux isn't built by default, although it can be built in a second pass. That's because the main Swift repository contains SourceKit, but on non-Darwin platforms, libdispatch is a separate project which is built separately.
As a result, libdispatch needs to be built before SourceKit can be built on Linux. This suggests two mechanisms could be used:
The key problem with adding a dependency in the normal way is that libdispatch is generated with makefiles and autoconf, while the Swift project is generated with CMake. An alternative is to set up a dummy target, which is satisfied if the libdispatch project output exists, and if not to shell explicitly out to a
/bin/make
call to trigger the creation of the native libdispatch code. Although inelegant, it works and the SourceKit library is now built in a single pass.Resolves SR-1676.