Skip to content
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

Improvements to corrosion_link_libraries() #508

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improvements to corrosion_link_libraries() #508

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
26 changes: 23 additions & 3 deletions cmake/Corrosion.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1077,7 +1077,6 @@ function(corrosion_link_libraries target_name)
" aside from establishing a build dependency."
)
endif()
add_dependencies(_cargo-build_${target_name} ${ARGN})
foreach(library ${ARGN})
set_property(
TARGET _cargo-build_${target_name}
Expand All @@ -1086,8 +1085,29 @@ function(corrosion_link_libraries target_name)
$<TARGET_PROPERTY:${library},LINKER_LANGUAGE>
)

corrosion_add_target_local_rustflags(${target_name} "-L$<TARGET_LINKER_FILE_DIR:${library}>")
corrosion_add_target_local_rustflags(${target_name} "-l$<TARGET_LINKER_FILE_BASE_NAME:${library}>")
if (TARGET ${library})
corrosion_add_target_local_rustflags(${target_name}
"-L$<TARGET_LINKER_FILE_DIR:${library}>"
"-l$<TARGET_LINKER_FILE_BASE_NAME:${library}>"
)
add_dependencies(_cargo-build_${target_name} ${library})
get_property(libs TARGET ${library} PROPERTY INTERFACE_LINK_LIBRARIES)
Copy link
Collaborator

Choose a reason for hiding this comment

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

INTERFACE_LINK_LIBRARIES in CMake is evaluated at build time. This means simply reading this property here would cause us to miss any libraries that are added to INTERFACE_LINK_LIBRARIES after corrosion_link_libraries().
We could work around this by making a deferred function call for TARGETs, which reads the property at a later point in time.

This is just a nit, it's not required for merging this PR.

corrosion_link_libraries(${target_name} ${libs})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a recursion check here? While ideally there would be no cyclic dependencies, I don't think that assumption holds in practice.

The implementation could probably use something like this (not tested)

set(recursion_list "${recursion_list};${library}")
if( "${library}" IN ${recursion_list})
   return
endif()
# do the things
# the pop happens implicitly, due to scoping of variable names in functions.

elseif(IS_ABSOLUTE ${library})
# Linking via full path
corrosion_add_target_local_rustflags(${target_name} "-Clink-arg=${library}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add

else()
# We have to assume ${library} is a non-CMake library name
corrosion_add_target_local_rustflags(${target_name} "-l${library}")
endif()

# Propagate dependency on ${library}
set_property(
TARGET _cargo-build_${target_name}
APPEND
PROPERTY INTERFACE_LINK_LIBRARIES
Copy link
Collaborator

Choose a reason for hiding this comment

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

_cargo-build_${target_name} is a custom target, so I don't believe this property will have any effect.
Is your intention here to add a rule to trigger rebuilds, or to propogate the dependencies to C/C++ consumers of a Rust static library ? If it's the latter, then #506 should be sufficient.

${library}
)
endforeach()
endfunction(corrosion_link_libraries)

Expand Down