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.

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

Conversation

Felix-El
Copy link
Contributor

As mentioned in #505,

  • Allow linking by full path and non-target library name
  • Propagate linking dependencies to artifacts

- Allow linking by full path and non-target library name
- Propagate linking dependencies to artifacts
corrosion_link_libraries(${target_name} ${libs})
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

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.

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

)
add_dependencies(_cargo-build_${target_name} ${library})
get_property(libs TARGET ${library} PROPERTY INTERFACE_LINK_LIBRARIES)
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.

@jschwe
Copy link
Collaborator

jschwe commented Sep 5, 2024

Partially merged via #559. What's remaining is

Propagate linking dependencies to artifacts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants