fix: unblock main CI — dead-stripped symbol + cargo-test staticlib build + oversized link/mod.rs#5406
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThree CI breakages are fixed: a CI workflow update ensures ChangesCI unblock fixes and module extraction
Sequence Diagram(s)sequenceDiagram
participant Caller
participant build_and_run_link
participant select_linker_command
participant cargo_build as cargo build
participant pkg_config
participant Linker
Caller->>build_and_run_link: args_input, ctx, target, obj_paths, …
build_and_run_link->>build_and_run_link: verify perry.lock gate
build_and_run_link->>select_linker_command: compute platform booleans
select_linker_command-->>build_and_run_link: base linker command
build_and_run_link->>build_and_run_link: append objects, stdlib/runtime, platform system libs, UI archive
loop each native_library manifest
build_and_run_link->>cargo_build: sandboxed cargo build (tier3/Android/HarmonyOS env)
cargo_build-->>build_and_run_link: native archive path
build_and_run_link->>pkg_config: GTK4/GStreamer/WebKit flags (Linux UI)
pkg_config-->>build_and_run_link: linker flags
end
build_and_run_link->>build_and_run_link: embed Info.plist, prepare link-cache, optional response-file rewrite
build_and_run_link->>Linker: execute final linker command
Linker-->>build_and_run_link: exit status
build_and_run_link-->>Caller: Ok(LinkCacheStatus) or Err
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 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 `@CHANGELOG.md`:
- Around line 1-27: The CHANGELOG.md file has been modified in this PR, but
external contributor PRs must not touch CHANGELOG.md according to the
repository's coding guidelines. Remove all changes to CHANGELOG.md from this PR
using git (e.g., git checkout HEAD -- CHANGELOG.md or by unstaging it), as the
maintainer will add the version block and write the changelog entry at merge
time as part of the release workflow.
In `@crates/perry/src/commands/compile/link/build_and_run.rs`:
- Around line 181-182: The issue is that when name_str equals triple, the root
path construction at lines 204-205 is incorrectly joining the triple again,
resulting in target/<triple>/<triple> instead of target/<triple>. To fix this,
modify the logic so that when name_str == triple (the HarmonyOS fallback case),
the root path is set to target/<triple> without joining the triple component
again. The fix involves adjusting the condition or the path construction logic
to distinguish between the perry-auto- prefix case and the exact triple match
case, ensuring the correct build directory path is used for locating the
required .o inputs.
🪄 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: c066ce43-85cf-4243-b252-10ac3ffb1423
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
CHANGELOG.mdCLAUDE.mdCargo.tomlcrates/perry/src/commands/compile/link/build_and_run.rscrates/perry/src/commands/compile/link/mod.rs
Comment on lines
+1
to
+27
| ## v0.5.1184 — chore(link): extract build_and_run_link into its own module (file-size gate) | ||
|
|
||
| `crates/perry/src/commands/compile/link/mod.rs` crossed the 2000-line file-size | ||
| gate (2132 lines) after #5400 grew the Windows response-file path, turning the | ||
| `lint` job red for every PR branched off main. Split the single ~1770-line | ||
| `build_and_run_link` orchestrator (the only oversized item) into a sibling | ||
| `link/build_and_run.rs`, leaving mod.rs at 357 lines and the new file at 1785. | ||
|
|
||
| Pure mechanical move, no behavior change: | ||
|
|
||
| - The function moved verbatim; `use super::*` pulls in every helper that stays in | ||
| the parent `link` module (the `NativeBackendLinkMetadata` selection, the | ||
| `resolve_optional_framework_dir` / `find_project_root_for` / | ||
| `rewrite_link_with_response_file` / `quote_response_arg` / `response_file_contents` | ||
| helpers, and the sibling-module re-exports). | ||
| - `build_and_run_link` is now `pub(crate)` (was `pub(super)`) so mod.rs can | ||
| re-export it to the parent `compile` module via | ||
| `pub(super) use build_and_run::build_and_run_link;`. | ||
| - The five fully-qualified paths that reached into the `compile` module | ||
| (`library_search::`, `sandbox_buildrs::`, `optimized_libs::`, | ||
| `run_lock_verify_for_compile`, `find_perry_workspace_root`) became | ||
| `super::super::…` from the new two-level-deep module. | ||
|
|
||
| All 599 perry bin unit tests pass (including the link `response_file_tests` / | ||
| `native_package_selection_tests` / `optional_framework_dir_tests`); a hello-world | ||
| compile+link round-trips through the moved driver. | ||
|
|
There was a problem hiding this comment.
Drop this changelog entry from the PR.
As per coding guidelines, external contributor PRs must not touch CHANGELOG.md; the maintainer adds the version block at merge time. Keeping it here conflicts with the repo’s release workflow.
As per coding guidelines, “External contributor PRs must NOT touch [workspace.package] version in Cargo.toml, the **Current Version:** line in CLAUDE.md, or CHANGELOG.md; the maintainer bumps the version and writes the changelog entry at merge time.”
🤖 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 `@CHANGELOG.md` around lines 1 - 27, The CHANGELOG.md file has been modified in
this PR, but external contributor PRs must not touch CHANGELOG.md according to
the repository's coding guidelines. Remove all changes to CHANGELOG.md from this
PR using git (e.g., git checkout HEAD -- CHANGELOG.md or by unstaging it), as
the maintainer will add the version block and write the changelog entry at merge
time as part of the release workflow.
Sources: Coding guidelines, Learnings
Comment on lines
+181
to
+182
| if name_str.starts_with("perry-auto-") || name_str == triple { | ||
| roots.push(entry.path()); |
There was a problem hiding this comment.
HarmonyOS fallback root composes an invalid build/ path
When name_str == triple, root is set to target/<triple>, then root.join(triple) produces target/<triple>/<triple>/.... That misses the real non-auto Cargo build dir and can drop required .o inputs.
Suggested fix
- if name_str.starts_with("perry-auto-") || name_str == triple {
- roots.push(entry.path());
- }
+ if name_str.starts_with("perry-auto-") {
+ roots.push(entry.path());
+ } else if name_str == triple {
+ // Non-auto layout lives under target/<triple>/release/build,
+ // so use target/ as the root before appending <triple>.
+ roots.push(std::path::PathBuf::from("target"));
+ }Also applies to: 204-205
🤖 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/link/build_and_run.rs` around lines 181 -
182, The issue is that when name_str equals triple, the root path construction
at lines 204-205 is incorrectly joining the triple again, resulting in
target/<triple>/<triple> instead of target/<triple>. To fix this, modify the
logic so that when name_str == triple (the HarmonyOS fallback case), the root
path is set to target/<triple> without joining the triple component again. The
fix involves adjusting the condition or the path construction logic to
distinguish between the perry-auto- prefix case and the exact triple match case,
ensuring the correct build directory path is used for locating the required .o
inputs.
proggeramlug
force-pushed
the
chore/split-link-mod-file-size
branch
from
709fc36 to
0534e25
Compare
proggeramlug
changed the title
…oversized link/mod.rs Three pre-existing CI fragilities on main turning required checks red for PRs: 0. cargo-test runs 'cargo test -p perry-runtime', which never builds the staticlib crate-type, so libperry_runtime.a/libperry_stdlib.a only exist from cache. A PR touching perry-runtime invalidates the cached staticlib and cargo never rebuilds it -> PERRY_NO_AUTO_OPTIMIZE=1 tests fail with 'Could not find libperry_runtime.a'. Fix: add 'cargo build -p perry-runtime -p perry-stdlib' to the cargo-test job before the integration-test loop. 1. js_array_numeric_value_to_raw_f64 (#5291) is #[no_mangle] but called only from generated code, so the linker dead-strips it from libperry_runtime.a -> undefined symbol at link. Fix: #[used] keepalive anchor. 2. link/mod.rs crossed the 2000-line file-size gate (2132) after #5400. Split the ~1770-line build_and_run_link orchestrator into link/build_and_run.rs.
proggeramlug
force-pushed
the
chore/split-link-mod-file-size
branch
from
0534e25 to
bffb4ac
Compare
proggeramlug
changed the title
This was referenced Jun 18, 2026
nglmercer
pushed a commit
to nglmercer/perry
that referenced
this pull request
Jun 18, 2026
…lict in compile/link/mod.rs Brings main's PerryTS#5400 (Windows response-file link) + PerryTS#5406/PerryTS#5408/PerryTS#5410/PerryTS#5412 codegen split + PerryTS#5402 require Tier 2 + PerryTS#5414 unknown builtin-namespace fix into the Windows plugin support branch. The conflict in compile/link/mod.rs is structural: main extracted the orchestrator into link/build_and_run.rs while the branch kept it inline. Resolved by taking main's refactor and re-applying the Windows plugin support on top: - compile/link/mod.rs: add PLUGIN_HOST_SYMBOLS const (the runtime + plugin export list — single source of truth for the macOS -u force-keep and the Windows .def file) plus use std::io::Write for the .def writer. - compile/link/build_and_run.rs: convert the if ctx.needs_plugins && !is_windows block to if ctx.needs_plugins with platform branches. The Windows branch writes a per-build perry_plugin_host_<pid>.def using the NAME directive (not LIBRARY — that one tells link.exe the output is a DLL and breaks the host .exe) and passes /DEF:<path> to the linker. PLUGIN_HOST_SYMBOLS replaces the inline untime_syms array on macOS too. - compile.rs: dylib link path on Windows now uses lld-link with /FORCE:UNRESOLVED (the .def file lists plugin_activate / perry_plugin_abi_version / plugin_deactivate so lld-link's empty default export table doesn't break GetProcAddress in the host) and writes a per-build .def file with the same three symbols. Lifts the pre-existing 'use link.exe' out for the LLVM-friendly linker. - codegen/entry.rs: re-apply the dylib plugin ABI shim block (emits perry_plugin_abi_version, plugin_activate, and the optional plugin_deactivate) on top of main's refactor of compile_module_entry.
This was referenced Jun 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three pre-existing CI fragilities on
mainthat were turning required checks red for PRs (including #5402). Bundled because all are "make main's CI green again."0.
cargo-testnever builds the runtime staticlib (root cause of the "Could not find" failures)The
cargo-testjob runscargo test -p perry-runtime, which builds only lib/bin/test targets — not thestaticlibcrate-type — solibperry_runtime.a/libperry_stdlib.aare never produced by the job and only exist when restored from the rust-cache. Integration tests that compile withPERRY_NO_AUTO_OPTIMIZE=1(e.g.functional_batch2_regressions) link the prebuilt archive directly, so the moment a PR touches perry-runtime/perry-stdlib the cached staticlib is invalidated, cargo never rebuilds it, and those tests fail withCould not find libperry_runtime.a. Fix: add an explicitcargo build -p perry-runtime -p perry-stdlibto the cargo-test job before the integration-test loop. (Verified locally:cargo test -p perry-runtime --no-rundoes not produce the.a;cargo build -p perry-runtimedoes.)1. dead-stripped runtime symbol → undefined symbol at link
js_array_numeric_value_to_raw_f64(added by #5291) is#[no_mangle]but called only from generated machine code, so without a#[used]anchor the linker dead-strips it fromlibperry_runtime.a→Undefined symbols: _js_array_numeric_value_to_raw_f64. Fix: add the#[used]keepalive anchor.2.
link/mod.rsover the file-size gate (lint)Crossed 2000 lines (2132) after #5400. Split the ~1770-line
build_and_run_linkorchestrator intolink/build_and_run.rs(mod.rs → 357, new file → 1785; pure mechanical move).Verification
cargo fmt --checkclean.functional_batch2_regressions5/5 pass locally with the#[used]fix; 599 perry bin tests pass; hello-world compile+link round-trips.Summary by CodeRabbit
cargo-testjob.