refactor(compilation): extract ccache into a swappable extension#9840
Open
iav wants to merge 6 commits into
Open
refactor(compilation): extract ccache into a swappable extension#9840iav wants to merge 6 commits into
iav wants to merge 6 commits into
Conversation
Contributor
|
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 (9)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughReplaces inline ccache wiring with a generic compile-wrapper, implements ccache as an extension (config validation, env wiring, pre/post hooks, helpers), auto-enables ccache when requested, and updates ATF/Crust/kernel/uboot build flows to use the new hooks and wrapper. ChangesCompilation Wrapper Refactoring
Sequence DiagramsequenceDiagram
participant Build as BuildTarget
participant Wrapper as do_with_compile_wrapper
participant PreExt as compile_wrapper_pre__ccache
participant Make as make
participant PostExt as compile_wrapper_post__ccache
Build->>Wrapper: start build under wrapper
Wrapper->>Wrapper: register post-hook cleanup
Wrapper->>PreExt: call_extension_method compile_wrapper_pre
PreExt->>PreExt: clear stats, report cache dir/size
Wrapper->>Make: run make (compiler wrapped by CCACHE)
Make->>Make: compile
Wrapper->>PostExt: explicit post-hook call
PostExt->>PostExt: parse stats, compute hit%, report
Wrapper->>Build: return exit code
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
github-actions
Bot
added
05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@extensions/ccache.sh`:
- Around line 109-117: When SHOW_CCACHE is enabled, guard the probes that
compute sizes by validating __ext_ccache_dir_actual before running du and
numfmt: ensure __ext_ccache_dir_actual is non-empty and a directory (e.g., test
-n and -d) and only then set __ext_ccache_dir_size_before and compute
ccache_dir_size_before_human; otherwise set __ext_ccache_dir_size_before to a
safe default (0) and ccache_dir_size_before_human to a fallback like "unset" or
"N/A". Update the logic around __ext_ccache_dir_size_before, the du call, and
the numfmt call so they are conditional on the existence check to keep the
post-hook best-effort and avoid aborting on empty/invalid
__ext_ccache_dir_actual.
🪄 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: f98c058c-d6d2-44e4-be35-f0577a0cfeeb
📒 Files selected for processing (11)
extensions/ccache.shlib/functions/compilation/atf.shlib/functions/compilation/ccache.shlib/functions/compilation/compile-wrapper.shlib/functions/compilation/crust.shlib/functions/compilation/kernel.shlib/functions/compilation/uboot.shlib/functions/configuration/compilation-config.shlib/functions/configuration/main-config.shlib/functions/general/extensions.shlib/library-functions.sh
💤 Files with no reviewable changes (1)
- lib/functions/compilation/ccache.sh
iav
force-pushed
the
refactor/ccache-as-extension
branch
from
f32ac58 to
f1138ac
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/library-functions.sh (1)
288-300: ⚡ Quick winRemove duplicate
compile-wrapper.shsourcing.Line 288-Line 290 and Line 297-Line 299 source the same file twice. Please keep a single inclusion to avoid duplicate top-level execution and keep the generated aggregation deterministic.
Proposed fix
-# no errors tolerated. invoked before each sourced file to make sure. -#set -o pipefail # trace ERR through pipes - will be enabled "soon" -#set -o nounset ## set -u : exit the script if you try to use an uninitialised variable - one day will be enabled -set -o errtrace # trace ERR through - enabled -set -o errexit ## set -e : exit the script if any statement returns a non-true return value - enabled -### lib/functions/compilation/compile-wrapper.sh -# shellcheck source=lib/functions/compilation/compile-wrapper.sh -source "${SRC}"/lib/functions/compilation/compile-wrapper.sh🤖 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 `@lib/library-functions.sh` around lines 288 - 300, Remove the duplicate sourcing of compile-wrapper.sh: locate the two identical source statements referencing source "${SRC}"/lib/functions/compilation/compile-wrapper.sh (the top-level include around the set -o errtrace/errexit block) and delete one of them so the file is only sourced once; ensure the remaining block preserves the intended shell options (set -o errtrace and set -o errexit) and any comments, leaving a single source line for compile-wrapper.sh to avoid duplicate execution.
🤖 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 `@lib/library-functions.sh`:
- Around line 288-300: Remove the duplicate sourcing of compile-wrapper.sh:
locate the two identical source statements referencing source
"${SRC}"/lib/functions/compilation/compile-wrapper.sh (the top-level include
around the set -o errtrace/errexit block) and delete one of them so the file is
only sourced once; ensure the remaining block preserves the intended shell
options (set -o errtrace and set -o errexit) and any comments, leaving a single
source line for compile-wrapper.sh to avoid duplicate execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 57a7c283-8c3e-43ed-9f3f-5ecb69277112
📒 Files selected for processing (8)
extensions/ccache.shlib/functions/compilation/ccache.shlib/functions/compilation/kernel.shlib/functions/compilation/uboot.shlib/functions/configuration/compilation-config.shlib/functions/configuration/main-config.shlib/functions/general/extensions.shlib/library-functions.sh
💤 Files with no reviewable changes (1)
- lib/functions/compilation/ccache.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/functions/compilation/uboot.sh
- lib/functions/general/extensions.sh
- lib/functions/compilation/kernel.sh
- extensions/ccache.sh
- lib/functions/configuration/compilation-config.sh
iav
force-pushed
the
refactor/ccache-as-extension
branch
3 times, most recently
from
d3170e8 to
6dd6a7c
Compare
8 tasks
iav
force-pushed
the
refactor/ccache-as-extension
branch
from
6dd6a7c to
6645b8f
Compare
Member
How safe is moving lib.config reading higher in the chain? |
Contributor
Author
|
@igorpecovnik dug into the origin of this: the lib.config warning I added in one of the codex rounds was a guard against a theoretical edge case, not an observed pattern. I have no evidence that anyone actually sets Three options, in increasing aggressiveness:
|
The variable has been declared but never assigned or used since 8278dc5 (2023-06-20, 'allwinner: Enable crust compilation'), which copy-pasted the binutils_version + binutils_flags skeleton from atf.sh but did not bring along the gcc/ld probe block that conditionally populates the flag, nor the TF_LDFLAGS-style use site. Crust is or1k-elf firmware where --no-warn-rwx-segment is not relevant, so the absence of the probe is by design; the leftover empty declaration is just dead. Removes a pre-existing SC2034 'appears unused' warning that surfaced when the file is touched by other refactors. Assisted-by: Claude:claude-opus-4.7
CROSS_COMPILE and CC were hard-coded to 'ccache' which:
- breaks when USE_CCACHE=no (CCACHE='' but 'ccache' still tried)
- prevents wrapper substitution by extensions (sccache, distcc)
Match the established kernel/uboot convention where the cache wrapper
is parameterised via ${CCACHE} (set by configure-ccache based on
USE_CCACHE). Default value '' means no wrapper.
Assisted-by: Claude:claude-opus-4.7
…le_wrapper
Adds three new extension points to the compilation pipeline:
- atf_make_config: called right before invoking 'make' for ATF (TF-A).
Receives env that will be passed to make; extensions can mutate
CCACHE / CC / CROSS_COMPILE to inject wrappers.
- crust_make_config: same for the or1k Crust firmware (defconfig +
target make).
- do_with_compile_wrapper / compile_wrapper_pre / compile_wrapper_post:
high-level wrapper that bookends an arbitrary compilation block
with pre/post hooks. Lets extensions observe a compile pass
end-to-end (clear stats / dump stats / mount caches / etc) without
having to patch each artifact's compile.sh.
Pure addition: no existing call site is wired yet (kernel/uboot
migration follows in subsequent commits). The hooks are no-ops until
an extension implements them.
Assisted-by: Claude:claude-opus-4.7
Extracts ccache-specific logic from lib/functions/compilation/ccache.sh
into a new opt-out extension extensions/ccache.sh. The extension wires
itself into the compilation pipeline through the new hook points:
- compile_prepare_vars: dispatched from prepare_compilation_vars
(early). Computes CCACHE / CCACHE_DIR / CCACHE_UMASK / PATH and
exports them so subsequent make-quoted-params arrays expand
correctly. Runs before any artifact's make invocation.
- compile_wrapper_pre/_post: zeros / reports per-build stats around
each artifact's compile pass.
- {kernel,uboot,atf,crust}_make_config: ensures CCACHE_DIR /
CCACHE_UMASK are visible to the make subshell on each artifact.
This is an additive change: nothing yet calls call_extension_method
"compile_prepare_vars" — that wiring lands in the next refactor commit.
Assisted-by: Claude:claude-opus-4.7
iav
force-pushed
the
refactor/ccache-as-extension
branch
from
6645b8f to
b7c3436
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/functions/compilation/crust.sh (1)
63-69:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard
$CCACHEexpansion to prevent leading space inCROSS_COMPILEwhen cache is disabled.Same issue as ATF (see comment on atf.sh:96-99): when
CCACHEis empty, the unguarded expansion"CROSS_COMPILE='$CCACHE $CRUST_COMPILER'"produces a leading space that breaks the compiler invocation.Apply the same conditional-suffix fix to both make invocations (lines 65 and 69).
🔧 Proposed fix
run_host_command_logged CCACHE_BASEDIR="$(pwd)" \ "CFLAGS='-fdiagnostics-color=always -Wno-error=attributes -Wno-error=incompatible-pointer-types'" \ - make ${CRUSTCONFIG} "${CTHREADS}" "CROSS_COMPILE='$CCACHE $CRUST_COMPILER'" + make ${CRUSTCONFIG} "${CTHREADS}" "CROSS_COMPILE='${CCACHE:+$CCACHE }$CRUST_COMPILER'" run_host_command_logged CCACHE_BASEDIR="$(pwd)" \ "CFLAGS='-fdiagnostics-color=always -Wno-error=attributes -Wno-error=incompatible-pointer-types'" \ - make $target_make "${CTHREADS}" "CROSS_COMPILE='$CCACHE $CRUST_COMPILER'" + make $target_make "${CTHREADS}" "CROSS_COMPILE='${CCACHE:+$CCACHE }$CRUST_COMPILER'"🤖 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 `@lib/functions/compilation/crust.sh` around lines 63 - 69, The CROSS_COMPILE argument currently unconditionally expands CCACHE causing a leading space when CCACHE is empty; update both calls to run_host_command_logged that build with "CROSS_COMPILE='$CCACHE $CRUST_COMPILER'" to guard the cache token (e.g. use a conditional expansion so the space only appears when CCACHE is non-empty), making the string effectively "CROSS_COMPILE='${CCACHE:+$CCACHE }$CRUST_COMPILER'"; adjust both occurrences around the make invocations (the lines using run_host_command_logged, CROSS_COMPILE, CCACHE and CRUST_COMPILER) so the compiler invocation is not broken when CCACHE is disabled.
🤖 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 `@lib/functions/compilation/atf.sh`:
- Around line 96-99: The CROSS_COMPILE assignment can get a leading space when
CCACHE is empty; in the run_host_command_logged call where
CROSS_COMPILE='${CCACHE} ${ATF_COMPILER}' and CC='${CCACHE} ${ATF_COMPILER}gcc',
change the expansions to use conditional parameter expansion so the space is
only inserted when CCACHE is non-empty (e.g., use the ${CCACHE:+...} form around
the space and CCACHE) so both CROSS_COMPILE and CC are constructed correctly;
update the two occurrences inside the run_host_command_logged invocation
(referencing CROSS_COMPILE, CC, CCACHE, ATF_COMPILER) accordingly.
---
Outside diff comments:
In `@lib/functions/compilation/crust.sh`:
- Around line 63-69: The CROSS_COMPILE argument currently unconditionally
expands CCACHE causing a leading space when CCACHE is empty; update both calls
to run_host_command_logged that build with "CROSS_COMPILE='$CCACHE
$CRUST_COMPILER'" to guard the cache token (e.g. use a conditional expansion so
the space only appears when CCACHE is non-empty), making the string effectively
"CROSS_COMPILE='${CCACHE:+$CCACHE }$CRUST_COMPILER'"; adjust both occurrences
around the make invocations (the lines using run_host_command_logged,
CROSS_COMPILE, CCACHE and CRUST_COMPILER) so the compiler invocation is not
broken when CCACHE is disabled.
🪄 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: d6fb8ea0-443c-4249-83f7-ef62086bbfc8
📒 Files selected for processing (11)
extensions/ccache.shlib/functions/compilation/atf.shlib/functions/compilation/ccache.shlib/functions/compilation/compile-wrapper.shlib/functions/compilation/crust.shlib/functions/compilation/kernel.shlib/functions/compilation/uboot.shlib/functions/configuration/compilation-config.shlib/functions/configuration/main-config.shlib/functions/general/extensions.shlib/library-functions.sh
💤 Files with no reviewable changes (1)
- lib/functions/compilation/ccache.sh
…che.sh
Wires the new ccache extension (extensions/ccache.sh) into the
compilation pipeline, replacing the in-core lib/functions/compilation/
ccache.sh module that was sourced unconditionally.
Changes:
- lib/functions/compilation/kernel.sh, uboot.sh: wrap the compile
pass with do_with_compile_wrapper so the new compile_wrapper_pre/
_post hooks fire around it.
- lib/functions/configuration/compilation-config.sh: dispatch
compile_prepare_vars (early phase, before any make-quoted-params
array is built) instead of inlining configure-ccache. Also clears
stale CCACHE export from a previous artifact iteration so a
second uboot/kernel call in the same shell can't reuse a now-
disabled cache. Keeps a one-shot deprecation warning if any
user-patch still sources the legacy lib.config.
- lib/functions/configuration/main-config.sh: if USE_CCACHE=yes
(or the ccache-remote extension is enabled) auto-enable the
'ccache' extension before initialize_extension_manager runs.
Honours both EXT alias and the legacy space-separated
ENABLE_EXTENSIONS form.
- lib/functions/general/extensions.sh: add extension_list_normalized
helper used by the auto-enable shim and per-extension mutex
checks. It (a) normalises all IFS whitespace (space, tab,
newline) — matching what initialize_extension_manager splits on
— and (b) folds in extensions enabled programmatically via
enable_extension() (from board/family/user configuration) by
scanning extension_function_info. Without this fold-in, a board
that does `enable_extension ccache-remote` would slip past both
the auto-enable shim and the mutex guard, leaving USE_CCACHE=yes
with no backend hook (or letting two compile-cache backends
coexist).
- lib/functions/compilation/ccache.sh: removed (logic moved to
extensions/ccache.sh).
Behaviour is preserved for USE_CCACHE=yes (default-on for kernel/uboot
builds) and USE_CCACHE=no callers. ENABLE_EXTENSIONS=ccache-remote
implicitly pulls in ccache, matching pre-refactor coupling.
Assisted-by: Claude:claude-opus-4.7
…ding space
When USE_CCACHE=no, CCACHE is set to "" by compilation-config.sh.
The make-argument pattern "CROSS_COMPILE='${CCACHE} ${COMPILER}'" then
expands to "CROSS_COMPILE=' arm-…-'" (literal leading space). make
in turn evaluates ${CROSS_COMPILE}gcc as " arm-…-gcc" with that
leading space and fails to exec.
Switch to "${CCACHE:+${CCACHE} }${COMPILER}" so the trailing space is
only inserted when CCACHE is non-empty. Matches the idiom already used
in lib/functions/compilation/uboot.sh:259.
- atf.sh:96 — regression introduced earlier in this PR by the
"use ${CCACHE} variable instead of hardcoded ccache" commit; the
previous hardcoded ccache was always non-empty so the latent issue
was masked.
- crust.sh:65,69 — pre-existing in main, same fix applied since the
pattern is identical.
Assisted-by: Claude:claude-opus-4.7
iav
force-pushed
the
refactor/ccache-as-extension
branch
from
e2e5eba to
4032a46
Compare
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.
Goal
Move ccache out of core compilation into a swappable extension. After this PR a parallel cache backend (sccache, distcc-style, distributed cache) can ship as a sibling extension without touching core.
A concrete near-term motivation: a future
extensions/sccache.shwould let CI workflows running on GitHub-hosted runners route their compile-cache straight into the native GitHub Actions cache (SCCACHE_GHA_ENABLED=on, viamozilla-actions/sccache-action) — 10 GB free per repository, low-latency on the runner's local region. The same extension framework also covers self-hosted runners (S3/R2/MinIO bucket, WebDAV endpoint, Redis-on-disk, ...) without core changes — backend choice is purely an env-var concern per workflow.What changes (vs
armbian/build:main)New files
extensions/ccache.sh— implementscompile_prepare_vars,compile_wrapper_pre/_post, and{kernel,uboot,atf,crust}_make_confighooks.lib/functions/compilation/compile-wrapper.sh—do_with_compile_wrapper+compile_wrapper_pre/_posthooks.Removed
lib/functions/compilation/ccache.sh(logic moved into the extension).Modified
lib/functions/compilation/atf.sh— addsatf_make_confighook; replaces hardcodedccachewith${CCACHE}(latent bug:USE_CCACHE=nowas still draggingccacheinto ATF).lib/functions/compilation/crust.sh— addscrust_make_confighook; drops deadbinutils_flags_crust=""declaration (preexisting SC2034 since 2023).lib/functions/compilation/{kernel,uboot}.sh— compile phase wrapped indo_with_compile_wrapper.lib/functions/configuration/compilation-config.sh— dispatchescompile_prepare_varsinstead of inliningconfigure-ccache; resets staleCCACHEbetween artifacts; warns ifUSE_CCACHEis set inuserpatches/lib.config(sourced too late forenable_extension).lib/functions/configuration/main-config.sh— auto-enables theccacheextension whenUSE_CCACHE=yes,PRIVATE_CCACHE=yes, orENABLE_EXTENSIONS=ccache-remote, beforeinitialize_extension_manager.lib/functions/general/extensions.sh— addsextension_list_normalizedhelper (used by the auto-enable shim and per-extension mutex checks).lib/library-functions.sh— regenerated.User-visible behavior
Unchanged.
USE_CCACHE=yes(default for kernel/u-boot) andUSE_CCACHE=nobehave exactly as before;ENABLE_EXTENSIONS=ccache-remoteimplicitly pulls inccache, matching the pre-refactor coupling.Test plan
Built on a fresh DigitalOcean cloud builder (AMD EPYC 8c, Ubuntu 24.04),
BOARD=helios64 BRANCH=edge:USE_CCACHE=yeskernel coldCcache result hit=12 miss=14410USE_CCACHE=nokernelUSE_CCACHE=yesuboot+ATF coldCcache result hit=8 miss=1002ENABLE_EXTENSIONS=ccache-remote+CCACHE_REMOTE_STORAGE=http://m1:8088/ccache/over WireGuardRemote ccache result hit=4536 miss=2791 write=2740 err=0 (61%)Logs: paste.armbian.com/{enaretezuc,noxayoyoni,vonuyajido}.
Known design choices
USE_CCACHE=yesinuserpatches/lib.configdoes not auto-enable theccacheextension.lib.configis sourced afterinitialize_extension_manager, too late to enable extensions; theframework limitation is documented in
main-config.sh(too late to define hook functions or add extensions in lib.config). The shimemits a warning at compile-vars phase nudging users to migrate to
ENABLE_EXTENSIONS=ccache(the auto-enable code path).USE_CCACHE=yes,PRIVATE_CCACHE=yes, orENABLE_EXTENSIONS=ccache-remote(the onlyextension in-tree that sets
USE_CCACHE=yesfrom itsextension_prepare_config). Any future extension that also setsUSE_CCACHEfrom its prepare hook will need to add itself to thesame trigger list, since the shim runs before
initialize_extension_manager.Summary by CodeRabbit
New Features
Refactor
Chores