-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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.
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
[CMake] Enable CMP0157 NEW if available #76587
base: main
Are you sure you want to change the base?
Conversation
Use new CMake Swift compilation mode if possible. Disable sowe workaround when CMP0157 is enabled.
swiftlang/swift-syntax#2856 |
set(CMP0157_IS_NEW FALSE) | ||
if(POLICY CMP0157) | ||
cmake_policy(SET CMP0157 NEW) | ||
set(CMP0157_IS_NEW TRUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of setting this variable, we should probably use cmake_policy(GET...)
at the locations where we are querying for it so that it can't get out of sync and there's only one source of truth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't use cmake_policy(GET...)
because it would require a temporary variable.
cmake_policy(GET CMP0157 CMP0157_value)
if(NOT CMP0157_value STREQUAL NEW)
...
endif()
Is there a way to check the value in if(...)
condition?
@@ -16,12 +15,18 @@ endfunction() | |||
function(force_target_link_libraries TARGET) | |||
target_link_libraries(${TARGET} ${ARGN}) | |||
|
|||
cmake_parse_arguments(ARGS "PUBLIC;PRIVATE;INTERFACE" "" "" ${ARGN}) | |||
force_add_dependencies(${TARGET} ${ARGS_UNPARSED_ARGUMENTS}) | |||
if(NOT CMP0157_IS_NEW) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't this get fixed in 3.26 or something?
We can use CMAKE_VERSION
with one of VERSION_LESS
, VERSION_GREATER
, VERSION_EQUAL
, VERSION_LESS_EQUAL
, or VERSION_GREATER_EQUAL
to do the comparison directly on the version itself rather than using the presence of the CMP0157 policy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure, tbh I didn't tried just removing this without enabling CMP0157
.
Is just checking CMAKE_VERSION
enough? As far as I tried, CMP0157
had to be set NEW
to remove these workarounds. Though I didn't tried removing each workaround individually .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the missing dependency edge on the swiftmodule here: https://gitlab.kitware.com/cmake/cmake/-/merge_requests/8084.
It should be fixed in anything newer than CMake 3.26. Versions older that 3.29 may have unnecessary rebuilds though, but that shouldn't be different than the forced rebuild anyway.
COMMAND "${CMAKE_COMMAND}" -E touch_nocreate $<TARGET_FILE:${name}> $<TARGET_OBJECTS:${name}> "${module_file}" | ||
COMMAND_EXPAND_LISTS | ||
COMMENT "Update mtime of library outputs workaround") | ||
if(NOT CMP0157_IS_NEW) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
COMMAND "${CMAKE_COMMAND}" -E touch "${CMAKE_CURRENT_BINARY_DIR}/${name}.swiftmodule" | ||
COMMAND_EXPAND_LISTS | ||
COMMENT "Update mtime of executable outputs workaround") | ||
if(NOT CMP0157_IS_NEW) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
Windows failure looks relevant https://ci-external.swift.org/job/swift-PR-windows/31664/
|
Use new CMake Swift compilation mode if possible.
Disable sowe workaround when CMP0157 is enabled.