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

Fix rhel and windows builds #164

Open
wants to merge 5 commits into
base: rolling
Choose a base branch
from
Open

Fix rhel and windows builds #164

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fc97148
Don't export the generated libraries as targets anymore, as nothing s…
ivanpauno May 16, 2022
349bb44
Don't link against targets that are not needed
ivanpauno May 16, 2022
58d48bb
Only remove the 'ament_export_targets()' call, keep the other stuff a…
ivanpauno May 16, 2022
69dfd51
Try a different approach
ivanpauno May 16, 2022
39e52ec
Fix windows debug support
ivanpauno May 16, 2022
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
5 changes: 0 additions & 5 deletions rosidl_generator_py/cmake/rosidl_generator_py_generate_interfaces.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,14 +273,9 @@ if(WIN32)
endif()
if(NOT rosidl_generate_interfaces_SKIP_INSTALL)
install(TARGETS ${_target_name_lib}
EXPORT export_${_target_name_lib}
ARCHIVE DESTINATION lib
LIBRARY DESTINATION lib
RUNTIME DESTINATION bin)

# Export this target so downstream interface packages can depend on it
rosidl_export_typesupport_targets("${rosidl_generator_py_suffix}" "${_target_name_lib}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Downstream interface packages depend on them. I'm not 100% sure they have to depend on them though.

rosidl_python/rosidl_generator_py/cmake/rosidl_generator_py_generate_interfaces.cmake

Line 264 in 3ce24ba

target_link_libraries(${_target_name_lib} PRIVATE ${${_pkg_name}_TARGETS${rosidl_generator_py_suffix}})

That's using the special rosidl suffix'd targets variable. How about removing only the ament_export_targets()? That way downstream targets won't accidentally link against them if using pkg_name_TARGETS or ament_target_dependencies().

Copy link
Member Author

Choose a reason for hiding this comment

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

Downstream interface packages depend on them. I'm not 100% sure they have to depend on them though.

Ah, I see.
I'm pretty sure that's not needed, but let me double check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Downstream interface packages depend on them. I'm not 100% sure they have to depend on them though.

I checked the generated code, and yes a downstream interface package depends on the generated library.

How about removing only the ament_export_targets()? That way downstream targets won't accidentally link against them if using pkg_name_TARGETS or ament_target_dependencies().

Agreed.
I will run CI again to see if it actually fixes the problem, I think it should.

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked the generated code, and yes a downstream interface package depends on the generated library.

Based on this, it seems that a bunch of stuff that I moved to PRIVATE in #160 is not correct.
Why I did that is because ament_export_dependencies() doesn't allow you to specify components, so I worked that arround by not having to export anything at all.
But in retrospective that seems incorrect.

I have now exported the dependency of FindPython3 manually, creating a config file.

That's using the special rosidl suffix'd targets variable. How about removing only the ament_export_targets()? That way downstream targets won't accidentally link against them if using pkg_name_TARGETS or ament_target_dependencies().

Unfortunately rosidl_export_typesupport_targets() requires you to also ament_export_targets().
It would be nice if ament_export_targets() had an option to not append the target to pkg_name_TARGETS.
But considering that I have undone most of what I did in #160, I think we don't need to worry about that now.

ament_export_targets(export_${_target_name_lib})
endif()

if(BUILD_TESTING AND rosidl_generate_interfaces_ADD_LINTER_TESTS)
Expand Down