Introduce a flag to control the path to Swift headers installation.#766
Introduce a flag to control the path to Swift headers installation.#766stevapple wants to merge 1 commit intoswiftlang:mainfrom
Conversation
a new flag to control the path to Swift headers installation.
edymtt
left a comment
edymtt
left a comment
There was a problem hiding this comment.
The change looks reasonable to me -- I would have some feedback to make this change more readable and close to where this is used.
| @@ -289,9 +293,9 @@ add_compile_definitions($<$<OR:$<COMPILE_LANGUAGE:C>,$<COMPILE_LANGUAGE:CXX>>:HA | |||
|
|
|||
| if(ENABLE_SWIFT) | |||
| set(INSTALL_TARGET_DIR "${CMAKE_INSTALL_LIBDIR}/swift$<$<NOT:$<BOOL:${BUILD_SHARED_LIBS}>>:_static>/$<LOWER_CASE:${CMAKE_SYSTEM_NAME}>" CACHE PATH "Path where the libraries will be installed") | |||
There was a problem hiding this comment.
We could use SWIFT_RUNTIME_RESOURCE_INSTALL_DIR in this line as well
| set(INSTALL_TARGET_DIR "${CMAKE_INSTALL_LIBDIR}/swift$<$<NOT:$<BOOL:${BUILD_SHARED_LIBS}>>:_static>/$<LOWER_CASE:${CMAKE_SYSTEM_NAME}>" CACHE PATH "Path where the libraries will be installed") | |
| set(INSTALL_TARGET_DIR "${SWIFT_RUNTIME_RESOURCE_INSTALL_DIR}/$<LOWER_CASE:${CMAKE_SYSTEM_NAME}>" CACHE PATH "Path where the libraries will be installed") |
There was a problem hiding this comment.
This is intentional — the change proposes to diverge INSTALL_TARGET_DIR from INSTALL_*_HEADERS_DIR, which falls separately into SDK and toolchain part. INSTALL_TARGET_DIR should always be where it is now.
| option(INSTALL_PRIVATE_HEADERS "installs private headers in the same location as the public ones" OFF) | ||
|
|
||
| option(ENABLE_SWIFT "enable libdispatch swift overlay" OFF) | ||
| set(SWIFT_RUNTIME_RESOURCE_INSTALL_DIR "" CACHE PATH "path to install swift compiler runtime resources") |
There was a problem hiding this comment.
What do you think about folding the initialization in line 130-132 into the set invocation, and moving such invocation above line 295?
This way the declaration is still protected by the ENABLE_SWIFT condition, and it will be near its usage.
There was a problem hiding this comment.
| set(SWIFT_RUNTIME_RESOURCE_INSTALL_DIR "" CACHE PATH "path to install swift compiler runtime resources") | |
| set(SWIFT_RUNTIME_RESOURCE_INSTALL_DIR "${CMAKE_INSTALL_LIBDIR}/swift$<$<NOT:$<BOOL:${BUILD_SHARED_LIBS}>>:_static>" CACHE PATH "path to install swift compiler runtime resources") |
There was a problem hiding this comment.
Maybe we can replace L130-132 with the line? I was hesitant about placing it into L295 because the install paths set there are identical with/without ENABLE_SWIFT, and SWIFT_RUNTIME_RESOURCE_INSTALL_DIR is just used to compute those real INSTALL_DIRs, which I think should be defined beforehand.
|
Hey @stevapple. I just want to understand what you are trying to do here. A few questions:
|
|
Hello @gottesmm, I’ll try to elaborate on what this pitch is going to solve: When we build a Swift toolchain, a typical build doesn’t split the SDK off the toolchain, so the old Dispatch build script simply installs both the libraries and headers into the same This causes some troubles for the Windows build. The Dispatch headers installed in SDK cannot be directly used by the compiler, so we now collect them from Such workaround may cause some other problem. Since it changed the SDK/toolchain layout, the toolchain tests may not correctly reflect the actual case for users. Layout changes have already cause some unexpected regressions with other compiler resources, and I’m not sure if we will meet it in Dispatch. As for answering your questions:
Yes, and I believe that’s the real design. Headers and libraries are separate parts that happens to have the same install dir suffix.
With the current implementation where we don’t correctly separate toolchain and SDK, there‘s larger possibility we make such mistake because we’re installing everything into swift install dir.
Of course yes, but this will diverge build and install layout, which may cause regressions that can’t be detected with tests.
Actually that’s the reverse side — by this fix we’ll be able to hide the unintentionally public Dispatch API on Windows. |
3 participants
The Swift compiler regards
libdispatchheaders as runtime resources, located relative to the compiler executable. If we want to distribute a standalone SDK, library and header install path should diverge.This PR introduces a flag to manually specify the path to Swift runtime resource directory, which is used to install headers for Swift. Notice that the flag is only effective under
ENABLE_SWIFT, and the current behavior is preserved unless the flag is provided.