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

Show file tree
Hide file tree
Changes from all commits
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
25 changes: 25 additions & 0 deletions rosidl_generator_py/cmake/rosidl_generator_py_generate_interfaces-extra.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Copyright 2022 Open Source Robotics Foundation, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# This is needed as ament_export_dependencies doesn't work with cmake components.

# Figure out Python3 debug/release before anything else can find_package it
if(WIN32 AND CMAKE_BUILD_TYPE STREQUAL "Debug")
find_package(python_cmake_module REQUIRED)
find_package(PythonExtra REQUIRED)

# Force FindPython3 to use the debug interpreter where ROS 2 expects it
set(Python3_EXECUTABLE "${PYTHON_EXECUTABLE_DEBUG}")
endif()
find_package(Python3 REQUIRED COMPONENTS Development NumPy)
35 changes: 18 additions & 17 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 @@ -18,9 +18,14 @@ find_package(rosidl_typesupport_c REQUIRED)
find_package(rosidl_typesupport_interface REQUIRED)

find_package(PythonInterp 3.6 REQUIRED)

find_package(python_cmake_module REQUIRED)
find_package(PythonExtra MODULE REQUIRED)
find_package(PythonExtra REQUIRED)
set(_Python3_EXECUTABLE ${Python3_EXECUTABLE})
if(WIN32 AND CMAKE_BUILD_TYPE STREQUAL "Debug")

# Force FindPython3 to use the debug interpreter where ROS 2 expects it
set(Python3_EXECUTABLE "${PYTHON_EXECUTABLE_DEBUG}")
endif()
find_package(Python3 REQUIRED COMPONENTS Development NumPy)

# Get a list of typesupport implementations from valid rmw implementations.
Expand Down Expand Up @@ -127,11 +132,6 @@ endif()

set(_target_suffix "__py")

set(_PYTHON_EXECUTABLE ${PYTHON_EXECUTABLE})
if(WIN32 AND "${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
set(PYTHON_EXECUTABLE ${PYTHON_EXECUTABLE_DEBUG})
endif()

# move custom command into a subdirectory to avoid multiple invocations on Windows
set(_subdir "${CMAKE_CURRENT_BINARY_DIR}/${rosidl_generate_interfaces_TARGET}${_target_suffix}")
file(MAKE_DIRECTORY "${_subdir}")
Expand Down Expand Up @@ -166,7 +166,6 @@ set(rosidl_generator_py_suffix "__rosidl_generator_py")
set(_target_name_lib "${rosidl_generate_interfaces_TARGET}${rosidl_generator_py_suffix}")
add_library(${_target_name_lib} SHARED ${_generated_c_files})
target_link_libraries(${_target_name_lib}
PRIVATE
${rosidl_generate_interfaces_TARGET}__rosidl_generator_c)
add_dependencies(
${_target_name_lib}
Expand All @@ -179,10 +178,10 @@ target_include_directories(${_target_name_lib}
${CMAKE_CURRENT_BINARY_DIR}/rosidl_generator_py
)

target_link_libraries(${_target_name_lib} PRIVATE Python3::NumPy Python3::Module)
target_link_libraries(${_target_name_lib} Python3::NumPy Python3::Module)

rosidl_get_typesupport_target(c_typesupport_target "${rosidl_generate_interfaces_TARGET}" "rosidl_typesupport_c")
target_link_libraries(${_target_name_lib} PRIVATE ${c_typesupport_target})
target_link_libraries(${_target_name_lib} ${c_typesupport_target})

foreach(_typesupport_impl ${_typesupport_impls})
find_package(${_typesupport_impl} REQUIRED)
Expand Down Expand Up @@ -217,7 +216,6 @@ foreach(_typesupport_impl ${_typesupport_impls})
endif()
target_link_libraries(
${_target_name}
PRIVATE
${_target_name_lib}
${rosidl_generate_interfaces_TARGET}__${_typesupport_impl}
Python3::Module
Expand All @@ -229,24 +227,23 @@ foreach(_typesupport_impl ${_typesupport_impls})
${CMAKE_CURRENT_BINARY_DIR}/rosidl_generator_py
)

target_link_libraries(${_target_name} PRIVATE ${c_typesupport_target})
target_link_libraries(${_target_name} ${c_typesupport_target})

ament_target_dependencies(${_target_name}
PUBLIC
"rosidl_runtime_c"
"rosidl_typesupport_c"
"rosidl_typesupport_interface"
)
foreach(_pkg_name ${rosidl_generate_interfaces_DEPENDENCY_PACKAGE_NAMES})
ament_target_dependencies(${_target_name} PUBLIC
ament_target_dependencies(${_target_name}
${_pkg_name}
)
endforeach()

add_dependencies(${_target_name}
${rosidl_generate_interfaces_TARGET}__${_typesupport_impl}
)
ament_target_dependencies(${_target_name} PUBLIC
ament_target_dependencies(${_target_name}
"rosidl_runtime_c"
"rosidl_generator_py"
)
Expand All @@ -257,11 +254,12 @@ foreach(_typesupport_impl ${_typesupport_impls})
endif()
endforeach()

set(PYTHON_EXECUTABLE ${_PYTHON_EXECUTABLE})
set(Python3_EXECUTABLE ${_Python3_EXECUTABLE})
unset(_Python3_EXECUTABLE)

# Depend on rosidl_generator_py generated targets from our dependencies
foreach(_pkg_name ${rosidl_generate_interfaces_DEPENDENCY_PACKAGE_NAMES})
target_link_libraries(${_target_name_lib} PRIVATE ${${_pkg_name}_TARGETS${rosidl_generator_py_suffix}})
target_link_libraries(${_target_name_lib} ${${_pkg_name}_TARGETS${rosidl_generator_py_suffix}})
endforeach()

set_lib_properties("")
Expand All @@ -281,6 +279,9 @@ if(NOT rosidl_generate_interfaces_SKIP_INSTALL)
# 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})

# This is needed as ament_export_dependencies doesn't work with cmake components.
list(APPEND ${PROJECT_NAME}_CONFIG_EXTRAS ${CMAKE_CURRENT_LIST_DIR}/rosidl_generator_py_generate_interfaces-extra.cmake)
endif()

if(BUILD_TESTING AND rosidl_generate_interfaces_ADD_LINTER_TESTS)
Expand Down