DRAFT: CLI: Add "tar.gz" packaging support and exercise with Holoscan GStreamer#1604
DRAFT: CLI: Add "tar.gz" packaging support and exercise with Holoscan GStreamer#1604tbirdso wants to merge 4 commits into
Conversation
Goal: Support source archive packaging for Holoscan Modules adhering to an internal specification, focusing on "holoscan-gstreamer" - Add "holohub_configure_tgz.cmake" CPack helper to create CPack TGZ generator configurations. Adhere to internal naming specifications and support component filtering to drop Python components. Matches existing Deb generator support. - Add TGZ to CLI packaging handling - Tag Python module install with COMPONENT holohub-python and operator installs with COMPONENT holoscan-gstreamer to enable per-component archive filtering Assisted-by: Claude:claude-sonnet-4-6 Signed-off-by: Tom Birdsong <tbirdsong@nvidia.com>
Addresses two findings from code review of MR !423. - Fix CTest install fixture to use --component holoscan-gstreamer (was --component gstreamer) so artifacts are installed when BUILD_TESTING is enabled - Add hard-fail in holohub.py when no CPack configs exist after a real build (non-dryrun path) - Add hard-fail when no config is found for a specific requested generator, listing available generator-specific configs in the error Assisted-by: Claude:claude-sonnet-4-6 Signed-off-by: Tom Birdsong <tbirdsong@nvidia.com>
New coverage for holohub_configure_tgz / CLI packaging changes. - test_package_tgz_invokes_cpack_tgz: verifies -G TGZ is passed to cpack when --pkg-generator TGZ is requested - test_package_multi_generator_deb_tgz: verifies two separate cpack calls are made for DEB,TGZ (one per generator) - test_package_tgz_routes_to_generator_specific_config: verifies CPackConfig-*-TGZ.cmake is used for TGZ and the base config for DEB - test_package_missing_cpack_configs_fatal: verifies fatal() is called when the build dir contains no CPack configs (non-dryrun path) - test_package_missing_generator_config_fatal: verifies fatal() lists available generators when the requested one has no matching config Assisted-by: Claude:claude-sonnet-4-6 Signed-off-by: Tom Birdsong <tbirdsong@nvidia.com>
Address divergent path requirements:
- wheel and Deb format expect "lib/holoscan_gstreamer_bridge.so"
- TGZ format expects "lib/<cuda_version>/holoscan_gstreamer_bridge.so"
Update TGZ handling as follows:
- Revert the operator library install to DESTINATION ${CMAKE_INSTALL_LIBDIR};
holohub_configure_tgz now overrides CMAKE_INSTALL_LIBDIR to lib/<cuda_major>
via the cmake cache when HOLOHUB_PKG_TGZ=ON, exploiting HoloHub's
modules-before-operators build order so operator install() rules pick it up
- CLI passes -DHOLOHUB_PKG_TGZ=ON to cmake configure only when TGZ is in the
requested generators, leaving DEB, WHEEL, and standalone builds unaffected
- Update CMAKE_INSTALL_DEFAULT_COMPONENT_NAME to holoscan-gstreamer (was
gstreamer) and add unit tests asserting the flag is present for TGZ builds
and absent for DEB-only builds
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Tom Birdsong <tbirdsong@nvidia.com>
Greptile SummaryThis PR adds TGZ (
Confidence Score: 3/5The CLI change that sets HOLOHUB_PKG_TGZ=ON at cmake configure time would silently produce a malformed DEB package when DEB and TGZ are both requested in the same run, because the library install path is changed globally for all generators. The CMAKE_INSTALL_LIBDIR forced override in holohub_configure_tgz is intentional for TGZ builds, but the CLI applies it unconditionally whenever TGZ appears in the generator list — even alongside DEB. Since both generators share the same cmake configure step, the DEB package would get lib/13/ instead of the expected lib/ layout. This is a real defect on a reachable user-facing code path (--pkg-generator DEB,TGZ), and no existing test exercises the DEB output path in a mixed build to catch the regression. utilities/cli/holohub.py (the HOLOHUB_PKG_TGZ=ON flag assignment) and cmake/modules/holohub_configure_tgz.cmake (the CMAKE_INSTALL_LIBDIR override and EXPORT_NAME path logic) warrant the most attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["holohub package module\n--pkg-generator DEB,TGZ"] --> B{cpack_generators\nnon-empty?}
B -- yes --> C["cmake configure\n-DBUILD_ALL=OFF -DMODULE_*=ON"]
C --> D{TGZ in\ngenerators?}
D -- yes --> E["Append -DHOLOHUB_PKG_TGZ=ON\nforces CMAKE_INSTALL_LIBDIR\n= lib/cuda_major/ globally"]
D -- no --> F["CMAKE_INSTALL_LIBDIR\n= lib/ (default)"]
E --> G["cmake build"]
F --> G
G --> H["Scan pkg/CPackConfig-*.cmake"]
H --> I["Classify: gen-specific vs base"]
I --> J{for each generator}
J --> K["DEB uses base config"]
J --> L["TGZ uses TGZ-specific config"]
K --> M["cpack -G DEB\nlib path = lib/13/ if TGZ also requested"]
L --> N["cpack -G TGZ\nlib path = lib/13/"]
B -- WHEEL only --> O["python -m build --wheel"]
Reviews (1): Last reviewed commit: "Module: Fix wheel rpath and scope KitMak..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if "TGZ" in cpack_generators: | ||
| cmake_args.append("-DHOLOHUB_PKG_TGZ=ON") |
There was a problem hiding this comment.
-DHOLOHUB_PKG_TGZ=ON is appended whenever TGZ is among the requested generators, which causes holohub_configure_tgz to force CMAKE_INSTALL_LIBDIR to lib/<cuda_major>/ for the entire configure step. All install() rules using ${CMAKE_INSTALL_LIBDIR} — including those for the DEB component — then install the library to lib/13/ instead of the expected lib/. Running --pkg-generator DEB,TGZ therefore silently produces a malformed Debian package with the wrong library path.
| NAMESPACE holoscan:: | ||
| COMPONENT ${export_component} | ||
| ) | ||
| include(CMakePackageConfigHelpers) | ||
| configure_package_config_file("${CMAKE_CURRENT_FUNCTION_LIST_DIR}/Config.cmake.in" | ||
| "${CMAKE_CURRENT_BINARY_DIR}/${ARG_NAME}Config.cmake" | ||
| INSTALL_DESTINATION ${config_install_dir} | ||
| NO_SET_AND_CHECK_MACRO | ||
| NO_CHECK_REQUIRED_COMPONENTS_MACRO | ||
| ) | ||
| write_basic_package_version_file( | ||
| "${CMAKE_CURRENT_BINARY_DIR}/${ARG_NAME}ConfigVersion.cmake" | ||
| VERSION "${ARG_VERSION}" | ||
| COMPATIBILITY AnyNewerVersion | ||
| ) | ||
| install(FILES | ||
| ${CMAKE_CURRENT_BINARY_DIR}/${ARG_NAME}Config.cmake | ||
| ${CMAKE_CURRENT_BINARY_DIR}/${ARG_NAME}ConfigVersion.cmake | ||
| DESTINATION ${config_install_dir} | ||
| COMPONENT ${export_component} | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
EXPORT_NAME cmake config path diverges from CMAKE_INSTALL_LIBDIR
When HOLOHUB_PKG_TGZ=ON, the CMAKE_INSTALL_LIBDIR override runs at the top of the function and sets it to lib/<cuda_major>/. The EXPORT_NAME block then hardcodes config_install_dir to lib/cmake/${ARG_NAME}, which no longer matches ${CMAKE_INSTALL_LIBDIR}/cmake/${ARG_NAME}. Any downstream consumer using find_package() against the TGZ tree would fail because the library is at lib/13/ but the config is at lib/cmake/. Consider using ${CMAKE_INSTALL_LIBDIR}/cmake/${ARG_NAME} for config_install_dir when in TGZ mode.
| # Only active when the CLI sets HOLOHUB_PKG_TGZ=ON (i.e. --pkg-generator TGZ); | ||
| # all other builds (normal installs, DEB, wheel) keep the default lib/ path. | ||
| if(HOLOHUB_PKG_TGZ) | ||
| find_package(CUDAToolkit QUIET) | ||
| if(CUDAToolkit_FOUND) | ||
| set(CMAKE_INSTALL_LIBDIR "lib/${CUDAToolkit_VERSION_MAJOR}" | ||
| CACHE STRING "Library install directory (KitMaker multi-variant layout)" FORCE) | ||
| endif() |
There was a problem hiding this comment.
The CMAKE_INSTALL_LIBDIR cache override with FORCE executes unconditionally (when HOLOHUB_PKG_TGZ is set) before the required-argument check that follows. If a caller omits NAME or VERSION, the global cache is already mutated before message(FATAL_ERROR) fires. While FATAL_ERROR prevents the build from proceeding, a partially modified cache can cause surprising behavior on the next reconfigure attempt. Consider moving argument validation before any side-effecting cache writes.
WalkthroughThis PR adds TGZ (tar.gz) archive packaging support to HoloHub. It introduces a CMake module for CPack TGZ configuration, applies TGZ packaging to the holoscan-gstreamer module, standardizes install component naming, updates CLI tooling to execute generator-specific packaging, and adds comprehensive test coverage for the new TGZ workflow. ChangesTGZ Packaging Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🧹 Nitpick comments (1)
utilities/cli/holohub.py (1)
482-487: ⚡ Quick winValidate
--pkg-generatorvalues at parse timeThe help text documents
DEB, TGZ, WHEEL, but invalid values currently pass through and fail later in packaging. Add explicit validation so users get immediate, actionable errors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utilities/cli/holohub.py` around lines 482 - 487, The --pkg-generator add_argument currently allows any string; update the parser call that defines "--pkg-generator" (dest="pkg_generator") to validate values at parse time by either (a) using argparse.choices for single-value input or (b) implementing a custom type/validator that splits the comma-separated string and verifies each token is one of {"DEB","TGZ","WHEEL"}, raising argparse.ArgumentTypeError on invalid entries; ensure the help text remains the same and the stored value is normalized (e.g., uppercase list or comma-joined string) so downstream packaging logic receives validated inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@utilities/cli/holohub.py`:
- Around line 2287-2288: The current logic sets HOLOHUB_PKG_TGZ in the global
cmake_args when "TGZ" is present in cpack_generators causing TGZ-specific
CMAKE_INSTALL_LIBDIR to leak into DEB builds; instead, detect TGZ in
cpack_generators and run a separate configure/generate/install flow for TGZ
without mutating the shared cmake_args used for DEB (i.e., do not append
"-DHOLOHUB_PKG_TGZ=ON" to the global cmake_args); update holohub.py to keep
cmake_args immutable for non-TGZ generators and create a copy or new
cmake_args_tgz with "-DHOLOHUB_PKG_TGZ=ON" only when invoking the TGZ
configure/generate steps so CMAKE_INSTALL_LIBDIR remains correct for DEB builds.
---
Nitpick comments:
In `@utilities/cli/holohub.py`:
- Around line 482-487: The --pkg-generator add_argument currently allows any
string; update the parser call that defines "--pkg-generator"
(dest="pkg_generator") to validate values at parse time by either (a) using
argparse.choices for single-value input or (b) implementing a custom
type/validator that splits the comma-separated string and verifies each token is
one of {"DEB","TGZ","WHEEL"}, raising argparse.ArgumentTypeError on invalid
entries; ensure the help text remains the same and the stored value is
normalized (e.g., uppercase list or comma-joined string) so downstream packaging
logic receives validated inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1ed2b25-7e13-49c0-8453-43af4f58a33a
📒 Files selected for processing (6)
CMakeLists.txtcmake/modules/holohub_configure_tgz.cmakemodules/holoscan-gstreamer/CMakeLists.txtoperators/gstreamer/CMakeLists.txtutilities/cli/holohub.pyutilities/cli/tests/test_cli.py
| if "TGZ" in cpack_generators: | ||
| cmake_args.append("-DHOLOHUB_PKG_TGZ=ON") |
There was a problem hiding this comment.
Split TGZ and non-TGZ configure paths to avoid mutating DEB layout
Line 2287 enables HOLOHUB_PKG_TGZ for the whole configure when --pkg-generator DEB,TGZ is used. Because TGZ mode forces CMAKE_INSTALL_LIBDIR globally during configure, the DEB package from the same build can inherit TGZ-specific install paths. This breaks generator isolation.
Suggested fix direction
- if "TGZ" in cpack_generators:
- cmake_args.append("-DHOLOHUB_PKG_TGZ=ON")
- holohub_cli_util.run_command(cmake_args, ...)
- holohub_cli_util.run_command(build_cmd, ...)
- # then cpack for all generators
+ # configure/build per generator class to isolate install layout
+ # (e.g., one build dir with HOLOHUB_PKG_TGZ=ON for TGZ, another without it for DEB/RPM/ZIP)
+ # then run cpack against each generator’s corresponding config set🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@utilities/cli/holohub.py` around lines 2287 - 2288, The current logic sets
HOLOHUB_PKG_TGZ in the global cmake_args when "TGZ" is present in
cpack_generators causing TGZ-specific CMAKE_INSTALL_LIBDIR to leak into DEB
builds; instead, detect TGZ in cpack_generators and run a separate
configure/generate/install flow for TGZ without mutating the shared cmake_args
used for DEB (i.e., do not append "-DHOLOHUB_PKG_TGZ=ON" to the global
cmake_args); update holohub.py to keep cmake_args immutable for non-TGZ
generators and create a copy or new cmake_args_tgz with "-DHOLOHUB_PKG_TGZ=ON"
only when invoking the TGZ configure/generate steps so CMAKE_INSTALL_LIBDIR
remains correct for DEB builds.
|
Moving CLI updates to holoscan-cli: nvidia-holoscan/holoscan-cli#188 |
Migrate TGZ generator support from holohub-internal tbirdso/holoscan-gstreamer-tgz branch to holoscan-cli: - Add TGZ to --pkg-generator help text alongside DEB and WHEEL - Pass -DHOLOHUB_PKG_TGZ=ON to cmake configure when TGZ is requested; this lets modules set CMAKE_INSTALL_LIBDIR to lib/<cuda_major>/ for the KitMaker multi-variant layout without affecting DEB/WHEEL builds - Route each generator to its matching CPackConfig-*-<GEN>.cmake if one exists, falling back to the base CPackConfig-*.cmake; this lets modules emit generator-specific CPack configs with different install layouts - Synthesize generator-specific filenames in dry-run so routing is consistent with real builds - Hard-fail with clear messages when no CPack configs were generated or when the requested generator has no matching config - Add six unit tests covering the new TGZ paths and error cases Ported from: nvidia-holoscan/holohub#1604 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Tom Birdsong <tbirdsong@nvidia.com>
tbirdso
changed the title
2 participants
Goal
Changes
CLI
Holoscan GStreamer
Results
Details
Details
Summary by CodeRabbit
New Features
Chores