Repository navigation
build: reduce macOS release binary size with LLD - #4281
codeshaunted wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
📝 WalkthroughWalkthroughThe Rustflags script now supports macOS hosts only and no longer accepts a target argument. Release and size-gate workflows apply its flags only to macOS jobs, with contract tests updated accordingly. ChangesSafe ICF release pipeline
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/tests/test_release_pipeline_contract.py (2)
656-668: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest that existing
RUSTFLAGSare preserved.The assertions only check the appended linker flags. A regression that overwrites the caller’s
RUSTFLAGSwould still pass, despite the PR contract. Assert the preservation expression or run the script with a sentinel existing value and verify it remains in the output.🤖 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 `@scripts/tests/test_release_pipeline_contract.py` around lines 656 - 668, Extend test_unix_toolchain_releases_and_size_gates_enable_safe_icf to verify that scripts/baml-release-rustflags preserves pre-existing RUSTFLAGS, not just appends linker flags. Assert the workflow or script uses the preservation expression, or execute the script with a sentinel RUSTFLAGS value and confirm that value remains in the generated output.
656-668: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope workflow assertions to the actual jobs and ordering.
Raw
count()checks can pass when the script path appears in an unrelated step or comment; they do not prove the environment step runs inbuild-toolchainand both size-gate jobs before their build/check steps. Use the existing job/step helpers or parse YAML to assert placement.🤖 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 `@scripts/tests/test_release_pipeline_contract.py` around lines 656 - 668, Update test_unix_toolchain_releases_and_size_gates_enable_safe_icf to inspect parsed workflow jobs or use the existing job/step helpers instead of raw script-path count assertions. Assert the rustflags script is configured within build-toolchain and in both size-gate jobs, with each environment step appearing before that job’s build/check step; retain the existing rustflags and release-condition assertions.
🤖 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 `@scripts/tests/test_release_pipeline_contract.py`:
- Around line 656-668: Extend
test_unix_toolchain_releases_and_size_gates_enable_safe_icf to verify that
scripts/baml-release-rustflags preserves pre-existing RUSTFLAGS, not just
appends linker flags. Assert the workflow or script uses the preservation
expression, or execute the script with a sentinel RUSTFLAGS value and confirm
that value remains in the generated output.
- Around line 656-668: Update
test_unix_toolchain_releases_and_size_gates_enable_safe_icf to inspect parsed
workflow jobs or use the existing job/step helpers instead of raw script-path
count assertions. Assert the rustflags script is configured within
build-toolchain and in both size-gate jobs, with each environment step appearing
before that job’s build/check step; retain the existing rustflags and
release-condition assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2bfd4559-9735-4f69-acfe-7ac77375a5b2
📒 Files selected for processing (4)
.github/workflows/release-baml-language.yml.github/workflows/size-gate.reusable.yamlscripts/baml-release-rustflagsscripts/tests/test_release_pipeline_contract.py
Binary size checks passed✅ 7 passed
Generated by |
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 @.github/workflows/release-baml-language.yml:
- Around line 342-344: Update the “Enable safe ICF” workflow step so
scripts/baml-release-rustflags must complete successfully before modifying
GITHUB_ENV. Capture its output in a temporary shell variable, fail the step on
command failure, and only then append the generated RUSTFLAGS value without
overwriting an existing value.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c23b10de-9b29-4512-b1d0-056be0eb6f17
📒 Files selected for processing (3)
.github/workflows/release-baml-language.ymlscripts/baml-release-rustflagsscripts/tests/test_release_pipeline_contract.py
| - name: Enable safe ICF | ||
| if: ${{ !contains(matrix.target, 'windows-msvc') }} | ||
| run: echo "RUSTFLAGS=$(scripts/baml-release-rustflags '${{ matrix.target }}')" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp)"
trap 'rm -f "$tmp"' EXIT
bash -c 'set -e; echo "RUSTFLAGS=$(false)" >> "$1"' bash "$tmp"
test -s "$tmp"Repository: BoundaryML/baml
Length of output: 153
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== workflow context =="
sed -n '320,355p' .github/workflows/release-baml-language.yml
echo
echo "== rustflags script outline/search =="
if [ -f scripts/baml-release-rustflags ]; then
wc -l scripts/baml-release-rustflags
sed -n '1,220p' scripts/baml-release-rustflags
else
echo "scripts/baml-release-rustflags not found"
fd -a 'baml-release-rustflags|rustflags' .
fi
echo
echo "== related environment writes =="
rg -n 'RUSTFLAGS=|baml-release-rustflags|Enable safe ICF' .github/workflows/release-baml-language.ymlRepository: BoundaryML/baml
Length of output: 2789
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp)"
trap 'rm -f "$tmp"' EXIT
echo "== current single-line behavior =="
bash -c 'set -e; echo "RUSTFLAGS=$(false)" >> "$1"; echo "job status: ${PIPESTATUS[*]}"' bash "$tmp"
echo "file contents:"
cat -n "$tmp"
echo "exit: $?"
echo
echo "== proposed fail-closed behavior =="
bash -c 'set -euo pipefail; if ! rustflags="$(false)"; then exit 1; fi; printf "RUSTFLAGS=%s\n" "$rustflags" >> "$1"' bash "$tmp"
echo "proposal exit status: $?"Repository: BoundaryML/baml
Length of output: 280
Fail closed when Rustflags generation fails.
The single-line command substitution happens before echo’s exit status, so RUSTFLAGS=$(fake-fail) can produce RUSTFLAGS= and exit successfully. This can let the job continue without safe ICF or replace existing RUSTFLAGS; write the value only after the script succeeds.
🤖 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 @.github/workflows/release-baml-language.yml around lines 342 - 344, Update
the “Enable safe ICF” workflow step so scripts/baml-release-rustflags must
complete successfully before modifying GITHUB_ENV. Capture its output in a
temporary shell variable, fail the step on command failure, and only then append
the generated RUSTFLAGS value without overwriting an existing value.
codeshaunted
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/tests/test_release_pipeline_contract.py (1)
656-672: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRestore the RELR regression assertion.
This change removes the
pack-relative-relocscheck, but the PR objective explicitly preserves RELR being disabled for packed programs. Keep an equivalent negative assertion against the actual packed-program flags or configuration so a future regression remains covered.🤖 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 `@scripts/tests/test_release_pipeline_contract.py` around lines 656 - 672, Update test_macos_toolchain_releases_and_size_gate_enable_safe_icf to restore a negative assertion confirming pack-relative-relocs/RELR remains disabled for packed programs. Assert against the actual packed-program flags or configuration used by the release pipeline, while preserving the existing toolchain and linker assertions.
🤖 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 @.github/workflows/release-baml-language.yml:
- Line 344: Update the release workflow step that invokes
scripts/baml-release-rustflags so a failure from that script causes the step to
fail before writing to GITHUB_ENV. Preserve the existing RUSTFLAGS export on
success, and ensure an empty or failed result cannot overwrite existing flags.
---
Nitpick comments:
In `@scripts/tests/test_release_pipeline_contract.py`:
- Around line 656-672: Update
test_macos_toolchain_releases_and_size_gate_enable_safe_icf to restore a
negative assertion confirming pack-relative-relocs/RELR remains disabled for
packed programs. Assert against the actual packed-program flags or configuration
used by the release pipeline, while preserving the existing toolchain and linker
assertions.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 484ae1e8-70d8-4f41-80cb-8833a062810a
📒 Files selected for processing (4)
.github/workflows/release-baml-language.yml.github/workflows/size-gate.reusable.yamlscripts/baml-release-rustflagsscripts/tests/test_release_pipeline_contract.py
💤 Files with no reviewable changes (1)
- .github/workflows/size-gate.reusable.yaml
| target: ${{ matrix.target }} | ||
| - name: Enable safe ICF | ||
| if: ${{ contains(matrix.target, 'apple-darwin') }} | ||
| run: echo "RUSTFLAGS=$(scripts/baml-release-rustflags)" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail closed before writing GITHUB_ENV.
If scripts/baml-release-rustflags fails, the command substitution can still let echo succeed and write RUSTFLAGS=, so the build continues without safe ICF and may overwrite existing flags.
Proposed fix
- run: echo "RUSTFLAGS=$(scripts/baml-release-rustflags)" >> "$GITHUB_ENV"
+ run: |
+ set -euo pipefail
+ rustflags="$(scripts/baml-release-rustflags)"
+ printf 'RUSTFLAGS=%s\n' "$rustflags" >> "$GITHUB_ENV"🤖 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 @.github/workflows/release-baml-language.yml at line 344, Update the release
workflow step that invokes scripts/baml-release-rustflags so a failure from that
script causes the step to fail before writing to GITHUB_ENV. Preserve the
existing RUSTFLAGS export on success, and ensure an empty or failed result
cannot overwrite existing flags.
Summary
Size impact
Measured with the
aarch64-apple-darwinsize gate:baml-cliRemoving
LC_FUNCTION_STARTSaccounts for the additional 53,424 B reduction inbaml-cliand 34,448 B in the packed program. It does not affect runtime performance, but removes function-boundary metadata used by macOS profilers and symbolication tools.Linux experiments were not retained:
baml-cliby 8,832 B versus the default linker in consecutive CI runs.DT_RELRsaved roughly 1 MB, butbaml packcannot parse the resulting ELF section with the pinnedobjectimplementation. It would also require glibc 2.36 or newer.Verification
baml-cliandbaml-pack-hostrelease buildspython3 -m unittest scripts.tests.test_release_pipeline_contract(23 tests)