feat: add --install-args to forward arguments to cmake --install - #217
feat: add --install-args to forward arguments to cmake --install#217wyli wants to merge 3 commits into
cmake --install#217Conversation
`holoscan install` emitted a bare `cmake --install <build-dir>`, so the only way to pick a destination was `--configure-args="-DCMAKE_INSTALL_PREFIX=..."`. That bakes the path into the build tree's CMake cache: a configured tree can only ever install to one location, and changing it forces a reconfigure. Add `--prefix`, forwarded to `cmake --install --prefix`. It is resolved at install time and leaves the cache untouched, so one built tree can be installed to several destinations. The flag is spelled to match CMake's own option; internally it is `install_prefix` so it cannot be confused with the unrelated `cli.prefix` metadata placeholder prefix used in the same module. Both paths are covered: the local branch appends the option to the cmake invocation, and the container branch forwards it (shell-quoted) into the recursive in-container `holoscan install ... --local` command. Signed-off-by: Wenqi Li <wenqil@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
WalkthroughThe install command accepts repeatable ChangesInstall argument handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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
🤖 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 `@src/holoscan_cli/commands/install.py`:
- Around line 105-113: Update the --prefix help text in the argument parser to
say the path is resolved inside the container when installation runs in a
container, replacing the misleading “Without --local” condition. Keep the
existing forwarding and install-time behavior descriptions unchanged.
🪄 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: 76335b81-de2d-4396-b8d8-dd5ca4ba2b00
📒 Files selected for processing (2)
src/holoscan_cli/commands/install.pytests/unit/test_lifecycle_commands.py
| parser.add_argument( | ||
| "--prefix", | ||
| dest="install_prefix", |
There was a problem hiding this comment.
Rather than overfitting to --prefix, could we expose a more general args forwarding approach as done for configuration args?
See other valid args: https://cmake.org/cmake/help/latest/manual/cmake.1.html#install-a-project . --component in particular might be useful for installing build subsets into particular paths.
There was a problem hiding this comment.
thanks, adding support of --install-args='--prefix /opt/holohub' --install-args=--strip.
Do we need --prefix=/opt/holohub as a shortcut of --install-args='--prefix /opt/holohub'? worth adding if it's a frequently used parameter
There was a problem hiding this comment.
My guess would be that it is not common enough to want it explicitly surfaced in the CLI, but @agirault might be best to confirm
wyli
changed the title
cmake --install
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/test_lifecycle_commands.py (2)
146-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd parser-level coverage for repeatable
--install-args.
_project_argsinjectsinstall_argsdirectly intoNamespace, so these handler tests pass even if the real parser does not register the option. Parse two--install-argsvalues through the install parser and assert that both values are retained.🤖 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 `@tests/unit/test_lifecycle_commands.py` at line 146, Add parser-level coverage in the install parser tests for repeatable --install-args: parse two occurrences through the real parser and assert both values are retained in the resulting Namespace. Do not rely on _project_args injecting install_args directly; exercise the parser registration and append behavior instead.
508-534: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the local forwarding test.
The test uses literal paths and leaves
configure_argsasNone. It does not verifyos.path.expandvarsor prove that existing configuration arguments remain unchanged. Use a quoted environment variable containing spaces, pass a non-emptyconfigure_argslist, and assert both results.🤖 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 `@tests/unit/test_lifecycle_commands.py` around lines 508 - 534, The test test_handle_install_local_forwards_install_args_at_install_time should use a quoted install argument containing an environment variable whose expanded value includes spaces, and pass a non-empty configure_args list through _project_args. Set the environment variable with monkeypatch, then assert calls contains the correctly expanded cmake install tokens and build_kwargs["configure_args"] exactly preserves the original configuration arguments.
🤖 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.
Nitpick comments:
In `@tests/unit/test_lifecycle_commands.py`:
- Line 146: Add parser-level coverage in the install parser tests for repeatable
--install-args: parse two occurrences through the real parser and assert both
values are retained in the resulting Namespace. Do not rely on _project_args
injecting install_args directly; exercise the parser registration and append
behavior instead.
- Around line 508-534: The test
test_handle_install_local_forwards_install_args_at_install_time should use a
quoted install argument containing an environment variable whose expanded value
includes spaces, and pass a non-empty configure_args list through _project_args.
Set the environment variable with monkeypatch, then assert calls contains the
correctly expanded cmake install tokens and build_kwargs["configure_args"]
exactly preserves the original configuration arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b5e8f8c-d6d9-40dd-b11d-485fbfc8e3b1
📒 Files selected for processing (2)
src/holoscan_cli/commands/install.pytests/unit/test_lifecycle_commands.py
2 participants
Problem
holoscan installemits a barecmake --install <build-dir>with no way to pass options to it. The only way to influence the install today is through the configure stage:That bakes the destination into the build tree's CMake cache, so a configured tree can only ever install to one location and retargeting it forces a reconfigure. It also cannot reach the rest of
cmake --install's options at all —--strip,--component,--default-directory-permissions.Change
Add
--install-args, mirroring the existing--configure-args: repeatable, forwarded verbatim tocmake --install.$ holoscan install smoke_app --local --dryrun \ --install-args='--prefix /opt/holohub' --install-args=--strip cmake --install <root>/build/smoke_app \ --prefix /opt/holohub \ --stripCMake resolves these at install time, so the build tree's cache is left untouched and one built tree can be installed repeatedly to different destinations without reconfiguring:
That second invocation is the case the configure-time approach cannot express.
Implementation notes
--configure-args— sameaction="append"shape, sameos.path.expandvarstreatment, so the two stages behave alike.shlex.split.--configure-argsappends each value as exactly one token, which suits-DFOO=ONbut notcmake --install's separate-value options —--component devis two tokens.shlex.splithandles both, and is identical for single-token values (shlex.split('--strip')is['--strip']). It differs only when a value contains quotes or shell metacharacters.shlex.quoted into the recursive in-containerholoscan install ... --localcommand, alongside the existing--configure-argsforwarding. Round-trip verified: the branch emits--install-args='--component dev', a shell re-splits that to the single argv token--install-args=--component dev, and the in-containershlex.splitrestores['--component', 'dev'].--configure-args, values are passed through as given;cmake --installis the one that rejects bad input.Notes for review
--prefixflag instead. That was replaced with the general forwarding mechanism per review feedback; no bespoke per-option flags remain.--install-argsvalues are appended in order. This matches the existing overlap between--build-typeand--configure-args.--local— or when a mode's env selects a local build — forwarded paths are resolved inside the container, so a destination must be a mounted path. Called out in the flag's help text.--devreturns early and never reachescmake --install, so--install-argsis accepted but unused on that path.Testing
Both existing install tests extended rather than new ones added: the local-path test pins the full emitted token list including ordering and asserts nothing leaks into the configure step; the container-path test asserts the quoted forwarding for a value containing a space.
Full unit suite: 420 passed, 1 skipped.
ruff,black,isortand codespell clean on both changed files.🤖 Generated with Claude Code