Skip to content

Split Swift SDK Overlay #401

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 4 commits into from
Nov 5, 2018
Merged

Conversation

compnerd
Copy link
Member

@compnerd compnerd commented Oct 12, 2018

This splits out the SDK overlay component and libdispatch runtime itself. Doing
so enables the re-use of libdispatch with and without swift and makes the
behaviour similar across Darwin and other platforms.

The goal here is to actually have the same build structure as Darwin on Linux
and Windows. We would have a swiftDispatch library and the core libdispatch
library. The motivation for this is the future work to actually attempt to build
a LSP in swift on the (toolchain) host. We can use the core libdispatch for
SourceKit and then add the swiftDispatch SDK overlay bits for the LSP
components. This avoids having two builds of libdispatch (one with swift support
and one without).

Copy link
Contributor

@gottesmm gottesmm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions.

@@ -2,7 +2,7 @@
include(CMakeParseArguments)

function(add_swift_target target)
set(options LIBRARY)
set(options LIBRARY;SHARED;STATIC)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One concern that I have with the way that the code is written is that the installation code is separate from the add_swift_target. I am worried about that getting out of sync with this code. I am not sure if today it is out of sync... but can you check just in case? In the future IMO add_swift_target should just handle the installation to make sure things stay in sync.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you would like me to verify. I think I would be find with extending this in a follow up to add support for installation of the output as well.

endif()
set(swiftDispatch_OUTPUT_FILE
${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_${library_kind}_LIBRARY_PREFIX}swiftDispatch${CMAKE_${library_kind}_LIBRARY_SUFFIX})
install(FILES
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the reason why I wrote in the review of the other commit that it would be better to just have this be taken care of by add_swift_target. It would ensure that these sorts of things are in sync.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Yes, adding an install option is reasonable.

@ktopley-apple
Copy link
Contributor

@compnerd could you please write a description of what this PR is trying to achieve? Thanks.

@compnerd
Copy link
Member Author

@ktopley-apple - sure. The reason that it was unclear is because this is actually a single commit (it was hidden due to the previous patches being held up).

This splits out the SDK overlay component and libdispatch runtime
itself. Doing so enables the re-use of libdispatch with and without
swift and makes the behaviour similar across Darwin and other platforms.

The goal here is to actually have the same build structure as Darwin on Linux and Windows. We would have a swiftDispatch library and the core libdispatch library. The motivation for this is the future work to actually attempt to build a LSP in swift on the (toolchain) host. We can use the core libdispatch for SourceKit and then add the swiftDispatch SDK overlay bits for the LSP components. This avoids having two builds of libdispatch (one with swift support and one without).

@compnerd
Copy link
Member Author

@swift-ci please test

@compnerd
Copy link
Member Author

CC: @benlangmuir @akyrtzi - this was the PR I was referring to wrt libdispatch SDK overlay splitting for the LSP support.

@compnerd compnerd force-pushed the split-sdk-overlay branch 3 times, most recently from 72ea644 to c1b3c48 Compare October 19, 2018 23:10
@compnerd
Copy link
Member Author

@swift-ci please test

Update the swift support CMake rules from XCTest.  This adds support for
additional features like partial module compilation which improves
incremental builds.  It allows for executable and library builds.
Enhance add_wift_target to support building static libraries.  We would
always generate shared libraries previously.
This splits out the SDK overlay component and libdispatch runtime
itself.  Doing so enables the re-use of libdispatch with and without
swift and makes the behaviour similar across Darwin and other platforms.
@compnerd
Copy link
Member Author

@swift-ci please test

@compnerd
Copy link
Member Author

Please test with following PRs:
swiftlang/swift-corelibs-foundation#1734

@swift-ci please test

@compnerd
Copy link
Member Author

Please test with following PRs:
swiftlang/swift-corelibs-foundation#1734
swiftlang/swift-package-manager#1829

@swift-ci please test

@ktopley-apple ktopley-apple merged commit ca08b5f into swiftlang:master Nov 5, 2018
@compnerd compnerd deleted the split-sdk-overlay branch November 5, 2018 16:49
rokhinip pushed a commit that referenced this pull request Nov 5, 2021
Split Swift SDK Overlay

Signed-off-by: Kim Topley <[email protected]>
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.

3 participants