Make image smaller by removing stuff we don't need. #1
Conversation
|
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. WalkthroughAdds a slim base Dockerfile and a CI Dockerfile that parameterizes Zephyr SDK toolchains via ZSDK_TOOLCHAINS (generating repeated -t flags), adds extensive image cleanup, and updates the GitHub Actions workflow to build the new images and run disk/cache pruning steps. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (11)
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.
Pull request overview
This PR optimizes the ZSWatch CI Docker image by removing unnecessary components to reduce image size. The changes focus on three main areas: limiting Zephyr SDK toolchains to only those needed by ZSWatch, removing unused development tools inherited from the base image, and cleaning up documentation directories.
Key changes:
- Limits Zephyr SDK installation to only arm-zephyr-eabi and x86_64-zephyr-elf toolchains instead of installing all toolchains
- Removes 32-bit support packages and unused debugging/coverage/documentation tools (valgrind, lcov, gcovr, doxygen)
- Removes documentation directories (doc, man, info) from the installed SDK to save space
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| apt-get purge --auto-remove -y \ | ||
| valgrind \ | ||
| lcov \ | ||
| gcovr \ | ||
| doxygen \ | ||
| thrift-compiler || true |
There was a problem hiding this comment.
The apt-get purge command is executed without first running apt-get update. When running in a fresh Docker layer, the package manager's cache may be stale or empty, which could cause the purge operation to fail or behave unexpectedly. Although the commands have || true as a safeguard, it's better practice to run apt-get update before package operations.
Consider adding:
apt-get update -y
apt-get purge --auto-remove -y \
valgrind \
...| libc6-dbg:i386 libfuse-dev:i386 libsdl2-dev:i386 || true | ||
| dpkg --remove-architecture i386 || true |
There was a problem hiding this comment.
Removing i386 architecture may cause issues with the existing PKG_CONFIG_PATH environment variable on line 79, which points to /usr/lib/i386-linux-gnu/pkgconfig. After this change, that directory will no longer exist, potentially causing package config lookups to fail.
Consider verifying whether i386 package config support is needed, and if not, update the PKG_CONFIG_PATH in a subsequent change to remove the i386 reference.
| zephyr-sdk-${ZSDK_VERSION}/setup.sh -t all -h -c | ||
| zephyr-sdk-${ZSDK_VERSION}/setup.sh -t "${ZSDK_TOOLCHAINS}" -h -c | ||
| rm zephyr-sdk-${ZSDK_VERSION}_linux-${HOSTTYPE}.tar.xz | ||
| find zephyr-sdk-${ZSDK_VERSION}/sysroots -maxdepth 4 -type d \( -name doc -o -name man -o -name info \) -exec rm -rf '{}' + |
There was a problem hiding this comment.
The find command with -exec rm -rf '{}' + may fail when removing parent directories before their children are processed. When multiple directories match (e.g., nested doc folders), removing a parent doc directory will cause subsequent attempts to remove its child directories to fail because they no longer exist.
Consider using -prune instead or changing + to \; to process one directory at a time:
find zephyr-sdk-${ZSDK_VERSION}/sysroots -maxdepth 4 -type d \( -name doc -o -name man -o -name info \) -exec rm -rf '{}' \;or use -prune to avoid descending into matched directories:
find zephyr-sdk-${ZSDK_VERSION}/sysroots -maxdepth 4 \( -name doc -o -name man -o -name info \) -prune -exec rm -rf '{}' +| find zephyr-sdk-${ZSDK_VERSION}/sysroots -maxdepth 4 -type d \( -name doc -o -name man -o -name info \) -exec rm -rf '{}' + | |
| find zephyr-sdk-${ZSDK_VERSION}/sysroots -maxdepth 4 -type d \( -name doc -o -name man -o -name info \) -exec rm -rf '{}' \; |
jakkra
force-pushed
the
zswatch_size_optimize
branch
from
65b3993 to
4437214
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Dockerfile.zswatch-ci (2)
32-50: Addapt-get updatebefore purge operations to ensure package cache is fresh.In a fresh Docker layer, the package manager cache may be stale. Although
|| truemitigates failures, it's best practice to update the cache first. This mirrors theapt-get updatein the earlier block (line 21).RUN <<'EOF' if [ "${HOSTTYPE}" = "x86_64" ]; then # Drop 32-bit support and multilib toolchains not needed for nRF/native_sim_64 + apt-get update -y apt-get purge --auto-remove -y \ gcc-multilib g++-multilib \ libc6-dbg:i386 libfuse-dev:i386 libsdl2-dev:i386 || true
66-66: Consider using-pruneor-maxdepth 1in find command to avoid nested directory removal risks.The current
find ... -exec rm -rf '{}' +batches deletions, which can fail if a parent directory is removed before its children are processed (e.g., nesteddocfolders). Using-pruneprevents descending into matched directories, or changing+to\;processes one at a time.- find zephyr-sdk-${ZSDK_VERSION}/sysroots -maxdepth 4 -type d \( -name doc -o -name man -o -name info \) -exec rm -rf '{}' + + find zephyr-sdk-${ZSDK_VERSION}/sysroots -maxdepth 4 -type d \( -name doc -o -name man -o -name info \) -exec rm -rf '{}' \;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/zswatch-ci-image.yml(1 hunks)Dockerfile.zswatch-ci(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push-ci
🔇 Additional comments (4)
Dockerfile.zswatch-ci (3)
15-17: Well-designed configurability for SDK toolchains.The new
ZSDK_TOOLCHAINSargument provides flexibility to customize which toolchains are installed, reducing image size for users who need fewer toolchains.
58-64: Shell loop correctly converts comma-separated toolchains into repeated-tflags.The logic properly parses
ZSDK_TOOLCHAINSand constructs the required arguments forsetup.sh. The positional parameter manipulation (set -- "$@" ...) is idiomatic and correct.
85-85: VerifyPKG_CONFIG_PATHreferences valid paths after removing i386 architecture.Line 37 removes the i386 architecture, so
/usr/lib/i386-linux-gnu/pkgconfig(referenced on line 85) will no longer exist. Confirm whether i386 package config support is needed, and update or remove this path reference if it's no longer applicable..github/workflows/zswatch-ci-image.yml (1)
9-9: Correctly addedDockerfile.zswatch-cito push-triggered path filters.This ensures the CI workflow re-runs whenever the
Dockerfile.zswatch-cifile changes, which is essential now that this file is being actively modified and built as part of the pipeline.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/zswatch-ci-image.yml (1)
55-56: Buildx cache pruning strategically placed.Pruning the buildx cache after the base image build but before the larger CI image build is well-timed to reclaim space. The 5GB
--keep-storagethreshold provides a reasonable safety margin while supporting the size reduction goal.Consider documenting the rationale behind the 5GB threshold if this value may need adjustment in the future as build patterns evolve.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/zswatch-ci-image.yml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push-ci
🔇 Additional comments (2)
.github/workflows/zswatch-ci-image.yml (2)
9-9: Correct addition of Dockerfile.zswatch-ci to trigger paths.Adding the new Dockerfile to the
pathstrigger ensures the workflow runs whenever the CI image definition changes, maintaining proper CI/CD integration.
27-33: Good disk cleanup strategy with proper error handling.The "Free disk space" step removes hosted tool caches (dotnet, ghc, android, CodeQL) and prunes Docker resources before the build, which aligns well with the PR objective to reduce image size. The
|| trueguards prevent step failures from cleanup command errors, and thedf -hcalls provide useful visibility into freed space.
jakkra
force-pushed
the
zswatch_size_optimize
branch
from
ec233cc to
e350be5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/zswatch-ci-image.yml(3 hunks)Dockerfile.zswatch-ci(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/zswatch-ci-image.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push-ci
🔇 Additional comments (2)
Dockerfile.zswatch-ci (2)
15-17: Good use of build-time configuration to tailor SDK installation.The
ZSDK_TOOLCHAINSarg allows selective installation of only required toolchains, directly supporting the goal of image size reduction. The arg and env var declaration are clean.
67-73: Dynamic toolchain flag generation looks correct.The logic correctly splits the comma-separated
ZSDK_TOOLCHAINSlist, generates repeated-tflags, and appends-h -c. This approach is cleaner than the previous-t alland should integrate seamlessly with the Zephyr SDK setup script.Please verify that the SDK setup script accepts this flag format and that the selected toolchains are installed correctly. You can test locally with:
docker build --build-arg ZSDK_TOOLCHAINS="arm-zephyr-eabi" -f Dockerfile.zswatch-ci .and confirm that only the specified toolchain is present in the final image.
| if [ "${HOSTTYPE}" = "x86_64" ]; then | ||
| # Drop 32-bit support and multilib toolchains not needed for nRF/native_sim_64 | ||
| apt-get purge --auto-remove -y \ | ||
| gcc-multilib g++-multilib \ | ||
| libc6-dbg:i386 libfuse-dev:i386 libsdl2-dev:i386 || true | ||
| dpkg --remove-architecture i386 || true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -name "Dockerfile.zswatch-ci" --type fRepository: ZSWatch/docker-image
Length of output: 294
🏁 Script executed:
fd "Dockerfile.zswatch-ci"Repository: ZSWatch/docker-image
Length of output: 85
🏁 Script executed:
cat -n Dockerfile.zswatch-ci | sed -n '30,40p;90,100p'Repository: ZSWatch/docker-image
Length of output: 815
🏁 Script executed:
grep -n "i386\|PKG_CONFIG_PATH" Dockerfile.zswatch-ciRepository: ZSWatch/docker-image
Length of output: 228
Resolve mismatch between i386 architecture removal and PKG_CONFIG_PATH.
Lines 37 removes the i386 architecture on x86_64 hosts, but line 94 still sets PKG_CONFIG_PATH=/usr/lib/i386-linux-gnu/pkgconfig. After the i386 removal, this path will not exist, resulting in an unused or stale environment variable.
Either update PKG_CONFIG_PATH to remove the i386 reference, or remove the i386 purge if the path is actually needed for the build.
🤖 Prompt for AI Agents
In Dockerfile.zswatch-ci around lines 32 to 37, the script purges the i386
architecture and removes i386 packages but later (line ~94) PKG_CONFIG_PATH is
set to include /usr/lib/i386-linux-gnu/pkgconfig, creating a mismatch; either
stop removing the i386 architecture and i386 packages if that path is required,
or update the PKG_CONFIG_PATH assignment to exclude the i386 path (and remove
any references to /usr/lib/i386-linux-gnu/pkgconfig) so no stale/nonexistent
i386 pkgconfig path is left in the environment.
e350be5 to
af33da7
Compare
jakkra
force-pushed
the
zswatch_size_optimize
branch
from
bf8d9b9 to
219bac2
Compare
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.