Hybrid RTEMS5 IOC support (ibek+msi runtime generation, NFS layout, autosave)#58
Merged
Conversation
New hybrid.py module handles the full sequence: linking ibek-support YAMLs, running ibek generate2, expanding ioc.subst with msi, and placing all files on NFS/TFTP. The existing in-container path is unchanged. Fixes #56 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… testing Extracts env vars from services repo values.yaml so manual exports are unnecessary. Also symlinks instance config, creates missing directories, sources full msi.vars for macro resolution, and refactors dev command to share the values.yaml parsing logic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Legacy-built RTEMS IOCs carry two genuinely different names:
* the build folder basename (IOC_ORIGINAL_LOCATION.name), which the
source .boot binary is named after, e.g. bl-va-ioc-01 ->
BL-VA-IOC-01.boot
* the deployment/instance name (IOC_NAME), used for the TFTP/NFS/boot
paths, e.g. bl19i-va-ioc-01
Previously IOC_NAME was derived from the build folder basename, so the
TFTP boot path, NFS mount and rtems-client-name all resolved to the
wrong name (/iocs/bl-va-ioc-01/rtems.ioc.bin).
* __main__.py: set IOC_NAME from the services instance folder name.
* globals.py: add IOC_BUILD_NAME (build folder basename) for locating
source binaries.
* copy.py, hybrid.py: name the source .boot from IOC_BUILD_NAME.
* configure.py: correct the NFS mount point (/epics_rtems_root ->
/epics).
* tests/test_globals.py: regression test pinning IOC_BUILD_NAME apart
from IOC_NAME for the bl-va-ioc-01 / bl19i-va-ioc-01 case.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The RTEMS crate NFS-mounts the export root at /epics, and the generated
st.cmd references fixed paths within it:
cd "/epics/ioc"; dbLoadDatabase dbd/ioc.dbd
STREAM_PROTOCOL_PATH /epics/runtime/protocol/
set_requestfile_path("/epics", "runtime") # autosave .req files
epics-script = /epics/runtime/st.cmd # from RTEMS_EPICS_SCRIPT
_copy_to_nfs previously placed everything flat in the NFS root, so st.cmd,
ioc.db, dbd/ and protocol/ landed at the wrong paths. Split the copy into:
runtime/ st.cmd, ioc.db, protocol/ (data/*.proto*), autosave *.req
ioc/ dbd/
The bulk build-time data/ folder is no longer copied (only its protocol
files are needed at runtime). save_current_version/check_new_version now
read and write rtems_proxy_version.txt under runtime/ to match.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ibek's st.cmd template renders the database load line as
'dbLoadRecords {{ get_env("RUNTIME_DIR") }}/ioc.db'. get_env has no
default, so with RUNTIME_DIR unset it emitted 'dbLoadRecords /ioc.db' --
the wrong absolute path on the crate.
Pass RUNTIME_DIR=GLOBALS.RUNTIME (/epics/runtime) to the ibek generate2
subprocess so the line renders as 'dbLoadRecords /epics/runtime/ioc.db',
matching where _copy_to_nfs now places ioc.db.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Step 4 of the worked example now covers pushing the generated runtime
files onto the real NFS export, mirroring the one-off TFTP push: empty
/dls_sw/i19/epics/rtems/bl19i-va-ioc-01 then copy /ioc_nfs in. Emptying
first is called out because the new runtime/+ioc/ layout would otherwise
leave stale flat files (e.g. an st.cmd pointing at the retired
/epics_rtems_root). The split-access caveat (devcontainer has the source
but read-only /dls_sw; workstation has write but no /ioc_nfs) is noted as
a known rough edge.
Also brings the docs in line with the recent code changes:
* IOC_NAME now comes from the instance folder (bl19i-va-ioc-01); the
.boot binary is located via IOC_BUILD_NAME (bl-va-ioc-01).
* NFS artefacts now live under /ioc_nfs/runtime and /ioc_nfs/ioc.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The generic-IOC project now builds with a fixed PROD name `ioc` (src/Makefile `PROD_IOC = ioc`, main source renamed to iocMain.cpp), so it emits bin/RTEMS-beatnik/ioc and ioc.boot regardless of the IOC instance name. rtems-proxy previously derived the boot image name from the (uppercased) build folder, e.g. BL-VA-IOC-01.boot, which no longer exists. Look for the generic ioc / ioc.boot names instead in both the TFTP copy paths (hybrid.py, copy.py), update the IOC_BUILD_NAME docstring, drop the stale .boot-name test assertion, and document the generic-IOC Makefile/iocMain.cpp setup in the docs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extend the rtems-hybrid-debug skill with: a "Regenerating & deploying the runtime" section noting that `msi` must be on PATH (lives at /epics/epics-base/bin/linux-x86_64/msi, symlinked into .venv/bin) or the generate step fails before the NFS copy; and the mapped-enum vs template mismatch that caused the DLS8515 parity asynSetOption to be dropped, so 7E2 gauges silently ran 7N2 and timed out. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a complete RTEMS hybrid IOC preparation pipeline ( ChangesHybrid Mode Pipeline
rtems-hybrid-debug Skill Document
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer / CLI
participant Main as start --hybrid --instance
participant Env as _load_instance_env()
participant HP as hybrid_prepare()
participant ibek as ibek runtime generate2
participant msi as msi macro expansion
participant NFS as NFS /epics
participant TFTP as TFTP /ioc_tftp
Dev->>Main: rtems-proxy start --hybrid --instance <ioc-path>
Main->>Env: _load_instance_env(ioc_path)
Env->>Env: read global/instance values.yaml
Env->>Env: set IOC_DOMAIN, IOC_NAME, os.environ
Env-->>Main: (env_vars, domain)
Main->>Main: reload_globals()
Main->>HP: hybrid_prepare(instance_path)
HP->>HP: _link_instance_config() → config/
HP->>HP: _link_ibek_support_yamls() → *.ibek.support.yaml
HP->>ibek: _run_ibek_generate() → st.cmd, ioc.subst
HP->>msi: _run_msi() → ioc.db from ioc.subst
HP->>ibek: _run_ibek_autosave() → *.req autosave files
HP->>NFS: _copy_to_nfs() → st.cmd, ioc.db, proto, dbd/, *.req
HP->>TFTP: _copy_binary_to_tftp() → ioc.boot → RTEMS_BINARY_DEFAULT_NAME
HP-->>Main: artifacts staged
Main->>Main: connect + reboot (unless --no-connect)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Dockerfile (1)
41-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop root runtime by declaring a non-root
USER.The image currently runs as root (Line 41 onward), and static analysis correctly flags this. Add a dedicated unprivileged user and run
rtems-proxyunder it.Suggested patch
FROM ghcr.io/epics-containers/epics-base-runtime:7.0.10ec1 AS runtime @@ RUN mkdir -p /epics/ibek-defs /epics/runtime /epics/autosave \ && chmod a+rwx /epics/ibek-defs /epics/runtime /epics/autosave + +RUN useradd --create-home --uid 10001 --shell /usr/sbin/nologin rtemsproxy +USER 10001🤖 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 `@Dockerfile` around lines 41 - 63, The runtime stage in the Dockerfile currently runs as root, which is a security risk. After the directory permission setup (the chmod commands in the RUN instruction that creates /epics/ibek-defs, /epics/runtime, and /epics/autosave directories), add a new RUN instruction to create a dedicated non-root user, then add a USER instruction before the ENTRYPOINT declaration to ensure the rtems-proxy application runs under that unprivileged user instead of root.Source: Linters/SAST tools
src/rtems_proxy/configure.py (1)
44-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
IOC_DOMAINandIOC_NAMEbefore writingepics-nfsmount.Line 70 now unconditionally builds mount from
IOC_DOMAIN/IOC_NAME. IfIOC_DOMAINis unset (orIOC_NAMEis default placeholder), the crate gets an invalid NFS path.Proposed fix
if not self.dry_run: for v in [ GLOBALS.RTEMS_IOC_NETMASK, GLOBALS.RTEMS_IOC_GATEWAY, GLOBALS.RTEMS_IOC_IP, GLOBALS.RTEMS_NFS_IP, GLOBALS.RTEMS_TFTP_IP, ]: if v is None or v == "": raise ValueError( "RTEMS_IOC_NETMASK, RTEMS_IOC_GATEWAY, RTEMS_IOC_IP, " "RTEMS_NFS_IP, and RTEMS_TFTP_IP must be set" ) + if not GLOBALS.IOC_DOMAIN: + raise ValueError("IOC_DOMAIN must be set") + if not GLOBALS.IOC_NAME or GLOBALS.IOC_NAME == "NO_IOC_NAME": + raise ValueError("IOC_NAME must be set to a real instance name")Also applies to: 70-84
🤖 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 `@src/rtems_proxy/configure.py` around lines 44 - 56, Add validation for GLOBALS.IOC_DOMAIN and GLOBALS.IOC_NAME to the existing validation block (in the if not self.dry_run section where RTEMS_IOC_NETMASK, RTEMS_IOC_GATEWAY, RTEMS_IOC_IP, RTEMS_NFS_IP, and RTEMS_TFTP_IP are checked). Include these two variables in the list being validated to ensure they are not None and not empty strings before they are used to build the NFS mount path in the code that follows (around line 70-84). Update the ValueError message to include IOC_DOMAIN and IOC_NAME in the list of required variables.src/rtems_proxy/copy.py (1)
58-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid creating a self-referential
st.cmdsymlink.If no startup script is found (Line 59),
ioc_script_nameisst.cmd; Line 84 then linksst.cmdto itself, producing a broken startup script path.Proposed fix
if len(sts) == 0: ioc_script_name = "st.cmd" @@ - ioc_script_path = Path(local_nfs_root) / GLOBALS.RTEMS_SCRIPT_DEFAULT_NAME - ioc_script_path.unlink(missing_ok=True) - ioc_script_path.symlink_to(Path(local_nfs_root) / ioc_script_name) + if ioc_script_name != GLOBALS.RTEMS_SCRIPT_DEFAULT_NAME: + ioc_script_path = Path(local_nfs_root) / GLOBALS.RTEMS_SCRIPT_DEFAULT_NAME + ioc_script_path.unlink(missing_ok=True) + ioc_script_path.symlink_to(Path(local_nfs_root) / ioc_script_name)🤖 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 `@src/rtems_proxy/copy.py` around lines 58 - 85, The code creates a self-referential symlink when no startup scripts are found. When len(sts) == 0, ioc_script_name is set to "st.cmd", and then line 85 creates a symlink from st.cmd to itself via ioc_script_path.symlink_to(). Fix this by adding a conditional check before the symlink creation to only create the symlink if ioc_script_name differs from GLOBALS.RTEMS_SCRIPT_DEFAULT_NAME, preventing the self-referential link.
🤖 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 @.claude/hooks/sandbox-check.sh:
- Around line 22-23: The sandbox-check.sh file at lines 22-23 is currently
testing for file existence and size, but the stated policy is that gitconfig
must NOT be readable. Replace the existence check with existence (`-e`) AND
readability check (`-r`) to properly enforce that the file is not readable.
Change the test to fail when the file exists AND is readable, explicitly using
the `-r` flag to test readability permission instead of checking file size.
- Line 16: The sandbox-check.sh script at line 16 currently bypasses all sandbox
checks using only the CLAUDE_CODE_REMOTE environment variable. Replace this
single-factor bypass with a multi-factor verification strategy by combining the
CLAUDE_CODE_REMOTE check with additional runtime signals such as validating the
presence of CLAUDE_CODE_REMOTE_SESSION_ID. Implement a layered verification
approach in the conditional check that requires multiple signals to be satisfied
before skipping sandbox validation, rather than allowing a single environment
variable to unconditionally bypass security-critical checks.
In @.claude/skills/rtems-hybrid-debug/SKILL.md:
- Around line 73-79: Add language identifiers to the fenced code blocks in the
file .claude/skills/rtems-hybrid-debug/SKILL.md. At lines 73-79 (anchor site)
and lines 241-244 (sibling site), the markdown code fences are unlabeled. Add an
appropriate language identifier (such as "shell" or "bash") immediately after
the opening triple backticks on each code block to enable proper syntax
highlighting and satisfy markdown linting requirements.
- Around line 167-171: The documentation in
.claude/skills/rtems-hybrid-debug/SKILL.md at lines 167-171 incorrectly
describes the permission mode created by install -m 644 as
"world-readable+writable". The file permission mode 644 actually creates
owner-writable and world-readable files, not world-writable. Correct the wording
in that section to accurately reflect that install -m 644 results in
owner-writable, world-readable (but not world-writable) files, which is what the
NFS root-squash export requires.
In @.devcontainer/devcontainer.json:
- Around line 73-76: The mount configuration is using object-form syntax
incorrectly by embedding mount options (readonly and bind-propagation=slave)
within the target field value. Convert this mount entry to string-form syntax
where all mount parameters (type, source, target, and options) are combined into
a single comma-separated string value. Replace the current object with three
properties (type, source, target) with a single string property containing all
mount specifications in the format:
type=bind,source=/dls_sw,target=/dls_sw,readonly,bind-propagation=slave.
In @.github/workflows/_dist.yml:
- Line 35: The `actions/setup-python` action on line 35 is referenced using the
mutable tag `@v5` instead of a pinned commit SHA. Replace `@v5` with a full
40-character commit SHA to pin the action to a specific immutable version. This
ensures the workflow always uses the exact version intended and closes the
supply-chain security gap.
In `@Dockerfile`:
- Line 30: The mkdir command creates a directory named /ioc_nfsv2, but the code
in globals.py resolves the NFS root path as /ioc_nfs, causing a mismatch. Update
the RUN mkdir command to create /ioc_nfs instead of /ioc_nfsv2 to align with the
runtime contract expected by the application.
In `@docs/hybrid.md`:
- Around line 228-283: The documentation for the `hybrid_prepare()` function is
missing the `_run_ibek_autosave()` step which generates autosave request files.
Insert a new step 6 between the current "Run msi" step and the "Copy to NFS"
step with the title "Run ibek runtime generate-autosave" that describes how this
step parses ioc.subst, matches DB templates to req files, and expands them with
instance macros into autosave_positions.req and autosave_settings.req in
/epics/runtime/. Renumber the existing "Copy to NFS" step to step 7 and "Copy
binary to TFTP" to step 8. In the manual debugging section, add corresponding
ibek runtime generate-autosave command after the msi expansion step and before
the verification step.
In `@src/rtems_proxy/hybrid.py`:
- Around line 211-255: The subprocess.run calls with check=True in the rsync
operations allow CalledProcessError and FileNotFoundError exceptions to
propagate as unhandled tracebacks instead of being normalized to a controlled
exit. Wrap all subprocess.run calls that perform rsync operations (the ones at
lines 211-255 copying st.cmd, ioc.db, protocol files, req files, and dbd
directory) and line 274 in try-except blocks that catch CalledProcessError and
FileNotFoundError, then convert these exceptions to typer.Exit(1) to maintain
consistent CLI behavior across all deployment steps.
- Around line 45-60: The symlink created by config_dst.symlink_to(config_src)
uses a potentially relative target path. When instance_path is a relative path
(from the --instance flag), config_src becomes relative and the symlink target
won't resolve correctly at runtime. Convert config_src to an absolute path using
the .resolve() method before passing it to symlink_to to ensure the symlink
always points to a valid absolute target regardless of how the instance path was
provided.
In `@src/rtems_proxy/rsync.sh.jinja`:
- Around line 15-23: The rsync.sh.jinja file has swapped variable assignments
where RTEMS_TFTP_IP is incorrectly set to a filesystem path value (/ioc_tftp)
instead of an IP address, while RTEMS_TFTP_PATH is never defined but used later
in mkdir and rsync operations. Fix this by assigning the filesystem path to
RTEMS_TFTP_PATH and assigning the proper IP address to RTEMS_TFTP_IP (which may
need to be templated or sourced from an environment variable). This ensures the
validation check correctly validates the IP address variable and the mkdir and
rsync operations use the correct path variable, preventing broken host
resolution and empty path issues.
In `@update-schema`:
- Around line 6-16: The script needs to be hardened to prevent continuation
after errors and to use proper output handling. Enable strict mode (set -e) at
the beginning of the script, quote all variable references including
IOC_ORIGINAL_LOCATION and EPICS_ROOT throughout, add validation to ensure the
glob pattern in the ln command matches at least one file before attempting the
symlink operation, and replace the shell output redirection (>) with the
documented --output flag in the ibek ioc generate-schema command to properly
handle the output and prevent noisy stdout from being written to
ioc.schema.json.
In `@vacuumSpace-fix.patch`:
- Around line 1-2: Remove the patch file vacuumSpace-fix.patch before merging,
as it is a temporary scratch artifact containing /tmp paths that duplicate
changes already present in the fixed YAML configuration. If documentation
explaining the fix is needed, add that rationale to the project documentation
directory instead and retain only the final production-ready YAML files in the
repository.
In `@vacuumSpace.ibek.support.yaml.fixed`:
- Around line 1-3: The file vacuumSpace.ibek.support.yaml.fixed does not match
the expected naming pattern for support YAML files. The
_link_ibek_support_yamls() function only symlinks files that match the pattern
*.ibek.support.yaml (without the .fixed suffix). Rename the file from
vacuumSpace.ibek.support.yaml.fixed to vacuumSpace.ibek.support.yaml to ensure
it is properly discovered and loaded during hybrid mode, or delete the file if
it is only a temporary artifact and not needed.
- Around line 242-390: The spaceGroup entity model declares parameters (device,
name, space0 through space7) and describes itself as a helper that creates a
group of spaces to represent a hutch, but all databases entries are commented
out. This means instantiating spaceGroup will appear to succeed during
configuration but will silently produce no actual IOC records. Either uncomment
and properly implement the databases entries in the spaceGroup entity model to
actually create the promised space group records, or remove the spaceGroup
entity entirely if it should not be a public entity. Choose the approach based
on whether this entity is intended to be functional.
- Around line 427-429: The template file `$(VACUUMSPACE)/db/space.template` is
instantiated with inconsistent argument orders in two different places (one with
`device, ionp, gauge, img, pirg, valve` and another with `device, name, ionp,
gauge, img, pirg, valve`), and `$(VACUUMSPACE)/db/space_b.template` is
instantiated with significantly different and incompatible argument sequences
across two locations. Since ibek merges rows by template path and appends values
positionally, these mismatches cause data to shift into wrong columns.
Standardize each template path to use a single consistent argument sequence
across all its instantiations: decide on the canonical arg order for
`space.template` and update all its usages to match, then do the same for
`space_b.template` (and determine whether `SELM` and `SELN` are required
parameters for `space_b.template` to include or exclude them consistently).
---
Outside diff comments:
In `@Dockerfile`:
- Around line 41-63: The runtime stage in the Dockerfile currently runs as root,
which is a security risk. After the directory permission setup (the chmod
commands in the RUN instruction that creates /epics/ibek-defs, /epics/runtime,
and /epics/autosave directories), add a new RUN instruction to create a
dedicated non-root user, then add a USER instruction before the ENTRYPOINT
declaration to ensure the rtems-proxy application runs under that unprivileged
user instead of root.
In `@src/rtems_proxy/configure.py`:
- Around line 44-56: Add validation for GLOBALS.IOC_DOMAIN and GLOBALS.IOC_NAME
to the existing validation block (in the if not self.dry_run section where
RTEMS_IOC_NETMASK, RTEMS_IOC_GATEWAY, RTEMS_IOC_IP, RTEMS_NFS_IP, and
RTEMS_TFTP_IP are checked). Include these two variables in the list being
validated to ensure they are not None and not empty strings before they are used
to build the NFS mount path in the code that follows (around line 70-84). Update
the ValueError message to include IOC_DOMAIN and IOC_NAME in the list of
required variables.
In `@src/rtems_proxy/copy.py`:
- Around line 58-85: The code creates a self-referential symlink when no startup
scripts are found. When len(sts) == 0, ioc_script_name is set to "st.cmd", and
then line 85 creates a symlink from st.cmd to itself via
ioc_script_path.symlink_to(). Fix this by adding a conditional check before the
symlink creation to only create the symlink if ioc_script_name differs from
GLOBALS.RTEMS_SCRIPT_DEFAULT_NAME, preventing the self-referential link.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3bf5d9f-3f26-4311-a7be-0af37ea1e925
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.claude/hooks/sandbox-check.sh.claude/settings.json.claude/skills/rtems-hybrid-debug/SKILL.md.claude/statusline-command.sh.devcontainer/devcontainer.json.github/workflows/_dist.yml.github/workflows/ci.yml.gitignore.gitmodules.python-versionDockerfileREADME.mddocs/example-bl19i-va-ioc-01.mddocs/hybrid.mddocs/overview.mdpyproject.tomlrtems-proxy.code-workspacesrc/rtems_proxy/__main__.pysrc/rtems_proxy/configure.pysrc/rtems_proxy/copy.pysrc/rtems_proxy/globals.pysrc/rtems_proxy/hybrid.pysrc/rtems_proxy/rsync.sh.jinjatests/test_globals.pyupdate-schemavacuumSpace-fix.patchvacuumSpace.ibek.support.yaml.fixed
💤 Files with no reviewable changes (2)
- rtems-proxy.code-workspace
- .python-version
These were promoted claude-sandbox artifacts (sandbox-check.sh hook, statusline-command.sh, and the settings.json wiring them up). The sandbox approach is being replaced with a guest sandbox, so these no longer belong in the repo. Only the repo-specific rtems-hybrid-debug skill remains under .claude. Resolves CodeRabbit review comments on sandbox-check.sh by removal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Annotate the two unlabeled fenced code blocks (st.cmd<->NFS contract and the FINS init error output) with the `text` language so they pass markdownlint MD040. - Correct the protocols-target wording: `install -m 644` pins owner-writable, world-readable (0644) files, not world-writable. Addresses CodeRabbit review comments on SKILL.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The mkdir created /ioc_nfsv2 but globals.py resolves the NFS root as /ioc_nfs (RTEMS_NFS_ROOT_PATH), used consistently across docs and the hybrid code. Align the directory with the runtime contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The TFTP filesystem path was assigned to RTEMS_TFTP_IP, clobbering the real IP exported by the env_vars loop, while RTEMS_TFTP_PATH (used by mkdir/rsync/inotifywait) was never set. Assign the path to RTEMS_TFTP_PATH, leave RTEMS_TFTP_IP as the host, and add it to the validation guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A relative --instance path produced a relative symlink target at /epics/ioc/config that ibek could not follow. Resolve config_src to an absolute path and mark the symlink as a directory target. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The six rsync calls used check=True, letting CalledProcessError or a missing rsync escape as a traceback instead of the controlled exit used by every other step. Add a _run_checked helper following the existing check=False + return-code pattern and route all rsync calls through it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Enable strict mode, quote IOC_ORIGINAL_LOCATION/$0, guard the support YAML glob so an empty match fails loudly instead of creating a broken symlink, and write the schema via ibek's --output flag rather than stdout redirection (which could capture stray output). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The step-by-step list and manual-debug block omitted _run_ibek_autosave, which runs between msi expansion and the NFS copy. Document it as step 6 (renumbering copy-to-NFS and copy-binary) and add the equivalent manual commands. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vacuumSpace-fix.patch and vacuumSpace.ibek.support.yaml.fixed were scratch files from debugging (they reference /tmp paths and are used nowhere in the repo). The real fix lives in the ibek-support repos. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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/rtems_proxy/hybrid.py`:
- Around line 188-197: Replace the direct subprocess.run() calls in the
functions _run_ibek_generate, _run_msi, and _run_ibek_autosave with calls to
_run_checked to ensure consistent error handling. Each function should use
_run_checked instead of calling subprocess.run directly, passing the appropriate
command list and error message to maintain consistent error normalization across
all subprocess operations and prevent raw tracebacks from missing executables.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: df7df878-50f7-4506-8379-124708c32552
📒 Files selected for processing (5)
Dockerfiledocs/hybrid.mdsrc/rtems_proxy/hybrid.pysrc/rtems_proxy/rsync.sh.jinjaupdate-schema
✅ Files skipped from review due to trivial changes (1)
- docs/hybrid.md
🚧 Files skipped from review as they are similar to previous changes (2)
- update-schema
- Dockerfile
Extend _run_checked with **kwargs (for env=) and use it in _run_ibek_generate, _run_msi and _run_ibek_autosave so a missing executable exits via typer.Exit(1) like the rsync calls, instead of leaking a traceback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # uv.lock
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rtems_proxy/hybrid.py (1)
83-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on duplicate basenames when staging support/autosave links.
Both link steps flatten files by
.name; if two modules ship the same filename, the later symlink silently replaces the earlier one. That can drop required ibek defs or autosave req inputs and produce incomplete runtime artifacts.Suggested minimal guard
def _link_ibek_support_yamls(): @@ - for yaml_file in yaml_files: + seen_yaml: dict[str, Path] = {} + for yaml_file in yaml_files: + prev = seen_yaml.get(yaml_file.name) + if prev and prev != yaml_file: + typer.echo( + f"Duplicate ibek support filename '{yaml_file.name}': {prev} and {yaml_file}" + ) + raise typer.Exit(1) + seen_yaml[yaml_file.name] = yaml_file link = ibek_defs / yaml_file.name if link.exists(): link.unlink() link.symlink_to(yaml_file) @@ def _run_ibek_autosave(): @@ - for req in req_files: + seen_req: dict[str, Path] = {} + for req in req_files: + prev = seen_req.get(req.name) + if prev and prev != req: + typer.echo(f"Duplicate autosave req filename '{req.name}': {prev} and {req}") + raise typer.Exit(1) + seen_req[req.name] = req link = autosave_dir / req.name if link.exists() or link.is_symlink(): link.unlink() link.symlink_to(req)Also applies to: 165-168
🤖 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 `@src/rtems_proxy/hybrid.py` around lines 83 - 86, When staging support/autosave links, both symlink creation steps flatten files by basename without detecting duplicates. If two modules ship the same filename, the later symlink silently overwrites the earlier one, potentially dropping required ibek definitions or autosave inputs. Add a guard check before each link.symlink_to() call to detect if a symlink with the same basename already exists from a different source file, and raise an error immediately with a clear message identifying the conflicting files rather than allowing silent replacement. This guard should be applied at both symlink creation locations where files are being flattened by basename.
🤖 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.
Outside diff comments:
In `@src/rtems_proxy/hybrid.py`:
- Around line 83-86: When staging support/autosave links, both symlink creation
steps flatten files by basename without detecting duplicates. If two modules
ship the same filename, the later symlink silently overwrites the earlier one,
potentially dropping required ibek definitions or autosave inputs. Add a guard
check before each link.symlink_to() call to detect if a symlink with the same
basename already exists from a different source file, and raise an error
immediately with a clear message identifying the conflicting files rather than
allowing silent replacement. This guard should be applied at both symlink
creation locations where files are being flattened by basename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e1f94b6-0441-4fac-bd41-54ab5384cc4d
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
src/rtems_proxy/hybrid.py
3 tasks
|
File change |
6 similar comments
|
File change |
|
File change |
|
File change |
|
File change |
|
File change |
|
File change |
|
Change file |
|
File change |
|
Change file |
|
File change |
|
Files change |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds first-class support for hybrid RTEMS5 IOCs driven by rtems-proxy: legacy cross-compiled IOC binaries booted with an ibek/msi-generated runtime, laid out on NFS, and deployable from a Linux devcontainer.
Highlights
--hybridflag automates ibek + msi runtime generation;--instanceflag makes thestartcommand devcontainer-friendly.src/rtems_proxy/hybrid.py; IOC-name resolution fixes for legacy-built RTEMS IOCs; genericioc/ioc.bootbinary names.runtime/+ioc/subfolders;RUNTIME_DIRset sodbLoadRecordsresolves paths; filesystem-copy step for stale-export safety./epics/autosaveaccess fixes.Docs & skills
docs/example-bl19i-va-ioc-01.md),docs/hybrid.md,docs/overview.md.rtems-hybrid-debugskill (Makefile foot-guns, msi/regen recipe, ibek shared-template column-misalignment foot-gun).Note for reviewers
Two scratch artifacts are included and may warrant removal before merge:
vacuumSpace-fix.patchandvacuumSpace.ibek.support.yaml.fixed.🤖 Generated with Claude Code
Summary by CodeRabbit
--hybridand--instanceto generate and stage runtime artifacts from instancevalues.yamlvalues.yaml