Skip to content

Enable Dispatch overlay on Linux #974

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

Closed
wants to merge 1 commit into from

Conversation

seabaylea
Copy link
Contributor

Now that libdispatch is compiling and passing almost all applicable tests on Linux we need to enable its use by Swift code by enabling the Dispatch overlay to be build on Linux. The code changes below are (very) heavily based on a commit (ac6de62) from @slavapestov in November that was previously reverted due to a build break.

This takes the same approach of enabling the Dispatch overlay on Linux as a non-default option, enabled by passing:

-- -build-swift-dispatch-overlay=1

to utils/build-script

The use of MACH_SEND, MACH_RECV and MEMORYPRESSURE are wrapped by #if defined (__APPLE__) in Dispatch.cpp and by #if _runtime(_ObjC) in Dispatch.swift as they're not supported by libdispatch on Linux (and unlikely to be on other non-Darwin platforms). This follows what Slava did previously.

Additionally qos_class_t isn't available on Linux as there's no sys/qos.h, so is replaced with CInt on Linux (as it is in libdispatch).

@gribozavr
Copy link
Contributor

This tries to use the system version of libdispatch in the overlay (which might not be there, or be terribly outdated or something else), not the version that we are building.

@seabaylea
Copy link
Contributor Author

Right - but there's (currently) no integration of the swift-corelibs-libdispatch build into the swift build. Hopefully this would at least allow further integration of Dispatch on Linux whist we work out how to integrate the builds.

@gribozavr
Copy link
Contributor

Sorry, but this will break the build on systems where the is no system libdispatch, or it is outdated.

@seabaylea
Copy link
Contributor Author

On Linux, the addition of the overlay is optional - so won't be broken out of the box.

Obviously I'm happy to make changes though if you have some suggestions on how to add the overlay and avoid breakages.

@lroseblade
Copy link

@seabaylea is away for a couple of weeks so I am following up on his behalf in his absence.

@gribozavr can you tell us what's required in order for this request to be accepted? We're prepared to make whatever changes are necessary. Or will this require a change to the build environment so Dispatch is always built for Linux?

@gribozavr
Copy link
Contributor

I see, so it won't be built by default. It should be fine as a platform for experimentation, but not for production.

One issue that I'm seeing is that on OS X, with this pull request merged, I'm seeing warnings from ninja:

ninja: warning: multiple rules generate lib/swift/macosx/x86_64/Dispatch.apinotesc. builds involving this target will not be correct; continuing anyway [-w dupbuild=warn]

Could you investigate? I don't immediately see what would be the cause of this.

@lroseblade
Copy link

Great! Thanks @gribozavr. Yes, we'll take a look.

@pushkarnk
Copy link
Member

--- ./cmake/modules/AddSwift.cmake.orig 2016-01-21 15:30:43.000000000 +0530
+++ ./cmake/modules/AddSwift.cmake  2016-01-21 15:30:12.000000000 +0530
@@ -815,7 +815,8 @@
           ${SWIFTLIB_SINGLE_SDK} STREQUAL "OSX")
         # HACK: don't build WatchKit API notes for OS X.
       else()
-        if (NOT IS_DIRECTORY "${SWIFT_SOURCE_DIR}/stdlib/public/SDK/${framework_name}")
+        if (NOT IS_DIRECTORY "${SWIFT_SOURCE_DIR}/stdlib/public/SDK/${framework_name}"
+               AND NOT "${framework_name}" STREQUAL "Dispatch" )
           list(APPEND SWIFTLIB_SINGLE_API_NOTES "${framework_name}")
         endif()
       endif()

The warnings are reported due a duplicate rule to build Dispatch.apinotesc. The apinotes compilation happens in the add_swift_library() function defined in AddSwift.cmake. For every module, the related apinotes are picked up. With the "dispatch overlay" changes we see Dispatch.apinotes being picked up for "Darwin" and "Dispatch" - the former is invalid. The above tweak will avoid this problem.

@pushkarnk
Copy link
Member

@gribozavr Could you please review/test the above change?

@jrose-apple
Copy link
Contributor

My understanding was that @parkera and @phausler did not want to go in this direction; they preferred that the corelibs version of dispatch become a mixed-source library (like mixed-source frameworks built with Xcode). I'm sure they can comment better than me, though, and maybe I was misunderstanding.

@parkera
Copy link
Contributor

parkera commented Jan 21, 2016

I first mixed this up with the changes to get the dispatch project into the build script, but @jrose-apple is right - dispatch should not use the overlay on Linux but instead provide all functionality itself. This has to be the long term direction. Instead of building part of dispatch (and other overlay projects) into every application, we provide their functionality as a library.

@parkera
Copy link
Contributor

parkera commented Jan 21, 2016

See also #1030 .

@tkremenek
Copy link
Member

@seabaylea Thanks for this work. It sounds, however, that @parkera advises we put non-Darwin changes to libDispatch in the corelibs. Closing this pull request.

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