fix(link): preserve native members in Windows UI dedup - #7920
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Windows linker now includes three additional system libraries. Archive extraction resolves path-qualified members by basename, excludes Windows import members, and assigns unique normalized filenames. Tests cover library emission, member resolution, filename handling, and import-member classification. ChangesWindows linking and archive handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/perry/src/commands/compile/strip_dedup.rs (1)
1892-1914: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover extraction and basename collisions end to end.
The new path test creates
loader_impl.objmanually and callsextracted_archive_member. It does not exercisellvm-ar x,std::fs::rename, orrebuild_archive. It cannot detect two path-qualified members with the same basename overwriting each other.Extend the existing archive fixture at Lines 1818-1891 with two members such as
a/loader_impl.objandb/loader_impl.obj, then assert that both symbol sets remain in the rebuilt archive.🤖 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 `@crates/perry/src/commands/compile/strip_dedup.rs` around lines 1892 - 1914, Extend the existing archive fixture and end-to-end rebuild test around the archive construction and rebuild flow, adding path-qualified members such as a/loader_impl.obj and b/loader_impl.obj with distinct symbols. Run the actual extraction, rename, and rebuild_archive path, then inspect the rebuilt archive to assert both members’ symbol sets are preserved and neither basename collision overwrites the other.
🤖 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 `@crates/perry/src/commands/compile/strip_dedup.rs`:
- Line 736: Update the archive member filtering condition around
is_windows_import_archive_member(m) to also require is_win_lib, so import-member
removal only occurs for Windows archives while ordinary foo.dll or foo.drv
members in other archives remain intact.
- Around line 817-824: Update the rlib extraction loop around
extracted_archive_member and the corresponding staticlib loop to count every
member for which extraction returns None, while preserving the existing
successful-object handling. Before calling rebuild_archive, return an error when
either loop recorded any extraction failures, so incomplete archives are never
rebuilt or returned as successful.
---
Nitpick comments:
In `@crates/perry/src/commands/compile/strip_dedup.rs`:
- Around line 1892-1914: Extend the existing archive fixture and end-to-end
rebuild test around the archive construction and rebuild flow, adding
path-qualified members such as a/loader_impl.obj and b/loader_impl.obj with
distinct symbols. Run the actual extraction, rename, and rebuild_archive path,
then inspect the rebuilt archive to assert both members’ symbol sets are
preserved and neither basename collision overwrites the other.
🪄 Autofix
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: 39f32916-38b6-4a18-b14c-7463b1b6264d
📒 Files selected for processing (2)
crates/perry/src/commands/compile/link/windows_link.rscrates/perry/src/commands/compile/strip_dedup.rs
| .iter() | ||
| .filter(|m| { | ||
| if m.ends_with(".dll") { | ||
| if is_windows_import_archive_member(m) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Restrict import-member filtering to Windows archives.
This check runs for every archive. If a non-Windows archive contains an ordinary member named foo.dll or foo.drv, the code drops it as an import member. Gate the predicate with is_win_lib.
Suggested fix
- if is_windows_import_archive_member(m) {
+ if is_win_lib && is_windows_import_archive_member(m) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if is_windows_import_archive_member(m) { | |
| if is_win_lib && is_windows_import_archive_member(m) { |
🤖 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 `@crates/perry/src/commands/compile/strip_dedup.rs` at line 736, Update the
archive member filtering condition around is_windows_import_archive_member(m) to
also require is_win_lib, so import-member removal only occurs for Windows
archives while ordinary foo.dll or foo.drv members in other archives remain
intact.
| if let Some(extracted) = extracted_archive_member(&extract_dir, member) { | ||
| // Move every extracted object to a unique flat name before | ||
| // extracting the next member. Two path-qualified members | ||
| // may share a basename, and llvm-ar would otherwise | ||
| // overwrite the earlier one. | ||
| let normalized = extract_dir.join(format!("rlib_{member_index}.obj")); | ||
| std::fs::rename(extracted, &normalized)?; | ||
| all_objects.push(normalized); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return an error when any member extraction fails.
When llvm-ar succeeds but extracted_archive_member returns None, the rlib loop silently omits the member. The staticlib loop records the failure but continues to rebuild_archive. The downstream code in crates/perry/src/commands/compile/link/build_and_run.rs at Lines 985-1010 uses the rebuilt archive for Ok and falls back to the original archive only for Err. This can produce an incomplete UI archive and remove required symbols.
Count failures in both loops. Return Err before rebuilding when any extraction fails.
Suggested failure handling
+ let mut extract_fail = 0usize;
+
// Extract rlib members.
...
if let Some(extracted) = extracted_archive_member(&extract_dir, member) {
...
all_objects.push(normalized);
rlib_extracted += 1;
+ } else {
+ extract_fail += 1;
}
+ } else {
+ extract_fail += 1;
}
...
if extract_fail > 0 {
eprintln!("[strip-dedup] WARNING: {extract_fail} members failed to extract from staticlib");
+ return Err(anyhow::anyhow!(
+ "failed to extract {extract_fail} archive members"
+ ));
}Also applies to: 838-859
🤖 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 `@crates/perry/src/commands/compile/strip_dedup.rs` around lines 817 - 824,
Update the rlib extraction loop around extracted_archive_member and the
corresponding staticlib loop to count every member for which extraction returns
None, while preserving the existing successful-object handling. Before calling
rebuild_archive, return an error when either loop recorded any extraction
failures, so incomplete archives are never rebuilt or returned as successful.
1 participant
Summary
obj/.../*.objmembers).drvmembers as Windows import-library members alongside.dll, and supplyuxtheme.lib,winspool.lib, andrpcrt4.libfrom the canonical system link lineWhy
#6051 fixed #6023's original
LNK1158: cannot run mt.exe, but the TextField half was never verified. During a current-main Windows validation,perry/uilinking failed earlier because UI archive dedup silently omitted WebView2's path-qualified objects and retained an incomplete flattened subset ofwinspool.drvmembers. This leftCreateCoreWebView2EnvironmentWithOptions,SetWindowTheme,UuidCreate, and winspool descriptor/thunk symbols unresolved.With the archive fix applied, the current Windows UI fixture links and runs. Sending
EN_CHANGEthrough the TextField's actual immediatePerryVStackparent reaches the callback and prints the updated value; Toggle callback dispatch also succeeds as a control. The historical mt.exe discovery/fallback tests remain green.Validation
cargo test -p perry --no-default-features --features dev-cli --bin perry windows_link -- --nocapture— 22 passedcargo test -p perry --no-default-features --features dev-cli --bin perry commands::compile::strip_dedup::strip_dedup_tests -- --nocapture— 9 passedcargo build --release -p perry-runtime-static -p perry-ui-windowstest-files/test_ui_controls.tslinked successfully on Windows with the coherent current-main runtime/UI archivesEN_CHANGEthroughPerryVStackemittedName: parent-route; direct app-route control emittedName: top-route; Toggle emittedNotifications: 1cargo fmt --check -p perryCloses #6023
Summary by CodeRabbit