Conversation
Signed-off-by: Francisco Gallego Salido <fgallego@rti.com>
|
@clalancette could you run a validation to make sure Linux aarch64 is being built and tested? Once the built is running correctly, any further test issues we find, we will fix them as part of ros2/rmw_connextdds#222. |
|
@clalancette @cottsay @fujitatomoya can someone look into this? We would like to get this merged before Lyrical Luth. |
| echo "Connext is only supported on amd64 architecture. Skipping Connext installation." >&2 | ||
| exit 1 |
There was a problem hiding this comment.
Existing behavior is to quietly skip connext installation on aarch64 even if requested. While this PR enables the deb installation scenario, I think we should continue the existing behavior and not fail the build if non-deb installation is requested.
The main scenario here is that someone might trigger a job from the launcher that uses the non-deb Connext installation. I don't think it would be a good experience if the aarch64 builds suddenly start failing and require re-running with connext disabled.
There was a problem hiding this comment.
There are a couple of places where we do exit 1. Should we remove those too so that the build doesn't fail?
There was a problem hiding this comment.
Should we remove those too so that the build doesn't fail?
It's totally appropriate to raise a critical error if something is expected to work and doesn't. This case is different though - it's not supported and is not expected to work, and the previous behavior for that scenario (requesting Connext via binaries on arm64) was to quietly continue the build.
There was a problem hiding this comment.
I refactored the Connext installation into a new function, maintaining the same behavior as before. Let me know what you think and launch a validation as soon as you feel okay with the code.
Signed-off-by: Francisco Gallego Salido <fgallego@rti.com>
… setenv script for ARM architectures Signed-off-by: Francisco Gallego Salido <fgallego@rti.com>
|
@cottsay @clalancette could you review the pr again and launch a validation if possible? |
| if test \( ${ROS_DISTRO} = humble -o ${ROS_DISTRO} = jazzy \); then \ | ||
| RUN \ | ||
| if test ${INSTALL_CONNEXT_DEBS} = true; then \ | ||
| if test \( ${PLATFORM} = x86 -a \( ${ROS_DISTRO} = humble -o ${ROS_DISTRO} = jazzy \) \); then \ |
There was a problem hiding this comment.
I think I'd rather see the test \( ${PLATFORM} = x86 check happen within this conditional block as a separate check. As it stands, this change would result in installing 7.3.0 on ALL non-x86 builds, ignoring the ROS_DISTRO entirely.
I think we'd like to keep the same behavior for Humble and Jazzy, and not install any Connext debs, rather than install a version we don't expect to work.
There was a problem hiding this comment.
Would you prefer a switch for each ROS_DISTRO, and inside handle the logic? Something similar to the entry_point
Co-authored-by: Scott K Logan <logans@cottsay.net> Signed-off-by: Francisco Gallego Salido <fgallego@rti.com>
Signed-off-by: Francisco Gallego Salido <fgallego@rti.com>
2 participants
Description
Enable building the
rmw_connextddsin aarch64 only using Debian packages.Fixes #852
Is this user-facing behavior change?
Pull requests can now run CI on aarch64 using the
rti-connext-dds-7.3.0-rosDebian package. This should also allow the generation of thermw_connextddspackages and its dependencies for aarch64.Did you use Generative AI?
No
Additional Information
The aim of these changes is to allow the Connext RMW to officially support aarch64 architectures.