Skip to content

Temporarily workaround CMake dependencies bug #1081

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
Nov 17, 2022
Merged

Temporarily workaround CMake dependencies bug #1081

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
25 changes: 25 additions & 0 deletions Sources/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,31 @@
# See http://swift.org/LICENSE.txt for license information
# See http://swift.org/CONTRIBUTORS.txt for Swift project authors

# cmake generation for Swift adds an order only dependency, which matches how C-family languages
# works. In that case, however, ninja (and presumably other generators) will rebuild on header
# changes. That's not the case for Swift, and thus if A -> B, A is not being rebuilt when the
# ABI/API of B changes.
#
# For now workaround this by touching a file whenever B is rebuilt and then compiling that file as
# part of A. Ideally this file would be generated by each of the targets, but that dependency didn't
# seem to be being tracked.
#
# Remove once rdar://102202478 is fixed.
function(target_link_libraries TARGET)
cmake_parse_arguments(ARGS "" "" "PUBLIC" ${ARGN})
Copy link
Member

Choose a reason for hiding this comment

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

The naming here feels a bit weird. I'm not getting that it's linking from the function name, and the PUBLIC seems to come out of nowhere.

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 I totally agree about the PUBLIC, but I was basically just aiming at making it a 1:1 replacement for target_link_libraries (at least for the case we use it). I don't really mind either way. Do you have a strong opinion here?

How about I rename it to force_target_link_libraries?

Copy link
Member

Choose a reason for hiding this comment

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

I actually think that this change is far too invasive for what it intends to do. I think that we should be more clever about this workaround. Please rename this function target_link_librares. We can subsequently, at the end of the function passthrough to the underlying implementation of target_link_libraries.

Something like this should do the trick:

function(target_link_libraries target)
  cmake_parse_arguments(TLL_ARGS "" "" "PUBLIC" ${ARGN})

  foreach(dependency ${TLL_PUBLIC})
    ...
  endforeach()

  _target_link_libraries(${target} PUBLIC ${TLL_PUBLIC} ${TLL_ARGN})
endfunction()

This way, in the future, the revert of this will just drop this one function and everything goes back to normal.

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 no idea you could do that. I've updated the PR to do this instead, but I'll leave the Swift PR as is - once the function is redefined, it's redefined from that point on (not scoped to a subdirectory). This is fine for swift-syntax, but not for the swift-side change (and that only touches the one file anyway). Thanks for the suggestion @compnerd!


_target_link_libraries(${TARGET} PUBLIC ${ARGS_PUBLIC})
foreach(DEPENDENCY ${ARGS_PUBLIC})
add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/forced-${DEPENDENCY}-dep.swift
COMMAND ${CMAKE_COMMAND} -E touch ${CMAKE_CURRENT_BINARY_DIR}/forced-${DEPENDENCY}-dep.swift
DEPENDS ${DEPENDENCY}
)
target_sources(${TARGET} PRIVATE
${CMAKE_CURRENT_BINARY_DIR}/forced-${DEPENDENCY}-dep.swift
)
endforeach()
endfunction()

add_subdirectory(SwiftBasicFormat)
add_subdirectory(SwiftSyntax)
add_subdirectory(SwiftDiagnostics)
Expand Down