-
Notifications
You must be signed in to change notification settings - Fork 30
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
Use RTI Connext 6.0.1 on Jammy and when building Rolling on Windows #614
Conversation
Someone please cut a corner off my POSIX chit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment inline, but the code otherwise looks decent to me.
CI seems to be unhappy, though :(.
echo "Sourcing RTI setenv script /opt/rti.com/rti_connext_dds-6.0.1/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.sh" | ||
. /opt/rti.com/rti_connext_dds-6.0.1/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.sh | ||
elif test -r /opt/rti.com/rti_connext_dds-5.3.1/resource/scripts/rtisetenv_x64Linux3gcc5.4.0.bash; then | ||
echo "rti_connextdds_cmake_module will guess the location of Connext 5.3.1 so don't source anything." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiousity, is it actively harmful to source the 5.3.1 setup script? If not, I think it would be more consistent to just source it here (even if rti_connextdds_cmake_module
may redo some work).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not. I was aiming for a minimal change. I think there's a fair bit of refactoring we could do to simplify the plumbing here if we take a step back and look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! one minor speed bump is that we can't source the 5.3.1 script from a POSIX sh script without sed
ing it first as we do in the release repository for connext_cmake_module (and which hasn't been ported to rti_connext_cmake_module due to the guessing behavior)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine, this isn't a blocking problem anyway. Just curious about it.
This finished but wasn't able to find Connext. @asorbini @clalancette what are your thoughts on setting NDDSHOME or CONNEXTDDS_DIR via the Chef scripts or as part of the ROS 2 batch job? Also @asorbini is there a preferred/recommended variable between the two? |
3e744ad
to
f296430
Compare
Thank you for these updates @nuclearsandwich, a few comments on some of the issues:
The "guessing logic" in rti_connext_dds_cmake_module should only focus on coming up with a value for CONNEXTDDS_ARCH, if unspecified. The module calls Can you provide some more info on what problems you experienced when trying to load 6.0.1? rti_connext_dds_cmake_module doesn't really need the # instead of:
if test -r /opt/rti.com/rti_connext_dds-6.0.1/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.sh; then
echo "Sourcing RTI setenv script /opt/rti.com/rti_connext_dds-6.0.1/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.sh"
. /opt/rti.com/rti_connext_dds-6.0.1/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.sh
elif test -r /opt/rti.com/rti_connext_dds-5.3.1/resource/scripts/rtisetenv_x64Linux3gcc5.4.0.bash; then
echo "rti_connextdds_cmake_module will guess the location of Connext 5.3.1 so don't source anything."
# we could do:
if test -d /opt/rti.com/rti_connext_dds-6.0.1/lib/x64Linux4gcc7.3.0; then
export CONNEXTDDS_DIR=/opt/rti.com/rti_connext_dds-6.0.1 \
CONNEXTDDS_ARCH=x64Linux4gcc7.3.0
if test -d /opt/rti.com/rti_connext_dds-5.3.1/lib/x64Linux3gcc5.4.0; then
export CONNEXTDDS_DIR=/opt/rti.com/rti_connext_dds-5.3.1 \
CONNEXTDDS_ARCH=x64Linux3gcc5.4.0
The
As you found out, the .debs do not contain the Security extensions, only the basic libraries.
Keep in mind that you have also access to the non-eval bundles (for both 5.3.1 and 6.0.1) which can be installed in "unattended" mode, without requiring a dedicated
That would be the preferred way for me. The
NDDSHOME is the variable that's been used "historically" by Connext ("ndds" in an earlier life), to identify the installation directory. We have gradually introduced CONNEXTDDS_DIR in recent years as an alternative, and I chose to support both in My preference would go to CONNEXTDDS_DIR, since I'm looking forward to a future where NDDSHOME is eventually phased out :) |
I think all my specific issues are discussed upthread. I've been documenting some of the whack-a-mole approach to getting this working through the various places this hooks into our infrastructure but I think with your latest comments I've got paths forward everywhere. I'll keep documenting the whack-a-mole primarily to aid my future self in adding support for additional connext versions going forward and ping you directly if I run into any roadblocks or would like fresh advice. Thanks!
I'll send you an email about this as it complicates some future infrastructure changes that we have in the pipeline. Thanks for confirming it.
This is good info. We have a couple of different build scenarios we need to handle across our infrastructure and documentation:
Ideally each of these would work the exact same way so that the procedures we document for end-users match what we verify each night in our infrastructure nightlies. The binary packaging jobs for the both the old connext_dds_module and current rti_connext_dds_cmake_module packages include patches to support sourcing a slightly modified for sh compatibility copy of the setenv script in that package. Because that's what we're doing for our binary packages I'd like to use the same procedure (sourcing the rti setenv scripts) across all our infra and projects. If there's a preference to change those patches to just set In a pros/cons discussion. I like the transparency of "just setting an environment variable" versus running an executable script that can perform any number of operations but I like the future-proof nature of running a script provided by RTI to make any adjustments to the environs required so that there's a consistent interface that's somewhat immune to platform-specific changes. When adding the inertia of continuing to do what we're already doing I'd probably lean toward continuing to source the setenv script but I could very easily be swayed by additional arguments to make the switch to the environment variables. |
This is exactly what I wanted to know. Thanks! |
Let us never speak of this.
Because we're using an evaluation binary we can't use the unattended mode.
Escaping a newline means you need a semicolon har har. Indent the loop line for readability.
Once ros-infrastructure/ros2-cookbooks#40 is reviewed for Windows we can update this PR to point back to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything I left here is a nit. I'll leave it up to you whether it is worth addressing all, none, or some of the things I pointed out.
echo "Sourcing RTI setenv script /opt/rti.com/rti_connext_dds-6.0.1/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.sh" | ||
. /opt/rti.com/rti_connext_dds-6.0.1/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.sh | ||
elif test -r /opt/rti.com/rti_connext_dds-5.3.1/resource/scripts/rtisetenv_x64Linux3gcc5.4.0.bash; then | ||
echo "rti_connextdds_cmake_module will guess the location of Connext 5.3.1 so don't source anything." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine, this isn't a blocking problem anyway. Just curious about it.
✅ Connext 5.3.1 is installed via debs and rti_connext_dds_cmake_module finds it.
✅ Connext 5.3.1 is installed via pexpect script and rti_connext_dds_module finds it. |
✅ Connext 6.0.1 is installed via pro bundle with pexpect script (we could use the unattended mode but the pexpect script is working and I'd rather wait to refactor until we don't need it for 5.3.1) |
✅ mypy tests are expected on Jammy right now (see #617) |
Based on the test results for the last Windows job it does not appear that rmw_connextdds was used to run security tests. |
This is also true in the nightlies. The tests are only run for rmw_cyclonedds and rmw_fastrtps https://ci.ros2.org/view/nightly/job/nightly_win_rel/2205/testReport/test_security/TestSecurePublisherSubscriber/ |
✅ ConnextDDS 6.0.1 is properly installed and detected. |
Installs RTI Connext 6.0.1 via debs on Linux.
For Windows I updated the private submodule to ship both 5.3.1 and 6.0.1 and updated the cookbook parameters to use 6.0.1 in Rolling. A build for that is being tested
Once we're through these initial builds I'll start full CI runs to verify that this doesn't break anything above it.