Ask the swift compiler for SWIFT_HOST_MODULE_TRIPLE if not supplied #2454
Conversation
| else() | ||
| message(FATAL_ERROR "Unrecognized architecture for Windows host") | ||
| endif() | ||
| else() |
There was a problem hiding this comment.
I wonder if the current if contents is even worth keeping. SWIFT_HOST_MODULE_TRIPLE would be set when this is part of the compiler build. So we'd only be in here if standalone, in which case falling back to the module triple from -print-target-info sounds the most reasonable to me anyway. Thoughts @etcwilde / @compnerd?
(And thanks @dabrahams, this LGTM even if we don't rip out the rest of the if).
There was a problem hiding this comment.
Yeah, something like this should work for all cases, I believe.
set(module_triple_command "${CMAKE_Swift_COMPILER}" -print-target-info)
if(CMAKE_Swift_COMPILER_TARGET)
list(APPEND module_triple_command -target ${CMAKE_Swift_COMPILER_TARGET})
endif()
execute_process(COMMAND ${module_triple_command} OUTPUT_VARIABLE target_info_json)
string(JSON SWIFT_HOST_MODULE_TRIPLE GET "${target_info_json}" "target" "moduleTriple")
There was a problem hiding this comment.
I had the same thought but didn't want to presume.
There was a problem hiding this comment.
@etcwilde You probably know CMake much better than me but your version seems like a lot of code for what I did in two lines. Is there a reason to make it so elaborate?
There was a problem hiding this comment.
The extra bits I added are for cross-compiling. Running -print-target-info alone will only give you the module triple for the triple that the compiler was configured to output to by default. If someone has specified the triple they want to output to, we want to ensure that the module triple reflects that. It's not always set, so we need to ensure that it is before adding it to the command.
There was a problem hiding this comment.
I definitely prefer @etcwilde's approach - cross-compilation is going to be most likely the path we take to get the Windows ARM64 toolchain release setup.
There was a problem hiding this comment.
So are y'all suggesting I replace lines 65-80 with @etcwilde's code?
There was a problem hiding this comment.
I ask because @bnbarham approved these changes as-is, but I can't run the CI or merge myself, and I'd like to move the PR forward. If that means making the edit I just described, please LMK.
There was a problem hiding this comment.
It'd be good to do, I am fine to merge this as is though - it's an improvement regardless. If you'd like to put up another (or you @etcwilde), feel free :). I'll run tests and merge. Thanks @dabrahams!
There was a problem hiding this comment.
I for one encourage @etcwilde to submit the improved PR once this lands.
|
@swift-ci please test |
|
@swift-ci please test macOS platform |
swiftlang/swift-syntax#2454 was merged upstream.
swiftlang/swift-syntax#2454 was merged upstream.
swiftlang/swift-syntax#2454 was merged upstream.
swiftlang/swift-syntax#2454 was merged upstream.
No description provided.