Skip to content

fix(rosidl_generator_rs_generate_interfaces): Remove poisoning of global CMAKE_SHARED_LINKER_FLAGS variable#22

Merged
esteve merged 1 commit into
ros2-rust:mainfrom
traversaro:patch-1
Apr 11, 2026
Merged

fix(rosidl_generator_rs_generate_interfaces): Remove poisoning of global CMAKE_SHARED_LINKER_FLAGS variable#22
esteve merged 1 commit into
ros2-rust:mainfrom
traversaro:patch-1

Conversation

@traversaro
Copy link
Copy Markdown
Contributor

@traversaro traversaro commented Mar 29, 2026

I could not find why those lines were there in the first place (they were there since the first version of the repo) but in general CMAKE_* variables are global variables that have a global effect, so unless there is a clear specific motivation, it is better to avoid modifying their value, as it could create side-effects in unrelated part of the CMake project in which this is used, see for example ros2/rosidl_python#253 .

…MAKE_SHARED_LINKER_FLAGS variable

I could not find why those lines were there in the first place (they were there since the first version of the repo) 
but in general CMAKE_* variables are global variables that have a global effect, so unless there is a clear specific motivation,
it is better to avoid modifying their value, as it could create side-effects in unrelated part of the CMake project in which this is used.
@esteve
Copy link
Copy Markdown
Contributor

esteve commented Apr 10, 2026

@traversaro thanks for the PR. These variable will only be set while rosidl_generator_rs runs, so it shouldn't have any side effects. I set those variable to make sure that the shared libraries that we create have all the symbols and fail at link time if they dont, instead of failing at runtime. Are there any side effects you're experiencing with the way these variables are set? Thanks.

@traversaro
Copy link
Copy Markdown
Contributor Author

traversaro commented Apr 11, 2026

These variable will only be set while rosidl_generator_rs runs, so it shouldn't have any side effects.

Unfortunately side effects are actually happening, due to how CMAKE_* variables work and how ament_execute_extensions work.

In particular rosidl_generate_interfaces() is implemented as a CMake macro in rosidl_cmake/cmake/rosidl_generate_interfaces.cmake, and inside it ROS calls ament_execute_extensions("rosidl_generate_idl_interfaces"). Then ament_execute_extensions.cmake iterates over all registered generators and literally does include("${_extension_file}") for each one.

So, depending on the order in which the generators are included, a variable set in one generator affects another generator, resulting in cross-talking problems like the one described in ros2/rosidl_python#253 .

Anyhow, this PR was opened exactly to understand if there was a motivation for having those lines. If such a motivation is there, I can convert the PR to set -Wl,--no-undefined explicitly only on the targets created by this generator.

@traversaro
Copy link
Copy Markdown
Contributor Author

Anyhow, this PR was opened exactly to understand if there was a motivation for having those lines. If such a motivation is there, I can convert the PR to set -Wl,--no-undefined explicitly only on the targets created by this generator.

I tried to do that, but to be honest I am a bit confused: which target in your idea should be affected by those flags? As far as I could see, the generator only generates .rs files, it does not create any library.

@esteve esteve changed the title rosidl_generator_rs_generate_interfaces: Remove poisoning of global CMAKE_SHARED_LINKER_FLAGS variable fix(rosidl_generator_rs_generate_interfaces): Remove poisoning of global CMAKE_SHARED_LINKER_FLAGS variable Apr 11, 2026
@esteve
Copy link
Copy Markdown
Contributor

esteve commented Apr 11, 2026

@traversaro you're right! Originally the generator created C libraries and they linked against the ROS libraries, but it hasn't done that in a long time. Thanks for the PR, I'll merge the changes right now.

@esteve esteve merged commit 0934c89 into ros2-rust:main Apr 11, 2026
@github-actions github-actions Bot mentioned this pull request Apr 11, 2026
@traversaro
Copy link
Copy Markdown
Contributor Author

Thanks!

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