Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Dockerfiles move TTS runtime preparation into a new ChangesTTS Runtime Image Multistage Build Refactoring
Workers image LD_LIBRARY_PATH change
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Web/Resgrid.Web.Tts/Dockerfile (1)
38-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback symlink branch will produce a broken
espeak-ng-datain the final image.When the Piper tarball does not include
espeak-ng-data, theelsebranch creates a symlink pointing to/usr/lib/x86_64-linux-gnu/espeak-ng-data(provided bylibespeak-ng1). That target only exists in thepublishstage — the final stage’s base image does not installlibespeak-ng1, andCOPYpreserves symlinks rather than dereferencing directory symlinks across stages, so the final image ends up with a dangling/usr/share/espeak-ng-dataand Piper will fail to find phoneme data at runtime.Either resolve the symlink at copy time (e.g.
cp -RLthe data into a real directory inpublishbefore the final COPY) or assert that the Piper bundle containsespeak-ng-dataand fail the build otherwise.🛡️ Proposed fix (materialize data in publish stage)
- && if [ -d /tmp/piper/espeak-ng-data ]; then cp -R /tmp/piper/espeak-ng-data /usr/share/; else ln -sf /usr/lib/x86_64-linux-gnu/espeak-ng-data /usr/share/espeak-ng-data; fi \ + && if [ -d /tmp/piper/espeak-ng-data ]; then \ + cp -R /tmp/piper/espeak-ng-data /usr/share/; \ + elif [ -d /usr/lib/x86_64-linux-gnu/espeak-ng-data ]; then \ + cp -R /usr/lib/x86_64-linux-gnu/espeak-ng-data /usr/share/espeak-ng-data; \ + else \ + echo "espeak-ng-data not found" >&2; exit 1; \ + fi \Also applies to: 70-70
🤖 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 `@Web/Resgrid.Web.Tts/Dockerfile` at line 38, The fallback branch in the Dockerfile line that currently does "if [ -d /tmp/piper/espeak-ng-data ]; then cp -R /tmp/piper/espeak-ng-data /usr/share/; else ln -sf /usr/lib/x86_64-linux-gnu/espeak-ng-data /usr/share/espeak-ng-data; fi" produces a dangling symlink in the final image because /usr/lib/... only exists in the publish stage; change this to materialize a real directory in the publish stage or fail the build: detect absence of /tmp/piper/espeak-ng-data and then copy (dereference) the espeak-ng-data contents from the publish-stage location into /usr/share/espeak-ng-data (e.g., use a non-symlink copy such as cp -a or cp -R with dereference) or exit with an error if neither source exists so the final image contains real data rather than a symlink.
🧹 Nitpick comments (3)
Web/Resgrid.Web.Tts/Dockerfile (3)
25-55: 💤 Low valueSingle mega-
RUNcouples unrelated concerns; consider splitting for cache locality.This 30-line
RUNdoes apt install, Piper extraction, espeak data placement, voice-model download, user creation, and dependency collection in one shot. Any change to the voice list (likely the most-edited part) invalidates the apt + Piper layers and re-pays the install cost on every rebuild. Splitting into ~3RUNs (apt + Piper binary, voice downloads, user + lib collection) would give much better layer caching during iteration without changing image size meaningfully.Not blocking — purely a build-time ergonomics improvement.
🤖 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 `@Web/Resgrid.Web.Tts/Dockerfile` around lines 25 - 55, The big RUN that handles apt installs, Piper extraction, espeak data handling, voice-model downloads and cleanup should be split into separate RUNs to improve layer caching: 1) a RUN that does apt-get update/install (ffmpeg, libespeak-ng1, ca-certificates, curl), creates /usr/local/share/piper-voices, downloads and installs the Piper binary using PIPER_VERSION and performs the espeak-ng-data copy/link and ldconfig, then cleans /tmp; 2) a separate RUN that performs the voice-model curl loop writing to /usr/local/share/piper-voices (the list of voice paths and the curl --retry invocations); and 3) a final RUN for any user creation, permissions (chmod +x /usr/local/bin/piper), and any remaining lib copying (find /tmp/piper -name '*.so*' -> /usr/local/lib) if still needed—preserve command order and cleanup (rm -rf) in each RUN to keep image size unchanged.
59-63: ⚡ Quick win
lddscan is missingffprobeand parses "not found" lines as paths.Two small issues in the dependency-collection loop:
ffprobeis copied into the final image (line 72) but is not included in thefor bin in …list, so any library it needs that piper/ffmpeg do not pull in will be absent at runtime. In practice ffprobe’s deps are a subset of ffmpeg’s, but it is safer to include it explicitly.awk '/=>/ {print $3}'also matcheslib… => not foundlines, producing the literal stringnotas a path. The downstream[ -f "$lib" ]filters it out, but it is worth tightening the filter and surfacing missing libraries instead of silently dropping them.♻️ Proposed refactor
- && for bin in /usr/local/bin/piper /usr/bin/ffmpeg; do \ - ldd "$bin" 2>/dev/null | awk '/=>/ {print $3}' >> /tmp/ttsdeps/libs.txt; \ - done \ + && for bin in /usr/local/bin/piper /usr/bin/ffmpeg /usr/bin/ffprobe; do \ + if ldd "$bin" 2>/dev/null | grep -q 'not found'; then \ + echo "Missing libraries for $bin" >&2; \ + ldd "$bin" | grep 'not found' >&2; \ + exit 1; \ + fi; \ + ldd "$bin" 2>/dev/null | awk '/=> \// {print $3}' >> /tmp/ttsdeps/libs.txt; \ + done \🤖 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 `@Web/Resgrid.Web.Tts/Dockerfile` around lines 59 - 63, The dependency-collection loop currently iterates "for bin in /usr/local/bin/piper /usr/bin/ffmpeg" and uses "ldd \"$bin\" | awk '/=>/ {print $3}'" which omits ffprobe and also extracts the word "not" from "=> not found"; update the loop to include /usr/bin/ffprobe alongside /usr/local/bin/piper and /usr/bin/ffmpeg, tighten the ldd parsing (e.g. only print the third field when it is an absolute path or exclude "not found" lines) instead of blindly using awk '/=>/ {print $3}', and surface missing libraries by capturing ldd lines containing "not found" to a separate file (e.g. /tmp/ttsdeps/missing.txt) so missing deps are visible rather than silently ignored.
62-63: ⚖️ Poor tradeoffFilter system libraries from dependency collection to avoid shadowing the runtime base image.
Lines 59-63 collect all libraries from
lddoutput (piper, ffmpeg) plus everything in/usr/local/lib, then copy them to/tmp/ttsdeps/. This includes system libraries likelibc.so.6,libstdc++.so.6,libgcc_s.so.1, etc., from the SDK image. BecauseLD_LIBRARY_PATHis prepended to/usr/local/lib/tts(line 78), these copies will take precedence over the runtime base image's system libraries.Both stages currently use Debian 13, so this is harmless today, but creates a hidden coupling: if the SDK base drifts a glibc patch version ahead, the runtime will silently use the older/newer libc from
/usr/local/lib/tts, risking version mismatches likeGLIBC_X.X not founderrors.Filter out system libraries guaranteed by the runtime base (libc, libstdc++, libgcc_s, libm, libdl, libpthread, librt, ld-linux*) and keep only TTS-specific ones (libespeak-ng and piper's bundled dependencies). Use a grep exclusion on the
lddoutput:ldd "$bin" 2>/dev/null | awk '/=>/ {print $3}' | grep -vE '(libc|libstdc\+\+|libgcc_s|libm|libdl|libpthread|librt|ld-linux)' >> /tmp/ttsdeps/libs.txtAlso applies to: 73
🤖 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 `@Web/Resgrid.Web.Tts/Dockerfile` around lines 62 - 63, The Dockerfile is copying all .so files from /usr/local/lib into /tmp/ttsdeps, which can shadow runtime system libraries; update the dependency collection for ldd outputs and the /usr/local/lib copy to exclude standard system libs (libc, libstdc++, libgcc_s, libm, libdl, libpthread, librt, ld-linux*) so only TTS-specific libs (espeak-ng, piper and their bundled deps) are collected; specifically, change the ldd collection pipeline that writes to /tmp/ttsdeps/libs.txt to filter out those names (use a grep -vE exclusion) and likewise modify the find/copy step (the line with find /usr/local/lib -name '*.so*' -type f >> /tmp/ttsdeps/libs.txt and the subsequent copy loop) to skip files matching those same system-library patterns, preserving LD_LIBRARY_PATH=/usr/local/lib/tts semantics.
🤖 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 `@Web/Resgrid.Web.Tts/Dockerfile`:
- Line 78: The ENV line in the Dockerfile sets LD_LIBRARY_PATH to
"/usr/local/lib/tts:${LD_LIBRARY_PATH}" which expands to a trailing colon (empty
path) and lets the dynamic linker treat CWD as a library search location; change
the ENV declaration to set LD_LIBRARY_PATH explicitly without the expansion,
e.g. replace the ENV line with one that sets
LD_LIBRARY_PATH="/usr/local/lib/tts" (remove the ${LD_LIBRARY_PATH} and trailing
colon) so no empty path segment is introduced.
- Around line 74-75: The Dockerfile currently copies /etc/passwd and /etc/group
from the publish stage (COPY --from=publish /etc/passwd /etc/passwd and COPY
--from=publish /etc/group /etc/group), which overwrites base-image system
accounts; remove those two COPY lines and instead append only the app user/group
entries into the final image's /etc/passwd and /etc/group via a RUN that echoes
the new group and user lines (so you preserve base users like root/_apt), and
also remove corresponding groupadd/useradd invocations from the publish stage
(they're unnecessary and not available in the hardened runtime).
---
Outside diff comments:
In `@Web/Resgrid.Web.Tts/Dockerfile`:
- Line 38: The fallback branch in the Dockerfile line that currently does "if [
-d /tmp/piper/espeak-ng-data ]; then cp -R /tmp/piper/espeak-ng-data
/usr/share/; else ln -sf /usr/lib/x86_64-linux-gnu/espeak-ng-data
/usr/share/espeak-ng-data; fi" produces a dangling symlink in the final image
because /usr/lib/... only exists in the publish stage; change this to
materialize a real directory in the publish stage or fail the build: detect
absence of /tmp/piper/espeak-ng-data and then copy (dereference) the
espeak-ng-data contents from the publish-stage location into
/usr/share/espeak-ng-data (e.g., use a non-symlink copy such as cp -a or cp -R
with dereference) or exit with an error if neither source exists so the final
image contains real data rather than a symlink.
---
Nitpick comments:
In `@Web/Resgrid.Web.Tts/Dockerfile`:
- Around line 25-55: The big RUN that handles apt installs, Piper extraction,
espeak data handling, voice-model downloads and cleanup should be split into
separate RUNs to improve layer caching: 1) a RUN that does apt-get
update/install (ffmpeg, libespeak-ng1, ca-certificates, curl), creates
/usr/local/share/piper-voices, downloads and installs the Piper binary using
PIPER_VERSION and performs the espeak-ng-data copy/link and ldconfig, then
cleans /tmp; 2) a separate RUN that performs the voice-model curl loop writing
to /usr/local/share/piper-voices (the list of voice paths and the curl --retry
invocations); and 3) a final RUN for any user creation, permissions (chmod +x
/usr/local/bin/piper), and any remaining lib copying (find /tmp/piper -name
'*.so*' -> /usr/local/lib) if still needed—preserve command order and cleanup
(rm -rf) in each RUN to keep image size unchanged.
- Around line 59-63: The dependency-collection loop currently iterates "for bin
in /usr/local/bin/piper /usr/bin/ffmpeg" and uses "ldd \"$bin\" | awk '/=>/
{print $3}'" which omits ffprobe and also extracts the word "not" from "=> not
found"; update the loop to include /usr/bin/ffprobe alongside
/usr/local/bin/piper and /usr/bin/ffmpeg, tighten the ldd parsing (e.g. only
print the third field when it is an absolute path or exclude "not found" lines)
instead of blindly using awk '/=>/ {print $3}', and surface missing libraries by
capturing ldd lines containing "not found" to a separate file (e.g.
/tmp/ttsdeps/missing.txt) so missing deps are visible rather than silently
ignored.
- Around line 62-63: The Dockerfile is copying all .so files from /usr/local/lib
into /tmp/ttsdeps, which can shadow runtime system libraries; update the
dependency collection for ldd outputs and the /usr/local/lib copy to exclude
standard system libs (libc, libstdc++, libgcc_s, libm, libdl, libpthread, librt,
ld-linux*) so only TTS-specific libs (espeak-ng, piper and their bundled deps)
are collected; specifically, change the ldd collection pipeline that writes to
/tmp/ttsdeps/libs.txt to filter out those names (use a grep -vE exclusion) and
likewise modify the find/copy step (the line with find /usr/local/lib -name
'*.so*' -type f >> /tmp/ttsdeps/libs.txt and the subsequent copy loop) to skip
files matching those same system-library patterns, preserving
LD_LIBRARY_PATH=/usr/local/lib/tts semantics.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6253ce3-d943-428f-b5c8-9d7064d3a304
📒 Files selected for processing (1)
Web/Resgrid.Web.Tts/Dockerfile
|
Approve |
Summary by CodeRabbit