Skip to content

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

Merged
merged 1 commit into from
Jan 10, 2017
Merged

Enable SourceKit building by default on Linux #5903

merged 1 commit into from
Jan 10, 2017

Conversation

alblue
Copy link
Contributor

@alblue alblue commented Nov 22, 2016

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:

  • Refactor out SourceKit to its own repository, and interleave the build such that SourceKit -> libdispatch -> Swift
  • Add a dependency between SourceKit and the libdispatch library

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.

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
Copy link
Contributor

@dgrove-oss dgrove-oss Nov 22, 2016

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.

@alblue
Copy link
Contributor Author

alblue commented Nov 22, 2016

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
@alblue
Copy link
Contributor Author

alblue commented Dec 6, 2016

@nkcsgexi or @akyrtzi, do you have any suggestions on how to make source kit build by default on Linux? Is the above PR the right direction, or should we be looking at something else?

@akyrtzi
Copy link
Contributor

akyrtzi commented Dec 6, 2016

How about removing need for libdispatch on linux by having a pthread or std::thread implementation of Concurrency-libdispatch.cpp ?

@alblue
Copy link
Contributor Author

alblue commented Dec 7, 2016

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.

@akyrtzi
Copy link
Contributor

akyrtzi commented Dec 7, 2016

Ok, this is fair. From the two options I would find "SourceKit to its own repository" rather undesirable.
To clarify, is this pull request implementing "Add a dependency between SourceKit and the libdispatch library" ? Is there more work needed ?

@jpsim
Copy link
Contributor

jpsim commented Dec 12, 2016

To clarify, is this pull request implementing "Add a dependency between SourceKit and the libdispatch library" ? Is there more work needed ?

The dependency already exists. This PR makes utils/build-script build libsourcekitdInProc.so on Linux.

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.

@alblue
Copy link
Contributor Author

alblue commented Dec 22, 2016

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.

@slavapestov
Copy link
Contributor

@swift-ci Please smoke test

@seabaylea
Copy link
Contributor

@akyrtzi @slavapestov do we think this is ready to merge?

@slavapestov
Copy link
Contributor

@swift-ci Please smoke test

@slavapestov
Copy link
Contributor

@shahmishal Do you know what's going on with this PR?

@slavapestov
Copy link
Contributor

@akyrtzi Can we merge this?

@slavapestov slavapestov self-assigned this Jan 10, 2017
@akyrtzi
Copy link
Contributor

akyrtzi commented Jan 10, 2017

@swift-ci smoke test

@aschwaighofer
Copy link
Contributor

This appears to have broke the linux 14_04 bot.

https://ci.swift.org/job/oss-swift-incremental-RA-linux-ubuntu-14_04/517/

@aschwaighofer
Copy link
Contributor

I will speculatively revert.

@alblue
Copy link
Contributor Author

alblue commented Jan 12, 2017

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.

@alblue
Copy link
Contributor Author

alblue commented Jan 14, 2017

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.

@alblue
Copy link
Contributor Author

alblue commented Jan 17, 2017

Resubmitted change as pull #6858

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.

7 participants