ros2 / ci

ROS 2 CI Infrastructure
http://ci.ros2.org/
Apache License 2.0
48 stars 30 forks source link

Remove support for Connext 5.3.1. #716

Closed clalancette closed 1 year ago

clalancette commented 1 year ago

There are no current distributions that currently require it.

One thing in this commit that warrants some explanation is the complete removal of the sourcing of Connext in the linux_batch/init.py and windows_batch/init.py files. They are completely removed because with Connext 6, they are no longer used. On Linux it is sourced from the entry_point.sh, and on Windows I'm not entirely sure where it is sourced from.

I'll also point out that there is one remaining reference to Connext 5.3.1 in the windows_docker_resources/install_ros2_foxy.json file. But I think it makes more sense to remove that when we completely remove foxy support in an upcoming change.

nuclearsandwich commented 1 year ago

on Windows I'm not entirely sure where it is sourced from.

In the interest of Chesterton's Fence ideally we'd document why those scripts are no longer needed.

nuclearsandwich commented 1 year ago

on Windows I'm not entirely sure where it is sourced from.

In the interest of Chesterton's Fence ideally we'd document why those scripts are no longer needed.

We dug into this offline a little and the abbreviated explanation is that on Windows our chef setup configures the CONNEXTDDS_DIR environment variable system-wide so that rti_load_connextddsdir in rti_connext_dds_cmake_module can find and use it.

clalancette commented 1 year ago

On Linux it is sourced from the entry_point.sh, and on Windows I'm not entirely sure where it is sourced from.

OK, after some good help from @nuclearsandwich , we figured out how Connext is sourced.

On Linux, we end up sourcing /opt/rti.com/rti_connext_dds-6.0.1/scripts/rtisetenv_x64Linux4gcc7.3.0.sh during the entrypoint.sh script. In turn, this script ends up setting two environment variables: NDDSHOME (the top-level of where Connext is installed), and LD_LIBRARY_PATH (so the linkers can find the shared libraries).

On Windows, we end up manually creating an environment variable called CONNEXTDDS_DIR during the chef scripts.

The last piece of the puzzle, then, is how these environment variables get turned into the libraries that are needed for linking/running. That happens when the rti_load_connextddsdir CMake function is called when trying to build rmw_connextdds. It is important to realize there that it first looks for the CONNEXTDDS_DIR environment variable (which is what we set on Windows), and if that doesn't exist, falls back to looking for NDDSHOME (which is what we set on Linux). So both work.

clalancette commented 1 year ago

With this approved, what I'm going to do here is to run some test jobs. I shouldn't need a test deploy since there are no changes to the job templates. Assuming those come back green I'll go ahead and merge. Thanks!

clalancette commented 1 year ago

CI:

clalancette commented 1 year ago

All right, looks like all CI was green, and I manually verified that they all found Connext as expected. I'll go ahead and merge this one.