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

Fix failure in config file installed by ament_cmake_export_dependencies when using more than one build type #263

Closed
wants to merge 2 commits into from

Conversation

ivanpauno
Copy link
Contributor

Fixes #262.

I'm not sure what we should do if the current configuration doesn't match any of the imported configurations.
Should IMPORTED_IMPLIB and IMPORTED_LOCATION be used in that case instead of the config specific ones?
Should one config be picked randomly from the imported configs?

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the bug Something isn't working label Jul 15, 2020
@ivanpauno ivanpauno self-assigned this Jul 15, 2020
@ivanpauno
Copy link
Contributor Author

This seem to not be the right approach. I got a link error when building lifecycle, likely related to this change in my local workspace.

@dirk-thomas
Copy link
Contributor

This seem to not be the right approach. I got a link error when building lifecycle, likely related to this change in my local workspace.

Does it actually enter the if block if("$<CONFIG>" IN_LIST _imported_configurations) when it fails to link or does is the condition FALSE and therefore doesn't link against anything from that target?

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Contributor Author

Does it actually enter the if block if("$" IN_LIST _imported_configurations) when it fails to link or does is the condition FALSE and therefore doesn't link against anything from that target?

Good point, generators expressions can't be used everywhere.
13fada7 fixes that, though I'm still unsure if this is the right approach.

if(_imported_implib)
list(APPEND _libraries "${_imported_implib}")
else()
get_target_property(_imported_location ${_target} IMPORTED_LOCATION_${_imported_configurations})
get_target_property(_imported_location ${_target} IMPORTED_LOCATION_$<CONFIG>)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems that generator expressions can't be used in get_target_property either, so $<CONFIG> is not being solved correctly 😕

@hidmic
Copy link
Contributor

hidmic commented Oct 16, 2020

@ivanpauno is this still a valid PR?

@siposcsaba89
Copy link
Contributor

siposcsaba89 commented Oct 17, 2020

@ivanpauno is this still a valid PR?

Hi everyone,

I just run into the same issue, when compiling ros2 on windows against libraries built with vcpkg, I wanted to also report the issue then I saw this PR, my solution is a little bit different: https://github.com/siposcsaba89/ament_cmake/commit/d36265934f022f97e674311dcb0039ed8c26b2e6

Thanks,
Csaba

@ivanpauno
Copy link
Contributor Author

@ivanpauno is this still a valid PR?

The PR tries to fix something that's still an issue, I was blocked on this comment and didn't find a way to workaround that problem.

I just run into the same issue, when compiling ros2 on windows against libraries built with vcpkg, I wanted to also report the issue then I saw this PR, my solution is a little bit different: siposcsaba89@d362659

Hi @siposcsaba89, your patch sounds reasonable to me, can you open a PR?

@ivanpauno ivanpauno closed this Oct 19, 2020
@ivanpauno ivanpauno deleted the ivanpauno/fix-get-target-property branch October 19, 2020 12:52
@siposcsaba89
Copy link
Contributor

@ivanpauno is this still a valid PR?

The PR tries to fix something that's still an issue, I was blocked on this comment and didn't find a way to workaround that problem.

I just run into the same issue, when compiling ros2 on windows against libraries built with vcpkg, I wanted to also report the issue then I saw this PR, my solution is a little bit different: siposcsaba89/ament_cmake@d362659

Hi @siposcsaba89, your patch sounds reasonable to me, can you open a PR?

Hi @ivanpauno ,

Of course, here it is #290

BR,
Csaba

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants