Upgrade the buck2 support with newest buckal #83
Conversation
Signed-off-by: Eli Ma <eli@patch.sh>
|
Claude finished @genedna's task in 2m 58s —— View job Code Review Complete ✓Reviewed PR #83: Upgrade the buck2 support with newest buckal Tasks
SummaryThis PR upgrades the buck2 build system to use the newest version of
Overall AssessmentStatus: APPROVE with minor recommendations The upgrade appears well-structured and follows the new buckal conventions. However, there are several areas that need attention: Critical IssuesNone identified - the changes appear safe and consistent. High Priority Concerns1. Missing PR DescriptionSeverity: High The PR has no description explaining:
Recommendation: Add a PR description covering: - Buckal version being upgraded to
- Motivation for the upgrade (new features, bug fixes, etc.)
- Testing performed (cargo build, buck2 build, etc.)
- Breaking changes or migration notes2. Documentation GapFile: third-party/README.md (DELETED) The deletion of
Recommendation:
Medium Priority Issues3. .gitignore Change UnclearFile: .gitignore:12 -examples/output_packs
+examples/output_packs/buck-outThis change narrows the ignore pattern from ignoring the entire Questions:
Recommendation: Clarify the intention behind this change in the PR description. 4. Windows Support Dependencies RemovedFiles:
These Windows-specific dependencies were removed, but:
Recommendation:
5. Toolchain Configuration SimplifiedFile: toolchains/BUCK The toolchain configuration was dramatically simplified:
Observations:
Recommendation: Either:
6. New Toolchain Helper FilesFiles Added:
These appear to be Windows MSVC linker wrappers, but:
Recommendation: Add header comments explaining their purpose and usage. Low Priority / Style Issues7. All BUCK Files Now Marked @generatedAll modified BUCK files now start with: # @generated by `cargo buckal`This is correct and follows the new buckal pattern. However:
8. Third-Party BUCK Pattern ChangesThe third-party BUCK files now use a more consistent pattern:
Example (colored/3.1.1/BUCK): http_archive(
name = "colored-vendor",
urls = ["https://static.crates.io/crates/colored/colored-3.1.1.crate"],
sha256 = "faf9468729b8cbcea668e36183cb69d317348c2e08e994829fb56ebfdfbaac34",
...
)This is good - improves reproducibility and hermetic builds. 9. Buckal Bundle ConfigurationFile: .buckconfig:24-26 [external_cell_buckal]
commit_hash = 22bd38c79d2348d9a6591b7156c42d615377eaad
git_origin = https://github.com/buck2hub/buckal-bundlesThis pins buckal to a specific commit. Good for reproducibility, but:
Recommendation: Add a comment above this section noting the buckal version/date. Testing RecommendationsSince I cannot run buck2 commands without the tool installed, I recommend:
Code Quality AssessmentPositive Aspects ✓
Security ✓
Performance ✓
Final Recommendations
Overall, this is a solid upgrade that modernizes the buck2 configuration. With the documentation gaps addressed, this should be good to merge. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc161c193c
ℹ️ 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".
| rustc_target_triple = select({ | ||
| "prelude//os/constraints:linux": "x86_64-unknown-linux-gnu", | ||
| "prelude//os/constraints:macos": "aarch64-apple-darwin", | ||
| "prelude//os/constraints:windows": "x86_64-pc-windows-msvc", |
There was a problem hiding this comment.
The new toolchain hardcodes prelude//os/constraints:macos to aarch64-apple-darwin, which forces Intel macs to build for arm64. On x86_64 macOS, rustc will try to cross-compile and typically fails unless the arm64 stdlib/SDK is installed, which is a regression from the prior host-based mapping. Consider selecting on CPU constraints (e.g., prelude//cpu/constraints:x86_64 vs arm64) or adding an x86_64 triple so Intel mac builds still target their native architecture.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR upgrades the Buck2 build system integration by migrating to the newest version of buckal (cargo-buckal). The upgrade modernizes the build configuration with improved toolchain definitions, platform specifications, and dependency management patterns.
Changes:
- Added custom toolchain configurations (Rust and C++ with OS-specific compiler selection)
- Migrated dependency patterns from
select()toos_deps/os_named_depsfor cleaner syntax - Added platform definitions for cross-compilation support
- Updated buckal bundle to newer commit (22bd38c79d2348d9a6591b7156c42d615377eaad)
Reviewed changes
Copilot reviewed 56 out of 57 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
toolchains/rust/demo_rust.bzl |
New Rust toolchain with platform-specific target triples |
toolchains/cxx/demo_cxx.bzl |
New C++ toolchain avoiding lld requirement, using gcc/bfd on Linux |
toolchains/cxx/tools/lld_link.py |
Python wrapper for Windows linker, auto-detecting Rust sysroot |
toolchains/cxx/tools/*.bat |
Architecture-specific Windows batch launchers for linker |
toolchains/BUCK |
Simplified toolchain setup using new demo configurations |
platforms/BUCK |
New platform definitions for aarch64-darwin, x86_64-windows-msvc, x86_64-linux-gnu |
PACKAGE |
Added cfg modifiers and constructor for build mode aliases |
.buckconfig |
Updated buckal bundle commit, alphabetized cells |
BUCK |
Removed test targets (likely intentional for buckal-generated code) |
third-party/rust/crates/*/BUCK |
Migrated 30+ crate BUCK files from select() to os_deps pattern |
third-party/rust/crates/*/0.52.6/BUCK |
Removed unused Windows crate versions (0.52.6) |
third-party/README.md |
Removed buckal installation/usage documentation |
.gitignore |
Made pattern more specific (buck-out subdir only) |
buckal.toml |
Removed configuration file |
|
|
||
| g++ -L"%SELF_CONTAINED%" %* | ||
| set "STATUS=!ERRORLEVEL!" | ||
| exit /b %STATUS% |
There was a problem hiding this comment.
With EnableDelayedExpansion enabled, variable expansion should use !VAR! syntax for consistency, especially when the variable was just set. The STATUS variable is set on the previous line, so it should be accessed using !STATUS! instead of %STATUS% to ensure the correct value is used.
| @@ -1,43 +1,21 @@ | |||
| # @generated by `cargo buckal` | |||
|
|
|||
| load("@//cxx:demo_cxx.bzl", "system_demo_cxx_toolchain") | |||
There was a problem hiding this comment.
The load path @//cxx:demo_cxx.bzl is incorrect. When loading from within the toolchains cell, @// refers to the root cell, not the toolchains cell. The file cxx/demo_cxx.bzl does not exist at the root of the repository; it exists at toolchains/cxx/demo_cxx.bzl. The correct path should be //cxx:demo_cxx.bzl (without the @) to load from the current cell, or @toolchains//cxx:demo_cxx.bzl to explicitly reference the toolchains cell.
| "system_python_toolchain", | ||
| ) | ||
| load("@prelude//toolchains:remote_test_execution.bzl", "remote_test_execution_toolchain") | ||
| load("@//rust:demo_rust.bzl", "system_demo_rust_toolchain") |
There was a problem hiding this comment.
The load path @//rust:demo_rust.bzl is incorrect. When loading from within the toolchains cell, @// refers to the root cell, not the toolchains cell. The file rust/demo_rust.bzl does not exist at the root of the repository; it exists at toolchains/rust/demo_rust.bzl. The correct path should be //rust:demo_rust.bzl (without the @) to load from the current cell, or @toolchains//rust:demo_rust.bzl to explicitly reference the toolchains cell.
| # @generated by `cargo buckal` | ||
|
|
||
| load("@prelude//cfg/modifier:set_cfg_modifiers.bzl", "set_cfg_modifiers") | ||
| load("@prelude//rust:with_workspace.bzl", "with_rust_workspace") |
There was a problem hiding this comment.
The import with_rust_workspace is loaded but never used in this file. This import should be removed to keep the code clean and avoid confusion.
| load("@prelude//rust:with_workspace.bzl", "with_rust_workspace") |
| if not defined NEED_SELF_CONTAINED ( | ||
| g++ %* | ||
| set "STATUS=!ERRORLEVEL!" | ||
| exit /b %STATUS% |
There was a problem hiding this comment.
Within a compound statement (parentheses block) with EnableDelayedExpansion, variable expansion should use !VAR! syntax instead of %VAR%. The STATUS variable is set on the previous line within this block, so it should be accessed using !STATUS! instead of %STATUS% to get the correct value. The current code may exit with an undefined or stale value.
1 participant
No description provided.