Feature/dds 7.5.0 #1122
Feature/dds 7.5.0 #1122
Conversation
Signed-off-by: lobolanja <[email protected]>
…I license key Signed-off-by: lobolanja <[email protected]>
|
Hi @lobolanja , thank you for your contribution. Maybe @ibstewart could help validate the proposed changes on arm64? |
There was a problem hiding this comment.
Overall the change looks good (just a few trivial things that would need addressing), but unfortunately I just tried building with this change on arm64 and I'm getting this build error:
CMake Error at /opt/rti.com/rti_connext_dds-7.5.0/resource/cmake/FindRTIConnextDDS.cmake:1410 (message):
architecture is unsupported by this module
I tried making it work by fixing the bootstrap.sh script to use the armv8 version of the rtisetenv script instead, and then tried getting the CONNEXTDDS_ARCH env variable into the container to use it for the build, but I'm having trouble getting that both (a) available inside the container, and (b) available to the holohub.py script for use at build time, since that needs to be provided as a cmake variable.
Unfortunately I don't have more cycles to look into fixing the build, but maybe somebody more familiar with the holohub build scripts and/or docker could take a look(@tbirdso)?
|
|
||
|
|
||
| ## Building the Application | ||
| ## Building locally the Application |
There was a problem hiding this comment.
Should be Building the application locally
| # Install connext 7.5.0 from .deb package | ||
| # Download the public key for the RTI APT repository | ||
| RUN curl -sSL -o /usr/share/keyrings/rti-official-archive.gpg \ | ||
| https://packages.rti.com/deb/official/repo.key |
There was a problem hiding this comment.
This second line should be indented
|
|
||
| ```sh | ||
| $ ./holohub build dds_video --configure-args="-DRTI_CONNEXT_DDS_DIR=~/rti/rti_connext_dds-7.3.0" | ||
| $ ./holohub build dds_video" |
|
I found a bit more time and was able to get a working arm64 build, though unfortunately it looks like my RTI license expired so I'm not actually able to run it at the moment. It was at least able to run to the point of telling me it's expired, so I think that means it should otherwise be working? Here's the patch that I applied on top of this PR to fix the arm64 build: |
|
@lobolanja Could you please update the copyright date of |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (1)
-
applications/dds/dds_video/Dockerfile, line 18-26 (link)syntax: duplicate ARG declarations for BASE_IMAGE and GPU_TYPE - lines 18-19 are redundant with lines 25-26
6 files reviewed, 6 comments
| ```sh | ||
| # 1. Build and launch the container | ||
| ./holohub run-container dds_video --docker-opts="-v $HOME/rti_connext_dds-7.3.0:/opt/rti.com/rti_connext_dds-7.3.0/" | ||
| ./holohub run-container dds_video --docker-opts="-v ./rti_license.dat:/opt/rti.com/rti_connext_dds-7.5.0/rti_license.dat" |
There was a problem hiding this comment.
syntax: Trailing whitespace on this line
| # 1. Build and launch the container | ||
| ./holohub run-container dds_video --docker-opts="-v $HOME/rti_connext_dds-7.3.0:/opt/rti.com/rti_connext_dds-7.3.0/" | ||
| ./holohub run-container dds_video --docker-opts="-v ./rti_license.dat:/opt/rti.com/rti_connext_dds-7.5.0/rti_license.dat" | ||
| # 3. Build the application |
There was a problem hiding this comment.
syntax: Step numbering is incorrect - this says # 3. but should be # 2. since there's no step 2 listed
| any application using one of these operators. To build on an IGX devkit (using | ||
| - **RTI Connext 7.5.0 Express** | ||
| - Provides access to the DDS domain. | ||
| - Already included if using the [container build](../../applications/dds/dds_video/Dockerfile) you can find in the dds_video application directory |
There was a problem hiding this comment.
style: line exceeds 100 characters (109 chars), consider wrapping to next line
| up to step 5 (Installing Java and setting JREHOME). | ||
|
|
||
| To build the operators, the `RTI_CONNEXT_DDS_DIR` CMake variable must point to | ||
| To build the operators out of the dockerfile, the `RTI_CONNEXT_DDS_DIR` CMake variable must point to |
There was a problem hiding this comment.
style: "out of the dockerfile" is unclear - should this be "outside of the Docker container" or "when building natively"? What specific build scenario is this referring to?
|
|
||
| ```sh | ||
| $ ./holohub build dds_video --configure-args="-DRTI_CONNEXT_DDS_DIR=~/rti/rti_connext_dds-7.3.0" | ||
| $ ./holohub build dds_video" |
There was a problem hiding this comment.
syntax: missing closing double-quote - should be $ ./holohub build dds_video
|
Hi @lobolanja, could please address the feedback on this PR? Thanks |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUpdates RTI Connext DDS from version 7.3.0 to 7.5.0 across configuration, build, and documentation files. Changes include Dockerfile installation of DDS 7.5.0, a new bootstrap script, updated environment paths, expanded README guidance, RTI Workspace gitignore entry, and Dockerfile metadata reference. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
applications/dds/dds_video/Dockerfile (2)
25-26: Remove duplicate ARG declarations.Lines 18–19 already declare
BASE_IMAGEandGPU_TYPE. These duplicate declarations on lines 25–26 override them unnecessarily and violate the DRY principle.Apply this diff to remove the duplicates:
ARG BASE_IMAGE ARG GPU_TYPE ############################################################ # Base image ############################################################ -ARG BASE_IMAGE -ARG GPU_TYPE - FROM ${BASE_IMAGE} AS base
3-3: Update copyright year to 2022-2025.The copyright header shows "2022-2023" but should reflect the current year range as noted in the PR feedback.
Apply this diff:
-# SPDX-FileCopyrightText: Copyright (c) 2022-2023 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-FileCopyrightText: Copyright (c) 2022-2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
♻️ Duplicate comments (3)
operators/dds/README.md (2)
27-27: Clarify "out of the dockerfile" phrasing.The phrase is ambiguous. Please clarify whether this means "outside of the container" or "when building natively without Docker."
33-33: Fix syntax error: remove trailing quote.The build command has an unclosed/trailing double-quote that must be removed.
Apply this diff:
-$ ./holohub build dds_video" +$ ./holohub build dds_videoapplications/dds/dds_video/README.md (1)
77-77: Fix step numbering.Step 3 is listed before steps 1 and 2. Renumber to: step 1 (line 75), step 2 (line 77), step 3 (line 78).
Apply this diff:
-# 1. Build and launch the container +# 1. Build and launch the container ./holohub run-container dds_video --docker-opts="-v ./rti_license.dat:/opt/rti.com/rti_connext_dds-7.5.0/rti_license.dat" -# 3. Build the application +# 2. Build the application ./holohub build dds_video -# Continue to the next section to run the application with the publisher. +# 3. Continue to the next section to run the application with the publisher.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)applications/dds/dds_video/Dockerfile(2 hunks)applications/dds/dds_video/README.md(3 hunks)applications/dds/dds_video/bootstrap.sh(1 hunks)operators/dds/README.md(1 hunks)operators/dds/metadata.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{operators,applications,workflows,gxf_extensions,tutorials}/**/README.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
README.md must include purpose, usage instructions, requirements, examples, and architecture overview
Files:
applications/dds/dds_video/README.mdoperators/dds/README.md
**/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use Holoscan SDK glossary terms when referring to HoloHub-specific locations in documentation
Files:
applications/dds/dds_video/README.mdoperators/dds/README.md
**/metadata.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When reviewing PRs that modify or add
metadata.jsonfiles, verify that the category (first tag in thetagsarray) matches one of the approved categories: Benchmarking, Camera, Computer Vision and Perception, Converter, Deployment, Development, Extended Reality, Healthcare AI, Image Processing, Inference, Interoperability, Medical Imaging, Natural Language and Conversational AI, Networking and Distributed Computing, Optimization, Quantum Computing, Rendering, Robotics, Scheduler, Signal Processing, Streaming, Threading, Video, Video Capture, Visualization, XR
**/metadata.json: Useinterfaceor specific metadata schema for defining contribution structure (metadata.json)
metadata.json must include name, authors, language, version, changelog, holoscan_sdk info, platforms, tags, ranking, dependencies, and run command
metadata.json ranking levels must be self-assessed from 0 (production-ready) to 5 (deprecated) with corresponding requirements
Dependencies must be correctly specified in metadata.json dependencies section
Files:
operators/dds/metadata.json
🧠 Learnings (6)
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to {operators,applications,workflows,gxf_extensions,tutorials}/**/README.md : README.md must include purpose, usage instructions, requirements, examples, and architecture overview
Applied to files:
operators/dds/README.md
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to operators/**/CMakeLists.txt : CMakeLists.txt must include build system integration using add_holohub_operator() for operators
Applied to files:
operators/dds/README.md
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to operators/**/ : Operators should follow the directory structure with metadata.json, README.md, implementation files, test files, and CMakeLists.txt
Applied to files:
operators/dds/README.md
📚 Learning: 2025-11-24T16:28:06.281Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.281Z
Learning: Applies to **CMakeLists.txt : Use CMake target dependencies: add DEPENDS EXTENSIONS for operators wrapping GXF extensions, add DEPENDS OPERATORS for applications/workflows
Applied to files:
operators/dds/README.md
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Each operator should have at least one associated application demonstrating its capabilities
Applied to files:
operators/dds/README.md
📚 Learning: 2025-11-24T16:28:06.280Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T16:28:06.280Z
Learning: Applies to {operators,applications,workflows,gxf_extensions}/**/*.{py,cpp,hpp} : All code must adhere to Holoscan SDK coding standards including proper error handling and validation
Applied to files:
operators/dds/README.md
🔇 Additional comments (3)
.gitignore (1)
59-60: LGTM!The addition of
rti_workspace/to .gitignore is appropriate for excluding generated RTI workspace artifacts from version control.operators/dds/metadata.json (1)
22-22: LGTM!The addition of the dockerfile field correctly references the updated Dockerfile for DDS video application builds. Metadata structure is complete and consistent with coding guidelines.
applications/dds/dds_video/Dockerfile (1)
79-95: RTI Connext 7.5.0 APT installation approach is solid.The multi-architecture repository setup (using
dpkg --print-architectureandVERSION_CODENAMEdetection) and license acceptance handling are well designed for reproducible container builds across x86_64 and aarch64 platforms. However, ensure that the correspondingbootstrap.shscript supports ARM architecture detection to match this container's multi-arch capability.
| # limitations under the License. | ||
|
|
||
| source /opt/rti.com/rti_connext_dds-7.3.0/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.bash | ||
| source /opt/rti.com/rti_connext_dds-7.5.0/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.bash |
There was a problem hiding this comment.
Hardcoded x64 architecture will fail on ARM platforms.
The path uses a hardcoded x64Linux architecture, which will not work on armv8/IGX platforms. Per the PR review feedback, runtime architecture detection is required to support ARM builds.
Apply this diff to detect architecture at runtime:
-source /opt/rti.com/rti_connext_dds-7.5.0/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.bash
+arch="x64"
+if [ "$(uname -m)" = "aarch64" ]; then
+ arch="armv8"
+fi
+source /opt/rti.com/rti_connext_dds-7.5.0/resource/scripts/rtisetenv_${arch}Linux4gcc7.3.0.bash📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| source /opt/rti.com/rti_connext_dds-7.5.0/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.bash | |
| arch="x64" | |
| if [ "$(uname -m)" = "aarch64" ]; then | |
| arch="armv8" | |
| fi | |
| source /opt/rti.com/rti_connext_dds-7.5.0/resource/scripts/rtisetenv_${arch}Linux4gcc7.3.0.bash |
🤖 Prompt for AI Agents
In applications/dds/dds_video/bootstrap.sh around line 17, the script currently
hardcodes the x64Linux architecture in the rtisetenv path which will break on
ARM; change it to detect the runtime architecture (e.g. use uname -m), map
common values (x86_64 -> x64Linux4gcc7.3.0, aarch64 -> armv8Linux... or the
correct RTI directory name for your ARM build), construct the full path using
that mapped directory name and then source that path, and add a clear
error/fallback if the detected architecture is unsupported.
|
|
||
|
|
||
| ## Building the Application | ||
| ## Building locally the Application |
There was a problem hiding this comment.
Adjust section title wording.
"Building locally the Application" is awkward. Prefer "Building the Application Locally."
🤖 Prompt for AI Agents
In applications/dds/dds_video/README.md around line 56, the section title
"Building locally the Application" is awkward; rename it to "Building the
Application Locally" by updating the heading text to that exact phrasing so the
wording is natural and follows conventional capitalization.
| ```sh | ||
| # 1. Build and launch the container | ||
| ./holohub run-container dds_video --docker-opts="-v $HOME/rti_connext_dds-7.3.0:/opt/rti.com/rti_connext_dds-7.3.0/" | ||
| ./holohub run-container dds_video --docker-opts="-v ./rti_license.dat:/opt/rti.com/rti_connext_dds-7.5.0/rti_license.dat" |
There was a problem hiding this comment.
Remove trailing whitespace.
Trailing whitespace at the end of the docker-opts line.
🤖 Prompt for AI Agents
In applications/dds/dds_video/README.md around line 76, the docker-opts command
contains trailing whitespace at the end of the line; remove the trailing
space(s) after the closing quote so the line ends exactly with the command (no
extra spaces or tabs), then save the file.
Greptile Summaryupgraded RTI Connext DDS from 7.3.0 to 7.5.0 with improved installation and documentation
The Dockerfile changes are well-structured and use Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant Docker as Dockerfile Build
participant APT as RTI APT Repo
participant Bootstrap as bootstrap.sh
participant App as DDS Application
Dev->>Docker: Trigger build with base image
Docker->>APT: curl RTI public key
APT-->>Docker: rti-official-archive.gpg
Docker->>Docker: Add RTI APT repository with arch detection
Note over Docker: Uses $(dpkg --print-architecture)
Docker->>Docker: Set RTI_LICENSE_AGREEMENT_ACCEPTED=accepted
Docker->>APT: apt install rti-connext-dds-7.5.0
APT-->>Docker: Install DDS 7.5.0 to /opt/rti.com/
Docker->>Docker: Install openjdk-21-jre
Docker->>Docker: Set NDDSHOME=/opt/rti.com/rti_connext_dds-7.5.0
Docker->>Docker: Copy bootstrap.sh to /bootstrap.sh
Dev->>Docker: docker run with mounted rti_license.dat
Docker->>Bootstrap: Execute /bootstrap.sh as CMD
Bootstrap->>Bootstrap: Source rtisetenv_x64Linux4gcc7.3.0.bash
Note over Bootstrap: ⚠️ Hardcoded x64 architecture
Bootstrap->>App: Launch bash shell with DDS environment
App->>App: Run publisher/subscriber with DDS 7.5.0
|
| # limitations under the License. | ||
|
|
||
| source /opt/rti.com/rti_connext_dds-7.3.0/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.bash | ||
| source /opt/rti.com/rti_connext_dds-7.5.0/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.bash |
There was a problem hiding this comment.
logic: hardcodes x64Linux4gcc7.3.0 but PR claims ARM/IGX support - will fail on aarch64 systems
| source /opt/rti.com/rti_connext_dds-7.5.0/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.bash | |
| # Detect architecture and source appropriate script | |
| ARCH=$(uname -m) | |
| if [ "$ARCH" = "x86_64" ]; then | |
| source /opt/rti.com/rti_connext_dds-7.5.0/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.bash | |
| elif [ "$ARCH" = "aarch64" ]; then | |
| source /opt/rti.com/rti_connext_dds-7.5.0/resource/scripts/rtisetenv_armv8Linux4gcc7.3.0.bash | |
| else | |
| echo "Unsupported architecture: $ARCH" | |
| exit 1 | |
| fi |
| ```sh | ||
| # 1. Build and launch the container | ||
| ./holohub run-container dds_video --docker-opts="-v $HOME/rti_connext_dds-7.3.0:/opt/rti.com/rti_connext_dds-7.3.0/" | ||
| ./holohub run-container dds_video --docker-opts="-v ./rti_license.dat:/opt/rti.com/rti_connext_dds-7.5.0/rti_license.dat" |
There was a problem hiding this comment.
syntax: trailing whitespace at end of line
| # 1. Build and launch the container | ||
| ./holohub run-container dds_video --docker-opts="-v $HOME/rti_connext_dds-7.3.0:/opt/rti.com/rti_connext_dds-7.3.0/" | ||
| ./holohub run-container dds_video --docker-opts="-v ./rti_license.dat:/opt/rti.com/rti_connext_dds-7.5.0/rti_license.dat" | ||
| # 3. Build the application |
There was a problem hiding this comment.
syntax: step says # 3. but should be # 2. (no step 2 exists)
| # 3. Build the application | |
| # 2. Build the application |
|
|
||
| ```sh | ||
| $ ./holohub build dds_video --configure-args="-DRTI_CONNEXT_DDS_DIR=~/rti/rti_connext_dds-7.3.0" | ||
| $ ./holohub build dds_video" |
There was a problem hiding this comment.
syntax: missing closing double-quote
| $ ./holohub build dds_video" | |
| $ ./holohub build dds_video |
There was a problem hiding this comment.
Additional Comments (1)
-
applications/dds/dds_video/Dockerfile, line 95 (link)style: missing
--no-install-recommendsflag, inconsistent with other apt installs in this Dockerfile (lines 54, 62, 99)
6 files reviewed, 1 comment
There was a problem hiding this comment.
Pull request overview
This PR upgrades the DDS integration in HoloHub from RTI Connext 7.3.0 to 7.5.0, modernizing the installation approach by using the official RTI APT repository instead of manual installation. The update simplifies the workflow by embedding DDS installation directly in the Docker container and requiring only the license file to be mounted at runtime.
Key changes:
- Upgraded RTI Connext DDS from version 7.3.0 to 7.5.0 with automated APT-based installation
- Updated documentation with clearer prerequisites, Docker build instructions, and simplified license handling
- Modified container setup to install DDS via APT repository rather than requiring host-mounted DDS installation
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
.gitignore |
Added rti_workspace/ to exclude RTI Connext workspace artifacts from version control |
operators/dds/metadata.json |
Added dockerfile reference linking operators to the DDS video application's Dockerfile |
operators/dds/README.md |
Updated prerequisites and build instructions to reference RTI Connext 7.5.0 and APT installation method |
applications/dds/dds_video/bootstrap.sh |
Updated version path from 7.3.0 to 7.5.0 in the RTI environment setup script |
applications/dds/dds_video/README.md |
Comprehensive documentation update with clearer Docker workflow, updated version references, and simplified license mounting |
applications/dds/dds_video/Dockerfile |
Added APT repository setup and installation of RTI Connext DDS 7.5.0, updated environment variables and bootstrap script integration |
| ```sh | ||
| # 1. Build and launch the container | ||
| ./holohub run-container dds_video --docker-opts="-v $HOME/rti_connext_dds-7.3.0:/opt/rti.com/rti_connext_dds-7.3.0/" | ||
| ./holohub run-container dds_video --docker-opts="-v ./rti_license.dat:/opt/rti.com/rti_connext_dds-7.5.0/rti_license.dat" |
There was a problem hiding this comment.
There is a trailing space at the end of this line. This should be removed for consistency and to avoid unnecessary whitespace in the file.
| ./holohub run-container dds_video --docker-opts="-v ./rti_license.dat:/opt/rti.com/rti_connext_dds-7.5.0/rti_license.dat" | |
| ./holohub run-container dds_video --docker-opts="-v ./rti_license.dat:/opt/rti.com/rti_connext_dds-7.5.0/rti_license.dat" |
|
|
||
| ```sh | ||
| $ ./holohub build dds_video --configure-args="-DRTI_CONNEXT_DDS_DIR=~/rti/rti_connext_dds-7.3.0" | ||
| $ ./holohub build dds_video" |
There was a problem hiding this comment.
There is a syntax error in this command - there's an extra opening double quote at the end of the line without a closing quote. The command should be:
$ ./holohub build dds_video
| $ ./holohub build dds_video" | |
| $ ./holohub build dds_video |
| RUN curl -sSL -o /usr/share/keyrings/rti-official-archive.gpg \ | ||
| https://packages.rti.com/deb/official/repo.key |
There was a problem hiding this comment.
The curl command is used here to download the RTI APT repository key, but curl is not explicitly installed in this Dockerfile. If curl is not available in the base image, this command will fail. Consider adding curl to the apt install command or verify that it's available in the base image.
There was a problem hiding this comment.
curl is already installed in the HSDK base image, so checking would be nice but not necessary here.
| # limitations under the License. | ||
|
|
||
| source /opt/rti.com/rti_connext_dds-7.3.0/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.bash | ||
| source /opt/rti.com/rti_connext_dds-7.5.0/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.bash |
There was a problem hiding this comment.
The bootstrap script hardcodes the x64Linux architecture in the path 'rtisetenv_x64Linux4gcc7.3.0.bash'. This contradicts the PR description which claims ARM/IGX (armv8) support. The script should detect the architecture dynamically or use a different approach that works across both x86_64 and ARM platforms. Consider using environment variables or checking the architecture at runtime.
| source /opt/rti.com/rti_connext_dds-7.5.0/resource/scripts/rtisetenv_x64Linux4gcc7.3.0.bash | |
| arch="$(uname -m)" | |
| case "${arch}" in | |
| x86_64) | |
| rti_arch="x64Linux4gcc7.3.0" | |
| ;; | |
| aarch64|arm64) | |
| rti_arch="armv8Linux4gcc7.3.0" | |
| ;; | |
| *) | |
| echo "Unsupported architecture: ${arch}" >&2 | |
| exit 1 | |
| ;; | |
| esac | |
| source "/opt/rti.com/rti_connext_dds-7.5.0/resource/scripts/rtisetenv_${rti_arch}.bash" |
| To build the operators out of the dockerfile, the `RTI_CONNEXT_DDS_DIR` CMake variable must point to | ||
| the installation path for RTI Connext. This can be done automatically by setting | ||
| the `NDDSHOME` environment variable to the RTI Connext installation directory | ||
| (such as when using the RTI `setenv` scripts), or manually at build time, e.g.: | ||
|
|
||
| ```sh | ||
| $ ./holohub build dds_video --configure-args="-DRTI_CONNEXT_DDS_DIR=~/rti/rti_connext_dds-7.3.0" | ||
| $ ./holohub build dds_video" | ||
| ``` |
There was a problem hiding this comment.
The build command example is incomplete and lacks context. The sentence "To build the operators out of the dockerfile, the RTI_CONNEXT_DDS_DIR CMake variable must point to..." followed by this command is confusing because the command shown doesn't include the configure-args mentioned in the preceding text. Either complete the example to show how to set RTI_CONNEXT_DDS_DIR, or clarify that when using the Dockerfile, these variables are already set and the simple command is sufficient.
There was a problem hiding this comment.
Additional Comments (1)
-
applications/dds/dds_video/bootstrap.sh, line 17 (link)logic: hardcoded
x64Linux4gcc7.3.0contradicts PR's ARM/IGX support claim - will fail on aarch64The Dockerfile correctly uses
dpkg --print-architectureto install the right DDS package for any architecture, but this bootstrap script hardcodes x64. On ARM systems, the path should be something likearmv8Linux4gcc7.3.0or similar.Add architecture detection:
Or use a more robust approach that checks available scripts in the directory.
6 files reviewed, 1 comment
Greptile found no issues!From now on, if a review finishes and we haven't found any issues, we will not post anything, but you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
4 participants
Summary
This PR updates DDS integration in HoloHub:
Documentation
README.mdfor the DDS Video Application, covering prerequisites, Docker/local build steps, and license setup.Dockerfile
dds_videoDockerfile to install RTI Connext DDS 7.5.0 from the official RTI APT repository.rti_license.dat).Testing Notes
Request for Feedback
IGX validation
Please run the DDS Video application (publisher + subscriber) on an IGX box following the updated instructions and confirm it works.
CI integration
Is it possible to add a CI step that at least verifies the DDS operator builds on IGX?
Greptile Overview
Updated On: 2025-10-20 14:57:21 UTC
Greptile Summary
This PR upgrades HoloHub's DDS integration from RTI Connext 7.3.0 to 7.5.0, modernizing the installation approach and improving developer experience. The key change replaces manual RTI installation with automated APT repository installation directly in the container, eliminating the need to mount the entire DDS SDK at runtime. Only the license file (
rti_license.dat) needs to be mounted now. Documentation has been restructured with clearer prerequisites, step-by-step Docker instructions, and simplified build commands. The.gitignorewas updated to exclude RTI workspace artifacts, and metadata now explicitly references the DDS Dockerfile. This change integrates with HoloHub's existing operator/application structure by ensuring the DDS operators can be built with a well-defined, reproducible container environment rather than depending on host-specific manual installations.Important Files Changed
Changed Files
.gitignorerti_workspace/pattern to exclude RTI Connext workspace artifacts from version controlapplications/dds/dds_video/Dockerfileapplications/dds/dds_video/bootstrap.shapplications/dds/dds_video/README.mdoperators/dds/README.mdoperators/dds/metadata.jsonConfidence score: 3/5
bootstrap.shthat contradicts PR's ARM/IGX support claims, (2) syntax error inoperators/dds/README.mdline 33 (unclosed quote), (3) duplicate ARG declarations in Dockerfile lines 18-19 and 25-26, and (4) incomplete build command example in operators README.applications/dds/dds_video/bootstrap.sh(architecture hardcoding),operators/dds/README.md(syntax error at line 33), andapplications/dds/dds_video/Dockerfile(duplicate ARG declarations and curl dependency).Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.