-
Notifications
You must be signed in to change notification settings - Fork 1
support debugger in max and try to support also linux. #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughBumps package version to 0.3.6 and extends build.rs with platform-specific post-copy adjustments: macOS uses install_name_tool to set library ids and additional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Cargo as Cargo/build
participant buildrs as build.rs
participant FS as FileSystem
participant mac as macOS(tooling)
participant linux as Linux(tooling)
Cargo->>buildrs: invoke build script
buildrs->>FS: copy catboost lib to OUT_DIR
alt target is macOS
buildrs->>mac: run install_name_tool on source lib (set id `@loader_path/`...)
buildrs->>FS: copy adjusted lib to final location
buildrs->>mac: run install_name_tool on copied lib (set id & rpath entries)
else target is Linux
buildrs->>linux: check for patchelf
opt patchelf available
buildrs->>linux: run patchelf on source lib (set soname)
buildrs->>FS: copy adjusted lib to final location
buildrs->>linux: run patchelf on copied lib (set soname & rpath entries)
end
opt patchelf unavailable
buildrs->>FS: leave rpath/soname unchanged (best-effort)
end
end
buildrs->>Cargo: emit link flags and rpath settings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
build.rs (1)
182-211: Excellent fix for the Linux SONAME issue!The critical issue from the previous review has been resolved. The SONAME is now correctly set to the plain library filename (lines 203, 208) instead of the problematic
$ORIGIN/...prefix. The best-effort approach using.output()to silently handle missingpatchelfis appropriate.Minor optional refinements:
- Consider moving
use std::process::Command;to the top of the function to avoid duplicate imports (lines 185, 198)- For consistency, macOS uses
.status()(lines 190, 196) while Linux uses.output()(lines 205, 210). Both work, but.output()is slightly better for suppressing stderr/stdout when silently ignoring failuresOptional: Consolidate Command import
+ use std::process::Command; + // On macOS/Linux, change the install name/soname to use @loader_path/$ORIGIN // This needs to be done on the source library in OUT_DIR before linking if os == "darwin" { - use std::process::Command; let _ = Command::new("install_name_tool") .arg("-id") .arg(format!("@loader_path/{}", lib_filename)) .arg(&lib_source_path) .status(); // Also update the copy let _ = Command::new("install_name_tool") .arg("-id") .arg(format!("@loader_path/{}", lib_filename)) .arg(&lib_dest_path) .status(); } else if os == "linux" { - use std::process::Command; // Use patchelf to set soname to just the library filename on Linux (if available) // This is optional - if patchelf is not installed, we just skip it let _ = Command::new("patchelf")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Docker image
🔇 Additional comments (2)
build.rs (2)
226-227: Good addition for debugger support on macOS.Adding
@loader_pathand@loader_path/../..rpaths complements the existing@executable_pathentries and helps the dynamic loader locate the library in debugger scenarios where the loading context may differ.
240-244: Good parity with macOS rpath handling.Adding the target/debug and target/release directories to Linux rpaths mirrors the macOS behavior and ensures consistent library discovery across build configurations, supporting the PR's debugger objectives.
Summary by CodeRabbit
Chores
Bug Fixes