patch-shebangs: optimize bash and add ANSI C implementation#482713
patch-shebangs: optimize bash and add ANSI C implementation#482713qweered wants to merge 2 commits intoNixOS:stagingfrom
Conversation
dfc579d to
6a44c22
Compare
Review dismissed automatically
This comment was marked as outdated.
This comment was marked as outdated.
6a44c22 to
ec7a09c
Compare
d17d854 to
f9b68a6
Compare
Why not use |
doronbehar
left a comment
There was a problem hiding this comment.
Overall looks good to me.
|
|
||
| # Make original file writable if it is read-only | ||
| local restoreReadOnly | ||
| local restoreReadOnly= |
There was a problem hiding this comment.
This change coincides with d61930c from #462319 - cc @bmillwood .
f14d9e0 to
461a2de
Compare
2ac44f6 to
a7dda1b
Compare
79a049b to
50c2605
Compare
- Reuse temp file across all files (avoids mktemp per-file overhead) - Use touch -r instead of stat + touch --date (avoids subshell) - Use printf/tail instead of sed for shebang replacement - Print status messages immediately (no buffering)
Add a portable C implementation that can be compiled and included in stdenv for significant performance improvements. Benchmark (1000 files): - Original bash: 26.1s - Optimized bash: 16.5s - C implementation: 0.12s (217x faster) Features: - C99/POSIX implementation for portability - Compiles with -std=c99 -O3 -Wall - Handles all edge cases: env shebangs, -S flag, read-only files - Preserves file timestamps - Same CLI interface as bash version
50c2605 to
3697136
Compare
|
Ok i think its ready for review, tested build manually and it works exactly as bash, now recompiling stdenv to be absolutely sure |
| local mode="" update="" | ||
| while [[ $# -gt 0 ]]; do | ||
| case "$1" in | ||
| --host) mode="--host"; shift ;; | ||
| --build) mode="--build"; shift ;; | ||
| --update) update="--update"; shift ;; | ||
| --) shift; break ;; | ||
| -*|--*) echo "Unknown option $1 supplied to patchShebangs" >&2; return 1 ;; | ||
| *) break ;; | ||
| esac | ||
| done | ||
| [[ -z "$mode" ]] && { [[ -n $strictDeps && $1 == "$NIX_STORE"* ]] && mode="--host" || mode="--build"; } | ||
| patch-shebangs $mode $update -- "$@" |
There was a problem hiding this comment.
Are you sure you have to put this logic here and not inside the C code?
| @@ -23,6 +23,24 @@ fixupOutputHooks+=(patchShebangsAuto) | |||
| # $ patchShebangs --build configure | |||
|
|
|||
| patchShebangs() { | |||
| # Use C implementation if available (much faster) | |||
There was a problem hiding this comment.
FYI, last time I tried to use C for setup hooks it was heavily discouraged, see #394610 (comment).
|
Hey @qweered, this is AI generated, isn't it? It has a certain Claude flavor. I'm asking because this isn't disclosed. I agree that the patch shebang hook could have better performance and be much better tested. I share the @K900 judgement that using raw C std library calls is the wrong call. What better could look like, maybe, is introducing a tool outside of Nixpkgs, with testing and documentation, which then fits into the Nixpkgs stdenv. I'm loathe to make changes on a performance basis here. |
|
Do we really need to have source in separate repository? patchShebangs is a lot simpler than patchElf and we also will not have trust issues. Yes this pr have been assisted by Claude opus |
Motivation
The
patchShebangshook is called during the fixup phase of nearly every package build. For most packages this is fast, but large projects with thousands of scripts become a serious bottleneck:With the original bash implementation, patching 1000 files takes 26 seconds. For Electron builds, this means 2+ minutes spent just on shebang patching, which adds up significantly in CI pipelines and when iterating on package development.
Benchmark Results
C implementation is 217x faster than original bash (0.12s vs 26.1s for 1000 files).
For Electron-scale builds (5000+ files), this reduces shebang patching from ~2.5 minutes to ~0.6 seconds.
Implementation
The C implementation:
patchShebangs) inextraNativeBuildInputsof final stdenvenvshebangs,-Sflag, read-only files, timestamp preservation, recursive directoriespatchShebangs.passthru.tests)Changes
Commit 1: Bash optimizations
mktempper-file overhead)touch -rinstead ofstat+touch --date(avoids subshell)printf/tailinstead ofsedfor shebang replacementCommit 2: C implementation + integration
pkgs/build-support/setup-hooks/patch-shebangs/extraNativeBuildInputsThings done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.