Skip to content

Ask the swift compiler for SWIFT_HOST_MODULE_TRIPLE if not supplied #2454

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

Merged
merged 1 commit into from
Feb 22, 2024
Merged

Ask the swift compiler for SWIFT_HOST_MODULE_TRIPLE if not supplied #2454

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
3 changes: 2 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ if("${SWIFT_HOST_MODULE_TRIPLE}" STREQUAL "")
message(FATAL_ERROR "Unrecognized architecture for Windows host")
endif()
else()
Copy link
Contributor

@bnbarham bnbarham Feb 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member

@etcwilde etcwilde Feb 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the same thought but didn't want to presume.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So are y'all suggesting I replace lines 65-80 with @etcwilde's code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I for one encourage @etcwilde to submit the improved PR once this lands.

message(FATAL_ERROR "Host module triple required")
execute_process(COMMAND ${CMAKE_Swift_COMPILER} -print-target-info OUTPUT_VARIABLE target_info)
string(JSON SWIFT_HOST_MODULE_TRIPLE GET ${target_info} target moduleTriple)
endif()
endif()
message(STATUS "Module triple: ${SWIFT_HOST_MODULE_TRIPLE}")
Expand Down