fix(windows): trim duplicate COFF archive members - #7085
Conversation
📝 WalkthroughWalkthroughWindows UI archive linking now removes fully duplicated COFF members using symbol evidence, resolves LLVM tools beside ChangesWindows COFF deduplication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WindowsBuild
participant build_and_run_link
participant strip_duplicate_objects_from_lib
participant llvm_ar
participant llvm_nm
WindowsBuild->>build_and_run_link: build Windows target
build_and_run_link->>strip_duplicate_objects_from_lib: trim UI archive
strip_duplicate_objects_from_lib->>llvm_ar: list and rebuild COFF members
strip_duplicate_objects_from_lib->>llvm_nm: inspect member symbols
build_and_run_link->>build_and_run_link: link with trimmed archive
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/test.yml:
- Around line 875-877: Update the “Test COFF duplicate-symbol archive trimming”
step so it fails when the filtered Cargo test matches zero tests, rather than
accepting Cargo’s successful empty run. Preserve the existing
`coff_archive_dedup_drops_only_fully_provided_members` filter and
`perry-dev`/`perry` target, and assert from the test output or equivalent status
check that at least one test executed.
In `@crates/perry/src/commands/compile/strip_dedup.rs`:
- Around line 605-609: Update the Windows validation around provided_symbols and
staticlib_member_symbols so it distinguishes llvm-nm parsing failure from
legitimately empty symbols when nm_works is true but no stdlib_path,
runtime_path, or rlib was found. Report the COFF archive read error only for the
nm failure case, and provide a separate diagnostic for missing runtime/stdlib
libraries; preserve successful handling when symbols are empty for an explicitly
valid reason.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 120e4643-24c0-42b0-afa2-258884aef777
📒 Files selected for processing (6)
.github/workflows/test.ymlchangelog.d/7085-windows-coff-dedup.mdcrates/perry/src/commands/compile/link/build_and_run.rscrates/perry/src/commands/compile/link/platform_cmd.rscrates/perry/src/commands/compile/strip_dedup.rscrates/perry/src/commands/compile/strip_dedup/stub_symbols.rs
💤 Files with no reviewable changes (1)
- crates/perry/src/commands/compile/strip_dedup/stub_symbols.rs
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/test.yml:
- Around line 875-885: Update the protected branch configuration to include the
windows-build status check as a required context, matching the existing job name
in test.yml. Leave the COFF regression test step unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 609d08b1-db3f-485e-89ae-359b306fab83
📒 Files selected for processing (2)
.github/workflows/test.ymlcrates/perry/src/commands/compile/strip_dedup.rs
| - name: Test COFF duplicate-symbol archive trimming | ||
| shell: bash | ||
| run: | | ||
| set -o pipefail | ||
| cargo test --profile perry-dev -p perry --bin perry \ | ||
| coff_archive_dedup_drops_only_fully_provided_members 2>&1 | tee test-output.log | ||
| grep -q 'test result: ok. 1 passed; 0 failed' test-output.log || { | ||
| echo "::error::expected COFF dedup test did not run exactly once" | ||
| exit 1 | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow files =="
git ls-files .github/workflows | sort
echo "== test.yml around referenced lines =="
if [ -f .github/workflows/test.yml ]; then
nl -ba .github/workflows/test.yml | sed -n '840,900p'
else
echo ".github/workflows/test.yml not found"
fi
echo "== job names in workflows =="
rg -n "^( )+[A-Za-z0-9_-_]+\s*:|name:\s*(windows-build|.*windows.*|windows.*COFF|COFF duplicate|COFF dedup)" .github/workflows -g '*.yml' -g '*.yaml' || true
echo "== required status check references =="
rg -n "required_status_checks|windows-build|windows-build|branch protection|required contexts|required context|COFF|dedup|windows" .github README* . 2>/dev/null | head -200 || trueRepository: PerryTS/perry
Length of output: 879
🌐 Web query:
GitHub required status checks branch protection require checks before merging include jobs workflow syntax required_status_checks
💡 Result:
GitHub branch protection rules allow you to require specific status checks to pass before a pull request can be merged into a protected branch [1][2]. Core Concepts and Configuration: - Required Status Checks: These are configured under branch protection rules [1]. A check must have a successful, skipped, or neutral status to allow merging [2][3]. - Strict vs. Loose: - Strict: Enabled by the "Require branches to be up to date before merging" setting [2]. The head branch must be updated with the base branch before merging is permitted [2]. - Loose: This setting is disabled [2]. The head branch does not need to be up to date, which reduces the number of required builds but may lead to status check failures after merging if incompatible changes exist [2]. - Workflow Syntax: GitHub Actions workflows are defined in YAML [4]. While there is no specific required_status_checks key within a workflow file itself, individual jobs within a workflow act as status checks [4]. If a workflow is skipped (e.g., due to path/branch filtering or commit messages), its associated checks remain in a "Pending" state, which will block merging if those checks are marked as required [4][5]. API and Infrastructure as Code: - When using the GitHub REST API or tools like Terraform to manage branch protection, the required_status_checks object is used to define these requirements [6][7][8]. - The object includes [6][7][8]: - strict (boolean): Controls the "up to date" requirement [6][8]. - checks (array of objects): Specifies the required checks [6][7]. Each object contains a context (string) and an app_id (integer or null) to uniquely identify the check [6][8]. - contexts (array of strings): A deprecated field previously used for the same purpose [6][8]. Troubleshooting: - If a workflow condition causes a job to be skipped, the check will not report success, effectively blocking the merge [4][5]. To address this, ensure that all required checks are configured to run under the conditions present in the pull request, or maintain a secondary "dummy" workflow that always succeeds to satisfy the requirement when the primary workflow is skipped [5].
Citations:
- 1: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/managing-a-branch-protection-rule
- 2: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches?require-status-checks-before-merging=
- 3: https://docs.github.com/en/pull-requests/reference/status-checks
- 4: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax?use_case=
- 5: https://stackoverflow.com/questions/77996177/how-can-i-handle-a-required-check-that-isnt-always-triggered
- 6: https://docs.github.com/en/rest/branches/branch-protection
- 7: https://docs.github.com/en/rest/branches/branches?apiVersion=2026-03-10
- 8: https://registry.terraform.io/providers/integrations/github/5.43.0/docs/resources/branch_protection_v3
🏁 Script executed:
#!/bin/bash
set -u
echo "== workflow files =="
git ls-files .github/workflows | sort
echo "== test.yml around referenced lines =="
if [ -f .github/workflows/test.yml ]; then
awk '{printf "%6d\t%s\n", NR, $0}' .github/workflows/test.yml | sed -n '840,905p'
else
echo ".github/workflows/test.yml not found"
fi
echo "== all workflow job names =="
awk '
/^[[:space:]]*$|^\s*#|$0 ~ /^[[:space:]]+name:/ { next }
/^\s{2}[A-Za-z0-9._-]+\s*:$/ {
sub(/[[:space:]]:/, "", $2)
print FILENAME ":" NR ": " $2
}
' .github/workflows/*.yml .github/workflows/*.yaml 2>/dev/null | sort
echo "== focused search for windows-build and related comments =="
rg -n "windows-build|windows-build|windows-build|windows.*build|COFF|duplicate-symbol|duplicate_symbol|dedup_drops|required|branch protection|contexts|required status|branch_protection|branch-protection|protection" .github/workflows/test.yml .github/workflows/*.yml README* .github 2>/dev/null || trueRepository: PerryTS/perry
Length of output: 13331
Make windows-build a required branch-protection check.
The test.yml comment at .github/workflows/test.yml:914-915 explicitly says windows-build is not yet in branch protection’s required contexts, so this COFF regression check can fail without blocking a merge. Add windows-build as a required status check in the protected branch configuration.
🤖 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/test.yml around lines 875 - 885, Update the protected
branch configuration to include the windows-build status check as a required
context, matching the existing job name in test.yml. Leave the COFF regression
test step unchanged.
Source: Coding guidelines
1 participant
Closes #6626
Summary
llvm-ar/llvm-nmbeside the resolvedlld-linkfor prebuilt Windows installs/FORCE:MULTIPLEonly for mixed-symbol members and external native libraries that cannot be removed wholesaleValidation
cargo test -p perry --bin perry strip_dedup_tests(6/6 on host)cargo fmt --all -- --checkgit diff --checkactionlint .github/workflows/test.yml(only pre-existing shellcheck findings)The required Windows build job now runs the real COFF regression test.
Summary by CodeRabbit
.librebuilding and correct removal of only fully duplicated members.