Skip to content
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.

Sign up for GitHub

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

Introduce a flag to control the path to Swift headers installation. #766

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Introduce a flag to control the path to Swift headers installation. #766

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,12 @@ endif()
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")
Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

Suggested change
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")

Copy link
Author

Choose a reason for hiding this comment

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

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.

if(ENABLE_SWIFT)
enable_language(Swift)
if(NOT SWIFT_RUNTIME_RESOURCE_INSTALL_DIR)
set(SWIFT_RUNTIME_RESOURCE_INSTALL_DIR "${CMAKE_INSTALL_LIBDIR}/swift$<$<NOT:$<BOOL:${BUILD_SHARED_LIBS}>>:_static>")
endif()
endif()

option(ENABLE_THREAD_LOCAL_STORAGE "enable usage of thread local storage via _Thread_local" ON)
Expand Down Expand Up @@ -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")
Copy link

Choose a reason for hiding this comment

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

We could use SWIFT_RUNTIME_RESOURCE_INSTALL_DIR in this line as well

Suggested change
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")

Copy link
Author

Choose a reason for hiding this comment

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

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.

set(INSTALL_DISPATCH_HEADERS_DIR "${CMAKE_INSTALL_LIBDIR}/swift$<$<NOT:$<BOOL:${BUILD_SHARED_LIBS}>>:_static>/dispatch" CACHE PATH "Path where the headers will be installed for libdispatch")
set(INSTALL_BLOCK_HEADERS_DIR "${CMAKE_INSTALL_LIBDIR}/swift$<$<NOT:$<BOOL:${BUILD_SHARED_LIBS}>>:_static>/Block" CACHE PATH "Path where the headers will be installed for the blocks runtime")
set(INSTALL_OS_HEADERS_DIR "${CMAKE_INSTALL_LIBDIR}/swift$<$<NOT:$<BOOL:${BUILD_SHARED_LIBS}>>:_static>/os" CACHE PATH "Path where the os/ headers will be installed")
set(INSTALL_DISPATCH_HEADERS_DIR "${SWIFT_RUNTIME_RESOURCE_INSTALL_DIR}/dispatch" CACHE PATH "Path where the headers will be installed for libdispatch")
set(INSTALL_BLOCK_HEADERS_DIR "${SWIFT_RUNTIME_RESOURCE_INSTALL_DIR}/Block" CACHE PATH "Path where the headers will be installed for the blocks runtime")
set(INSTALL_OS_HEADERS_DIR "${SWIFT_RUNTIME_RESOURCE_INSTALL_DIR}/os" CACHE PATH "Path where the os/ headers will be installed")
else()
set(INSTALL_TARGET_DIR "${CMAKE_INSTALL_LIBDIR}" CACHE PATH "Path where the libraries will be installed")
set(INSTALL_DISPATCH_HEADERS_DIR "include/dispatch" CACHE PATH "Path where the headers will be installed")
Expand Down