[CMake] Updates to allow inclusion using FetchContent #2173
Conversation
Various updates that allow swift-syntax to be included using FetchContent. Mostly this is just not replacing `target_link_libraries` since that is a global replacement, but we also now use a couple of variables from the swift if they're set (eg. `SWIFT_HOST_LIBRARIES_DEST_DIR` and `SWIFT_HOST_MODULE_TRIPLE`).
This was needed for rewriting RUNPATH. swift now build syntax libraries via 'FecthcContent', so RUNPATH are not rewritten.
And remove SwiftSyntaxTargets.cmake because it was just a duplication.
rintaro
force-pushed
the
fetch-content
branch
from
a2ef820 to
1b30b77
Compare
CMakeLists.txt
Outdated
| if(SWIFT_HOST_RUNTIME_DEST_DIR) | ||
| set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "${SWIFT_HOST_RUNTIME_DEST_DIR}") | ||
| else() |
There was a problem hiding this comment.
Thoughts on doing something like this?
if(NOT CMAKE_RUNTIME_OUTPUT_DIRECTORY)
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
endif()This would allow us to control the behaviour from the FetchContent with the standard CMake options.
There was a problem hiding this comment.
Ooo that's a very good point! That would be much nicer, assuming it works.
| elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64|ARM64|arm64") | ||
| set(SWIFT_MODULE_TRIPLE "aarch64-unknown-windows-msvc") | ||
| if("${SWIFT_HOST_MODULE_TRIPLE}" STREQUAL "") | ||
| # FIXME: This is a hack. It's all a hack. Windows isn't setting |
There was a problem hiding this comment.
Not sure I follow this. This is meant to be a user controlled option, what were you expecting? Or is this is a bug CMake that we should also try to get fixed?
There was a problem hiding this comment.
The comment is just what was already there, I don't think this is a CMake bug. For non-Windows it's all being set via build-script.
There was a problem hiding this comment.
Just to expand on this... The reason we need the module triple at all is to output the module in the subfolder format rather than the flat format. Ideally we'll get that fixed at some point in CMake itself 😅. CMAKE_Swift_COMPILER_TARGET is being set in build-script, but I assume that if this was added, it wasn't set for Windows somewhere (in PR testing?). SWIFT_HOST_MODULE_TRIPLE is being passed from the Swift build, so this path would only be taken in the standalone. Not ideal, but also not really anything to do with the fetchcontent change itself.
| function(target_link_swift_syntax_libraries TARGET) | ||
| cmake_parse_arguments(ARGS "" "" "PUBLIC" ${ARGN}) | ||
|
|
||
| target_link_libraries(${TARGET} PUBLIC ${ARGS_PUBLIC}) |
There was a problem hiding this comment.
Should PUBLIC not be conditional on ${ARGS_PUBLIC}?
There was a problem hiding this comment.
It was a hack. It used to parse dependencies as PUBLIC multi-value arguments.
I updated it because it's was certainly confusing.
The argument list handling was a hack and confusing.
So don't use 'SWIFT_HOST_TRIPLE'
|
Thanks! This should make our lives much simpler :) |
[CMake] Updates to allow inclusion using FetchContent (cherry picked from commit e0e5ad8) Conflicts: Sources/SwiftDiagnostics/CMakeLists.txt Sources/SwiftIDEUtils/CMakeLists.txt cmake/modules/AddSwiftHostLibrary.cmake
[CMake] Updates to allow inclusion using FetchContent (cherry picked from commit e0e5ad8) Conflicts: Sources/SwiftDiagnostics/CMakeLists.txt Sources/SwiftIDEUtils/CMakeLists.txt cmake/modules/AddSwiftHostLibrary.cmake
[CMake] Updates to allow inclusion using FetchContent (cherry picked from commit e0e5ad8)
Update for swiftlang/swift#68408