-
Notifications
You must be signed in to change notification settings - Fork 13
Upgrade colored and thiserror #82
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
Signed-off-by: Eli Ma <eli@patch.sh>
|
Claude finished @genedna's task in 2m 21s —— View job Code Review: Dependency UpgradesI've completed a comprehensive review of this dependency upgrade PR. Here's my analysis: SummaryThis PR upgrades two Rust dependencies:
The changes affect 100 files, primarily auto-generated BUCK build files and lock files, which is expected for a dependency update. Detailed Findings1. Code Quality ✅Positive:
Note:
2. Security ✅Assessment:
Usage in codebase:
3. Performance ⚡Assessment:
4. Testing
|
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.
Pull request overview
This PR upgrades two key dependencies (colored from 3.0.0 to 3.1.1 and thiserror from 2.0.17 to 2.0.18) and performs extensive cleanup of third-party Rust crate BUCK build files. The changes include removing unused imports and adding platform-specific dependency handling using Buck2's select() statements to improve cross-platform build support.
Changes:
- Upgraded
coloredcrate from version 3.0.0 to 3.1.1 - Upgraded
thiserrorandthiserror-implfrom version 2.0.17 to 2.0.18 - Added platform-specific dependency configurations across ~200+ third-party crate BUCK files using
select()statements for Linux, macOS, and Windows - Removed unused Buck2 load imports (
buildscript_run,rust_binary) from BUCK files where they weren't being used - Updated buckal.snap with new version fingerprints and removed unused Windows crate version entries
Reviewed changes
Copilot reviewed 293 out of 294 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Updated colored to 3.1.1 and thiserror to 2.0.18 |
| Cargo.lock | Resolved dependency tree with new versions and removed unused windows-sys 0.59.0 entries |
| BUCK | Updated references to new crate versions and added Windows platform-specific rustc_flags handling |
| buckal.snap | Updated fingerprints and removed unused version entries |
| third-party/rust/crates/colored/3.1.1/BUCK | New BUCK file for colored 3.1.1 with Windows-specific windows-sys dependency |
| third-party/rust/crates/thiserror/2.0.18/BUCK | New BUCK file for thiserror 2.0.18 with build script support |
| third-party/rust/crates/thiserror-impl/2.0.18/BUCK | New BUCK file for thiserror-impl 2.0.18 proc macro |
| ~200+ third-party BUCK files | Removed unused imports and added platform-specific dependency handling |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 412a4b01a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ] + select({ | ||
| "DEFAULT": [], | ||
| "config//os:linux": ["//third-party/rust/crates/windows_x86_64_gnu/0.53.1:windows_x86_64_gnu"], | ||
| "config//os:windows": ["//third-party/rust/crates/windows_x86_64_msvc/0.53.1:windows_x86_64_msvc"], |
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.
Select GNU import libs on Windows GNU builds
When building on Windows with the GNU ABI (mingw), this select keys only on config//os:windows, so it always pulls windows_x86_64_msvc. That mismatches the GNU toolchain and will typically fail at link time because the MSVC import libs are incompatible. The repo already distinguishes GNU vs MSVC elsewhere (e.g., via prelude//abi/constraints:gnu in the top-level BUCK), so this should branch on ABI and choose windows_x86_64_gnu for GNU builds (same issue exists in the 0.48.5 windows-targets BUCK).
Useful? React with 👍 / 👎.
Description
Routine dependency maintenance:
coloredfrom 3.0.0 to 3.1.1 (minor version bump)thiserrorfrom 2.0.17 to 2.0.18 (patch version bump)Testing