fix(brew): relocate text in skip-relocation bottles - #11665
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fe51617. Configure here.
📝 WalkthroughWalkthroughBottle pouring now always calls keg relocation. The relocation API accepts ChangesBrew keg relocation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Greptile SummaryThe PR changes
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (3): Last reviewed commit: "fix(brew): remove redundant relocation b..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/system/packages/brew/relocate.rs (1)
227-240: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore file permissions before skipping binary relocation.
Lines 227-234 add owner write permission before Line 238 evaluates
skip_linkage. Thecontinuebypasses the restoration at Line 283. A read-only skipped binary remains owner-writable after pouring.Evaluate
skip_linkagebefore changing permissions, or restorepermsbefore thecontinue.Proposed fix
- let perms = path.metadata()?.permissions(); - // bottle files are often read-only; lift that while we patch - let mut writable = perms.clone(); - std::os::unix::fs::PermissionsExt::set_mode( - &mut writable, - std::os::unix::fs::PermissionsExt::mode(&perms) | 0o200, - ); - std::fs::set_permissions(path, writable)?; let macho = is_macho(&content); let elf = cfg!(target_os = "linux") && super::elf::is_elf(&content); let shebang_end = text_executable_shebang_end(&content); if skip_linkage && (macho || elf || (content.contains(&0) && shebang_end.is_none())) { continue; } + let perms = path.metadata()?.permissions(); + let mut writable = perms.clone(); + std::os::unix::fs::PermissionsExt::set_mode( + &mut writable, + std::os::unix::fs::PermissionsExt::mode(&perms) | 0o200, + ); + std::fs::set_permissions(path, writable)?;🤖 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/system/packages/brew/relocate.rs` around lines 227 - 240, Restore the original permissions for skipped binaries in the relocation flow before continuing. Update the skip_linkage branch in the function containing the perms and macho/elf checks so files identified for skipping do not retain the owner-write bit added before patching, while preserving the existing relocation behavior for files that are processed.
🤖 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/system/packages/brew/relocate.rs`:
- Around line 227-240: Restore the original permissions for skipped binaries in
the relocation flow before continuing. Update the skip_linkage branch in the
function containing the perms and macho/elf checks so files identified for
skipping do not retain the owner-write bit added before patching, while
preserving the existing relocation behavior for files that are processed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 1117b696-249c-458d-8984-01063d8cb15c
📒 Files selected for processing (2)
src/system/packages/brew/pour.rssrc/system/packages/brew/relocate.rs
Instruction counts
No instruction-count regression above 1%. Only instruction counts gate. Wall clock is shown for context — on identical hardware it moves 4-20% run to run. Measured by tak — instruction-counted CLI benchmarks, stored in this repository's git notes.
|
1 participant
Summary
:any_skip_relocationas skipping only binary linkage relocationRoot cause
The brew pour path skipped
relocate_kegentirely for macOS bottles tagged:any_skip_relocation. Homebrew uses that tag only to suppress dynamic linkage relocation; it still replaces text placeholders such as@@HOMEBREW_PREFIX@@and@@HOMEBREW_CELLAR@@.This left scripts and configuration files containing unresolved placeholders and produced an empty
changed_filesreceipt entry.Reported in #11656.
Validation
cargo test system::packages::brew::relocate::tests(8 passed)mise run formatgit diff --checkAI-assisted — Tool: Codex; model: unavailable/unavailable; version: unavailable.
Note
Medium Risk
Touches the Homebrew bottle pour/relocation path used on every install; behavior change is intentional and covered by a regression test, but wrong linkage skipping could still break binaries on edge-case bottles.
Overview
Fixes
:any_skip_relocationbottles leaving@@HOMEBREW_*@@placeholders in scripts and config by always running relocation during pour instead of skipping it entirely on macOS (and on Linux when the bottle was built by Homebrew ≥ 5.1.15).relocate_kegnow takes askip_linkageflag: when set, Mach-O, ELF, and other binary linkage paths are left unchanged, but text files and shebang-only executables still get placeholder replacement—matching Homebrew’s meaning of:any_skip_relocation. The pour path always invokes relocation and passesskip_linkagefrom the bottle cellar tag (with the existing Linux Homebrew version gate for mis-tagged older bottles).Adds
test_skip_linkage_still_relocates_text_filesto assert text is rewritten, Mach-O bytes and permissions stay untouched, and receipts list only changed text files.Reviewed by Cursor Bugbot for commit 08427d3. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit