Conversation
WalkthroughReplaces direct per-platform LightGBM binary downloads with wheel-based downloads/extraction in Changes
Sequence Diagram(s)sequenceDiagram
participant Cargo as cargo build
participant BuildRs as build.rs
participant WheelHost as wheel host (HTTP)
participant Zip as wheel (.whl) extractor
participant FS as filesystem (libs/)
note over BuildRs: detect OS & arch -> choose wheel URL
Cargo->>BuildRs: run build script
BuildRs->>WheelHost: GET wheel_url
WheelHost-->>BuildRs: HTTP response (check status)
alt 200 OK
BuildRs->>Zip: open .whl (zip) and search for lib_lightgbm.*
alt linux/macos
Zip-->>BuildRs: found shared lib (.so / .dylib)
BuildRs->>FS: write lib to `libs/`
BuildRs-->>Cargo: emit rustc-link-search, rustc-link-lib, rpath entries
else windows
Zip-->>BuildRs: found `lib_lightgbm.dll` and `lib_lightgbm.lib`
BuildRs->>FS: write both to `libs/`
BuildRs-->>Cargo: emit link-search for import lib, warn about runtime DLL
end
else non-200
BuildRs-->>Cargo: fail build with descriptive HTTP error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Cargo.toml(2 hunks)build.rs(2 hunks)
🔇 Additional comments (7)
Cargo.toml (4)
12-13: Polars dependency configuration looks reasonable.The optional dependency with
default-features = falseis a good practice to minimize bloat.
18-18: Appropriate build dependency for wheel extraction.The
zipcrate is correctly added as a build dependency to support extracting the LightGBM library from Python wheel files.
23-23: LGTM!Standard feature flag pattern correctly configured.
33-36: Example configuration is correct.The
required-featuresattribute properly gates the example behind the feature flag.build.rs (3)
128-128: LGTM!Correctly extracts architecture information needed for macOS wheel selection.
131-133: Good practice to centralize library storage.Creating a dedicated
libs/directory improves organization.
331-331: Correct library name for linking.This change correctly specifies
_lightgbmas the library name, which causes the linker to look forlib_lightgbm.dylib(the linker prependsliband appends the extension). The previous valuelib_lightgbmwould have incorrectly looked forliblib_lightgbm.dylib.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
build.rs (1)
src/model.rs (1)
drop(297-301)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
.github/workflows/release.yml (1)
115-118: Suggest removing redundant token specification.The
CARGO_REGISTRY_TOKENis specified both as a--tokenparameter and as an environment variable, which is redundant. The CLI flag alone is sufficient.Apply this diff:
- name: Publish to crates.io run: cargo publish --token ${{ secrets.CARGO_REGISTRY_TOKEN }} - env: - CARGO_REGISTRY_TOKEN: ${{ secrets.CARGO_REGISTRY_TOKEN }}build.rs (1)
219-260: Suggest adding wheel cleanup.The downloaded wheel file at line 226 is never cleaned up, leaving build artifacts on disk. While past reviews mentioned this was addressed, the cleanup is not present in the current code.
Apply this diff:
println!( "cargo:warning=Extracted LightGBM library to: {}", lib_path.display() ); + // Clean up the downloaded wheel file + let _ = fs::remove_file(&wheel_path); return Ok(()); } } + // Clean up even on error + let _ = fs::remove_file(&wheel_path); Err(format!("{} not found in wheel", lib_filename).into())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/ci.yml(1 hunks).github/workflows/release.yml(1 hunks)build.rs(7 hunks)examples/advanced_usage.rs(3 hunks)examples/basic_usage.rs(2 hunks)src/error.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/basic_usage.rs
🧰 Additional context used
🧬 Code graph analysis (1)
build.rs (1)
src/model.rs (1)
drop(297-301)
🪛 actionlint (1.7.8)
.github/workflows/release.yml
28-28: label "ubuntu-24.04-arm64" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
97-97: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (7)
- GitHub Check: Test linux (x86_64) - LightGBM 4.6.0
- GitHub Check: Test linux (arm64) - LightGBM 4.6.0
- GitHub Check: Test Windows ARM64 (unsupported)
- GitHub Check: Test windows (x86_64) - LightGBM 4.6.0
- GitHub Check: Test linux (x86_64) - LightGBM 4.5.0
- GitHub Check: Test macos (arm64) - LightGBM 4.6.0
- GitHub Check: Test macos (arm64) - LightGBM 4.5.0
🔇 Additional comments (15)
examples/advanced_usage.rs (1)
1-42: LGTM! Formatting improvements.The changes are purely cosmetic—import reordering and formatting adjustments. No functional changes detected.
src/error.rs (1)
1-1: LGTM! Import consolidation.Moving the
crate::sysimport to the top improves readability and follows Rust conventions..github/workflows/ci.yml (7)
64-86: LGTM! Well-designed caching strategy.The multi-level caching (registry, index, build) with architecture-specific keys will significantly speed up CI runs while avoiding cross-platform cache conflicts.
88-97: LGTM! Correct system dependencies.The platform-specific dependencies (libclang-dev for Linux bindgen, libomp for macOS) are correctly specified.
99-128: LGTM! Comprehensive test coverage.Testing both with and without polars-support ensures feature compatibility. The separation of check, build, and test steps provides clear failure points.
137-155: LGTM! Architecture verification adds confidence.The platform-specific library verification (lipo on macOS, readelf on Linux) ensures the wheel extraction produces the correct architecture binaries.
157-186: LGTM! Solid linting and formatting checks.Running clippy with
-D warningsand testing both feature configurations ensures code quality. The separate fmt job maintains formatting consistency.
188-203: LGTM! Minimum version validation.Testing with LightGBM 4.0.0 ensures backward compatibility. Limiting to check/build (without full tests) is reasonable for minimum version validation.
205-225: LGTM! Defensive testing for unsupported platforms.Testing that Windows ARM64 fails gracefully with a clear error message is excellent defensive practice. The
continue-on-errorand outcome verification pattern is correctly implemented..github/workflows/release.yml (2)
46-58: LGTM! Comprehensive pre-release validation.Testing in release mode with both feature configurations before publishing ensures quality. Building all examples validates the release artifacts.
60-80: LGTM! Platform-appropriate artifact archiving.Using tar.gz for Unix and zip for Windows follows platform conventions. Error silencing on library copy handles cases where artifacts may not be present.
build.rs (4)
12-38: LGTM! Clear platform detection logic.The platform detection correctly handles common targets and provides clear error messages for unsupported configurations.
40-133: LGTM! Robust header download with graceful fallbacks.The header download correctly handles version-specific files (arrow.h, arrow.tpp) that don't exist in older LightGBM versions, ensuring compatibility across versions.
135-217: LGTM! Architecture-aware wheel extraction resolves ARM support.The wheel-based approach correctly handles architecture-specific binaries for macOS and Linux, with appropriate error messages for unsupported platforms (Windows i686/ARM64). This directly addresses the PR objective to support ARM architectures.
315-373: LGTM! Comprehensive library setup and rpath configuration.The library copy, search path, and platform-specific rpath configuration ensures the LightGBM library is accessible at both build and runtime.
| include: | ||
| - os: macos | ||
| arch: arm64 | ||
| runner: macos-14 | ||
| - os: macos | ||
| arch: x86_64 | ||
| runner: macos-13 | ||
| - os: linux | ||
| arch: x86_64 | ||
| runner: ubuntu-latest | ||
| - os: linux | ||
| arch: arm64 | ||
| runner: ubuntu-24.04-arm64 | ||
| - os: windows | ||
| arch: x86_64 | ||
| runner: windows-latest |
There was a problem hiding this comment.
Critical: Invalid runner label will cause release failure.
Static analysis indicates ubuntu-24.04-arm64 is an unknown label. Line 28 uses the incorrect runner label that will cause the Linux ARM64 release build to fail.
Apply this diff:
- os: linux
arch: arm64
- runner: ubuntu-24.04-arm64
+ runner: ubuntu-24.04-arm
lightgbm_version: "4.6.0"📝 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.
| include: | |
| - os: macos | |
| arch: arm64 | |
| runner: macos-14 | |
| - os: macos | |
| arch: x86_64 | |
| runner: macos-13 | |
| - os: linux | |
| arch: x86_64 | |
| runner: ubuntu-latest | |
| - os: linux | |
| arch: arm64 | |
| runner: ubuntu-24.04-arm64 | |
| - os: windows | |
| arch: x86_64 | |
| runner: windows-latest | |
| include: | |
| - os: macos | |
| arch: arm64 | |
| runner: macos-14 | |
| - os: macos | |
| arch: x86_64 | |
| runner: macos-13 | |
| - os: linux | |
| arch: x86_64 | |
| runner: ubuntu-latest | |
| - os: linux | |
| arch: arm64 | |
| runner: ubuntu-24.04-arm | |
| - os: windows | |
| arch: x86_64 | |
| runner: windows-latest |
🧰 Tools
🪛 actionlint (1.7.8)
28-28: label "ubuntu-24.04-arm64" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🤖 Prompt for AI Agents
In .github/workflows/release.yml around lines 16 to 31, the runner label
"ubuntu-24.04-arm64" on line 28 is invalid and will fail CI; change that runner
value to a valid GitHub-hosted runner label such as "ubuntu-24.04" (the job
already specifies arch: arm64 so the plain ubuntu-24.04 runner is correct).
Update the runner entry accordingly and keep the rest of the include matrix
unchanged.
| - name: Upload artifacts to release | ||
| uses: softprops/action-gh-release@v1 | ||
| with: | ||
| files: artifacts/**/* | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
Update outdated GitHub Action.
Static analysis indicates softprops/action-gh-release@v1 runner is too old. The v1 version may have compatibility issues with newer GitHub Actions runners.
Apply this diff:
- name: Upload artifacts to release
- uses: softprops/action-gh-release@v1
+ uses: softprops/action-gh-release@v2
with:
files: artifacts/**/*
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}📝 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.
| - name: Upload artifacts to release | |
| uses: softprops/action-gh-release@v1 | |
| with: | |
| files: artifacts/**/* | |
| env: | |
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| - name: Upload artifacts to release | |
| uses: softprops/action-gh-release@v2 | |
| with: | |
| files: artifacts/**/* | |
| env: | |
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
🧰 Tools
🪛 actionlint (1.7.8)
97-97: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/release.yml around lines 96 to 101: the workflow uses the
outdated softprops/action-gh-release@v1; update the action to a maintained major
release by replacing the uses reference with softprops/action-gh-release@v2 (or
the latest stable major tag), keep the files input and GITHUB_TOKEN env as-is,
and run the workflow to verify compatibility with the newer action.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
build.rs (2)
282-282: Critical: C++ standard must be C++17, not C++14.A previous review confirmed that LightGBM 4.0.0 requires C++17 and requested this change, which was marked as addressed. However, the code still uses
-std=c++14. The minimum supported LightGBM version (4.0.0, tested in your CI) requires C++17, so this will cause compilation failures or undefined behavior.Apply this diff:
- .clang_arg("-std=c++14") + .clang_arg("-std=c++17")
374-374: Critical: Windows MSVC builds will fail - import library mismatch.This issue was previously flagged and marked as addressed, but the code remains unchanged. When linking
dylib=_lightgbmon Windows MSVC, the linker expects an import library named_lightgbm.lib. However, your build only extractslib_lightgbm.dllfrom the wheel—no import library is created or extracted.Why this fails:
- MSVC's
link.exerequires.libimport libraries for DLL linkingcargo:rustc-link-lib=dylib=_lightgbm→ linker searches for_lightgbm.lib- Only
lib_lightgbm.dllexists inlibs/- Result: "cannot find _lightgbm.lib" link error
Solutions:
- Extract the
.libfile from the wheel if it contains one (check wheel contents)- Generate an import library using
gendef/dlltoolorllvm-dlltool- Change the link directive to match extracted filenames:
dylib=lib_lightgbmOption 3 (simplest fix) - change the library name to match the extracted file:
- println!("cargo:rustc-link-lib=dylib=_lightgbm"); + println!("cargo:rustc-link-lib=dylib=lib_lightgbm");Note: Verify this works with your Windows CI job before merging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yml(1 hunks)build.rs(9 hunks)src/sys.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/sys.rs
🧰 Additional context used
🧬 Code graph analysis (1)
build.rs (1)
src/model.rs (1)
drop(297-301)
⏰ 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: Test Windows ARM64 (unsupported)
| - os: linux | ||
| arch: arm64 | ||
| runner: ubuntu-latest | ||
| lightgbm_version: "4.6.0" |
There was a problem hiding this comment.
Critical: Linux ARM64 testing will not work with ubuntu-latest runner.
GitHub's ubuntu-latest runners are x86_64 only. This matrix entry will build for x86_64 architecture despite specifying arch: arm64, providing false confidence that ARM64 Linux builds work.
Options to fix:
- Remove the Linux ARM64 entry if ARM64 support isn't ready for CI testing
- Set up self-hosted ARM64 runners and reference them here
- Add QEMU emulation setup steps before the build
- Use a cross-compilation approach with explicit target triple
If you don't have ARM64 runners, apply this diff to remove the misleading entry:
- - os: linux
- arch: arm64
- runner: ubuntu-latest
- lightgbm_version: "4.6.0"
-📝 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.
| - os: linux | |
| arch: arm64 | |
| runner: ubuntu-latest | |
| lightgbm_version: "4.6.0" |
🤖 Prompt for AI Agents
In .github/workflows/ci.yml around lines 47 to 50 the matrix entry declares os:
linux and arch: arm64 but uses the ubuntu-latest runner (x86_64), which gives a
false ARM64 test; either remove this misleading Linux ARM64 matrix entry if you
do not have ARM64 runners, or replace it by referencing available ARM64 capacity
(point to a self-hosted ARM64 runner), or add QEMU emulation setup steps before
the build, or switch to a cross-compilation flow with an explicit ARM64
target—implement one of those fixes so the matrix no longer claims Linux ARM64
tests that actually run on x86_64.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
.github/workflows/release.yml (3)
84-93: Update to latest version of action-gh-release.The
softprops/action-gh-release@v1action is outdated and may have compatibility issues. This issue was previously flagged in code reviews.Apply this diff:
- name: Create GitHub Release - uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v2 with: tag_name: ${{ steps.version.outputs.tag }}
104-119: Fix invalid runner label for Linux ARM64.The runner label
ubuntu-24.04-arm64on line 116 is invalid and will cause the job to fail. This issue was previously flagged in code reviews.Apply this diff:
- os: linux arch: arm64 - runner: ubuntu-24.04-arm64 + runner: ubuntu-24.04-arm
181-187: Update to latest version of action-gh-release.The
softprops/action-gh-release@v1action is outdated and may have compatibility issues. This issue was previously flagged in code reviews.Apply this diff:
- name: Upload artifacts to release - uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v2 with:
🧹 Nitpick comments (1)
Cargo.toml (1)
12-13: Empty dependencies section serves no purpose.The empty
[dependencies]section can be removed unless you plan to add dependencies soon.Apply this diff if not needed:
-[dependencies] - [build-dependencies]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/release.yml(1 hunks)Cargo.toml(1 hunks)src/model.rs(5 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/release.yml
85-85: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
116-116: label "ubuntu-24.04-arm64" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
182-182: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (3)
- GitHub Check: Test windows (x86_64) - LightGBM 4.6.0
- GitHub Check: Test linux (x86_64) - LightGBM 4.6.0
- GitHub Check: Test Windows ARM64 (unsupported)
🔇 Additional comments (7)
Cargo.toml (1)
17-17: Addition of zip dependency is appropriate for wheel extraction.The
zipcrate is correctly added to[build-dependencies]for extracting LightGBM libraries from Python wheels.Note: The AI summary incorrectly states that
zipwas "moved from[build-dependencies]to[dependencies]", but the actual code showszipin[build-dependencies](line 17) with an empty[dependencies]section (lines 12-13).src/model.rs (5)
52-57: LGTM: Improved error construction formatting.The inline error construction is more concise while maintaining the same functionality.
86-88: LGTM: Consistent error construction formatting.The formatting is consistent with the changes in
Booster::load.
118-120: LGTM: Consistent formatting throughout.The error construction formatting is consistent across all load methods.
172-176: LGTM: Improved error message readability.The multi-line parameter formatting improves readability without changing functionality.
245-249: LGTM: Consistent error formatting.The error message formatting is consistent with the
predictmethod..github/workflows/release.yml (1)
189-206: LGTM: Publish job configured correctly.The crates.io publishing logic correctly:
- Depends on successful pre-release tests
- Conditionally checks out the appropriate ref
- Uses the CARGO_REGISTRY_TOKEN secret securely
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
build.rs (2)
282-282: Critical: Update C++ standard to C++17.LightGBM 4.0.0 and later require C++17, not C++14. Using C++14 may cause compilation failures when generating bindings. This was flagged in a previous review but the fix was not applied.
Apply this diff:
- .clang_arg("-std=c++14") + .clang_arg("-std=c++17")
219-260: Clean up downloaded wheel file after extraction.The wheel file remains in the build directory after extraction, leaving build artifacts on disk. This can accumulate over time and consume unnecessary space.
Apply this diff to clean up the wheel file:
println!( "cargo:warning=Extracted LightGBM library to: {}", lib_path.display() ); + // Clean up the downloaded wheel file + let _ = fs::remove_file(&wheel_path); return Ok(()); } } + // Clean up even on error + let _ = fs::remove_file(&wheel_path); Err(format!("{} not found in wheel", lib_filename).into())Optional improvement: Consider enhancing the error message to list files found in the wheel when the expected library is missing, which would aid debugging.
🧹 Nitpick comments (1)
build.rs (1)
331-331: Optional: Enhance error message context.The
expectmessage could include the source and destination paths to aid debugging if the copy fails.- fs::copy(&lib_source_path, &lib_dest_path).expect("Failed to copy library to target directory"); + fs::copy(&lib_source_path, &lib_dest_path) + .unwrap_or_else(|e| panic!("Failed to copy library from {} to {}: {}", + lib_source_path.display(), lib_dest_path.display(), e));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build.rs(9 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: Test Windows ARM64 (unsupported)
🔇 Additional comments (6)
build.rs (6)
12-38: LGTM!The platform detection logic correctly identifies OS and architecture combinations, with clear panic messages for unsupported targets.
40-133: LGTM!The header download logic correctly handles required and optional header files with appropriate error handling and user-facing warnings.
146-160: LGTM!The macOS wheel URL construction correctly uses platform-specific deployment targets (
macosx_10_15_x86_64for Intel,macosx_12_0_arm64for Apple Silicon), addressing the previous review feedback.
163-181: LGTM!The Linux wheel extraction logic correctly handles both x86_64 and aarch64 architectures with appropriate manylinux tags.
199-214: Good error handling for unsupported platforms.The explicit error messages for Windows i686, Windows ARM64, and other unsupported platform/architecture combinations are clear and guide users appropriately.
340-372: LGTM!The rpath configuration for macOS and Linux is comprehensive and includes multiple paths for IDE compatibility and runtime library resolution.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
build.rs (1)
348-348: Critical: Change C++14 to C++17 for LightGBM 4.0.0+ compatibility.LightGBM 4.0.0 and later require C++17. The current setting of
"-std=c++14"may cause compilation failures or undefined behavior with the minimum supported version (4.0.0).Apply this diff:
.clang_arg(format!("-I{}", lgbm_include_root.display())) .clang_arg("-xc++") - .clang_arg("-std=c++14") + .clang_arg("-std=c++17")Based on learnings
🧹 Nitpick comments (2)
build.rs (2)
219-260: Consider cleaning up downloaded wheel files.The wheel extraction works correctly, but the downloaded
.whlfile atout_dir/lightgbm.whlis never removed, leaving build artifacts on disk. Consider adding cleanup after successful extraction.Apply this diff to add cleanup:
println!( "cargo:warning=Extracted LightGBM library to: {}", lib_path.display() ); + let _ = fs::remove_file(&wheel_path); return Ok(()); } } + let _ = fs::remove_file(&wheel_path); Err(format!("{} not found in wheel", lib_filename).into())
262-326: LGTM: Correct Windows library extraction.The function properly extracts both
lib_lightgbm.dllandlib_lightgbm.lib(import library), addressing the Windows MSVC linking requirements. Error handling ensures both files are present before succeeding.Optional: Consider adding the same wheel cleanup as suggested for
download_and_extract_from_wheel.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build.rs(9 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). (3)
- GitHub Check: Test windows (x86_64) - LightGBM 4.6.0
- GitHub Check: Test Windows ARM64 (unsupported)
- GitHub Check: Test macos (arm64) - LightGBM 4.6.0
🔇 Additional comments (4)
build.rs (4)
57-59: LGTM: Clean HTTP status validation.The use of range contains pattern
(200..300).contains(&status)is idiomatic and clearly expresses the intent to check for successful HTTP responses.Also applies to: 75-77, 231-233
144-217: LGTM: Comprehensive platform-specific library download logic.The wheel-based download approach correctly handles:
- macOS with architecture-specific deployment targets (12_0 for ARM64, 10_15 for x86_64)
- Linux for both x86_64 and aarch64
- Windows x86_64 with explicit errors for unsupported i686 and ARM64
The error messages are clear and guide users toward alternatives.
397-406: LGTM: Appropriate Windows import library handling.The code correctly distinguishes between runtime (DLL copied to target) and link-time (import library remains in libs directory) requirements. The warning message provides useful feedback.
417-436: LGTM: Comprehensive macOS rpath configuration.The multiple rpath entries correctly handle various execution contexts (IDE, command line, different build profiles), ensuring the dynamic library can be found at runtime.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
.github/workflows/release.yml (3)
85-85: Update outdated GitHub Action.This issue was previously flagged. The
softprops/action-gh-release@v1action is too old and may have compatibility issues with newer GitHub Actions runners.Apply this diff:
- name: Create GitHub Release - uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v2 with: tag_name: ${{ steps.version.outputs.tag }}
116-116: Critical: Invalid runner label will cause release failure.This issue was previously flagged. The runner label
ubuntu-24.04-arm64is invalid and will cause the Linux ARM64 release build to fail.Apply this diff:
- os: linux arch: arm64 - runner: ubuntu-24.04-arm64 + runner: ubuntu-24.04-arm
182-182: Update outdated GitHub Action.This issue was previously flagged. The
softprops/action-gh-release@v1action is too old.Apply this diff:
- name: Upload artifacts to release - uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v2 with:build.rs (1)
348-348: Update C++ standard to C++17 for LightGBM compatibility.This issue was previously flagged. LightGBM 4.0.0+ requires C++17, not C++14. The current
-std=c++14flag may cause compilation issues with LightGBM headers that use C++17 features.Apply this diff:
.clang_arg(format!("-I{}", lgbm_include_root.display())) .clang_arg("-xc++") - .clang_arg("-std=c++14") + .clang_arg("-std=c++17") // Only generate bindings for functions starting with LGBM_
🧹 Nitpick comments (2)
build.rs (2)
219-260: Clean up downloaded wheel file after extraction.The wheel file downloaded to
out_dir/lightgbm.whlis never removed, leaving build artifacts on disk. This was mentioned in a previous review but not addressed.Apply this diff:
println!( "cargo:warning=Extracted LightGBM library to: {}", lib_path.display() ); + // Clean up the downloaded wheel file + let _ = fs::remove_file(&wheel_path); return Ok(()); } } + // Clean up even on error + let _ = fs::remove_file(&wheel_path); Err(format!("{} not found in wheel", lib_filename).into())
262-326: Clean up downloaded wheel file after extraction.Similar to the previous function, the Windows-specific extraction also leaves the wheel file on disk after downloading.
Apply this diff:
if dll_found && lib_found { + // Clean up the downloaded wheel file + let _ = fs::remove_file(&wheel_path); return Ok(()); } } if !dll_found { + let _ = fs::remove_file(&wheel_path); return Err("lib_lightgbm.dll not found in wheel".into()); } if !lib_found { + let _ = fs::remove_file(&wheel_path); return Err("lib_lightgbm.lib not found in wheel".into()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/release.yml(1 hunks)build.rs(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
build.rs (1)
src/model.rs (1)
drop(299-303)
🪛 actionlint (1.7.8)
.github/workflows/release.yml
85-85: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
116-116: label "ubuntu-24.04-arm64" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
182-182: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (2)
- GitHub Check: Test Windows ARM64 (unsupported)
- GitHub Check: Test windows (x86_64) - LightGBM 4.6.0
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
build.rs (1)
348-348: Critical: Update C++ standard from C++14 to C++17.A previous review confirmed that LightGBM 4.0.0+ requires C++17, and this was marked as addressed in commit 0184876. However, the code still uses
-std=c++14. This may cause compilation failures with LightGBM 4.0.0+ headers that rely on C++17 features.Apply this fix:
- .clang_arg("-std=c++14") + .clang_arg("-std=c++17")
🧹 Nitpick comments (2)
build.rs (2)
219-260: Consider cleaning up downloaded wheel files after extraction.The wheel files downloaded to
out_dir.join("lightgbm.whl")are not removed after successful extraction. While this doesn't affect functionality, it leaves build artifacts on disk.Apply this pattern to clean up the wheel file:
println!( "cargo:warning=Extracted LightGBM library to: {}", lib_path.display() ); + // Clean up the downloaded wheel file + let _ = fs::remove_file(&wheel_path); return Ok(()); } } + // Clean up even on error + let _ = fs::remove_file(&wheel_path); Err(format!("{} not found in wheel", lib_filename).into())Apply the same pattern to
download_and_extract_windows_libsafter line 314 and before line 318.Also applies to: 262-326
456-457: Consider removing unreachable catch-all case.The empty catch-all at lines 456-457 is unreachable because
get_platform_info()already panics for unsupported platforms (line 23), ensuringoscan only be"darwin","linux", or"windows". While harmless, removing it makes the code clearer.Apply this diff:
"windows" => { // On Windows, we need to tell the linker where to find the DLL at runtime // Copy the DLL to the output directory (already done above) println!("cargo:rustc-link-lib=dylib=lib_lightgbm"); } - _ => {} } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build.rs(9 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: Test windows (x86_64) - LightGBM 4.6.0
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
build.rs (4)
219-260: Consider cleaning up the downloaded wheel file.The wheel file (
lightgbm.whl) remains inOUT_DIRafter extraction. While Cargo eventually cleansOUT_DIR, immediate cleanup would reduce disk usage and make the build process tidier.You can add cleanup with:
println!( "cargo:warning=Extracted LightGBM library to: {}", lib_path.display() ); + let _ = fs::remove_file(&wheel_path); return Ok(()); } } + let _ = fs::remove_file(&wheel_path); Err(format!("{} not found in wheel", lib_filename).into())
262-326: Consider cleaning up the downloaded wheel file.Similar to
download_and_extract_from_wheel, the wheel file is not removed after extraction. Adding cleanup would be consistent with good build hygiene.Apply this diff:
if dll_found && lib_found { + let _ = fs::remove_file(&wheel_path); return Ok(()); } } + let _ = fs::remove_file(&wheel_path); if !dll_found { return Err("lib_lightgbm.dll not found in wheel".into()); }
399-409: Optional: The import library existence check is redundant.Since
download_and_extract_windows_libsalready ensures both the DLL and import library are present (or fails with an error), this check will always find the file. The warning message is still useful for debugging, but theif import_lib_source.exists()condition will always be true at this point.You could simplify to:
// On Windows, also copy the import library (.lib) to the libs directory for linking if os == "windows" { let import_lib_source = out_dir.join("libs").join("lib_lightgbm.lib"); - if import_lib_source.exists() { - // No need to copy the .lib to target dir, it's only used during linking - println!( - "cargo:warning=Found import library at: {}", - import_lib_source.display() - ); - } + // No need to copy the .lib to target dir, it's only used during linking + println!( + "cargo:warning=Found import library at: {}", + import_lib_source.display() + ); }
456-457: Optional: Remove or document the unreachable catch-all.Since
get_platform_info()only returns"darwin","linux", or"windows"(or panics for unsupported platforms), this catch-all branch is unreachable dead code.You can either remove it entirely:
"windows" => { // On Windows, we need to tell the linker where to find the DLL at runtime // Copy the DLL to the output directory (already done above) println!("cargo:rustc-link-lib=dylib=lib_lightgbm"); } - _ => {} }Or replace with an explicit panic for defensive programming:
"windows" => { // On Windows, we need to tell the linker where to find the DLL at runtime // Copy the DLL to the output directory (already done above) println!("cargo:rustc-link-lib=dylib=lib_lightgbm"); } - _ => {} + _ => panic!("Unsupported OS: {}", os), }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build.rs(9 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). (2)
- GitHub Check: Test Windows ARM64 (unsupported)
- GitHub Check: Test windows (x86_64) - LightGBM 4.6.0
🔇 Additional comments (4)
build.rs (4)
57-57: LGTM: Cleaner HTTP status range checks.The refactor to use
.contains(&status)is more idiomatic and readable than explicit comparisons.Also applies to: 75-75, 231-231
135-217: LGTM: Wheel-based download logic is well-structured.The platform and architecture detection correctly handles supported targets, with clear error messages for unsupported combinations. The use of different macOS deployment targets per architecture and appropriate Linux wheel platforms aligns with LightGBM's release distribution.
348-348: LGTM: C++ standard correctly set to C++17.This aligns with LightGBM 4.0.0+ requirements.
419-457: Platform-specific linking directives are correct.The rpath configuration and library link names are appropriate for each platform:
- macOS/Linux:
dylib=_lightgbmcorrectly resolves tolib_lightgbm.{dylib,so}via automaticlibprefix addition- Windows:
dylib=lib_lightgbmcorrectly points to the import library
1 participant
Summary by CodeRabbit
Compatibility
CI / Release
Chores
Style