nix: Add patchelf step to cross-builds #6651
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily involve modifications to GitHub Actions workflows and build configurations for Rust packages. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
hoprd/hoprd/build.rs (2)
6-11: Improve error handling patternThe current error handling pattern using
if let Err(_)is not idiomatic Rust. Consider usingis_err()or pattern matching withmatches!()for better readability.- if let Err(_) = std::env::var("VERGEN_GIT_SHA") { + if std::env::var("VERGEN_GIT_SHA").is_err() { let git = GixBuilder::default().sha(true).build()?; Emitter::default().add_instructions(&git)?.emit() } else { Ok(()) }
6-6: Add clarifying comment for environment variable checkConsider adding a comment explaining why we're checking for the pre-existence of
VERGEN_GIT_SHA. This would help other developers understand that this is intentionally allowing external build systems (like Nix) to provide the Git SHA.+ // Skip SHA generation if already provided by external build system (e.g., Nix) if std::env::var("VERGEN_GIT_SHA").is_err() {.github/workflows/build-binaries.yaml (1)
Line range hint
1-66: Well-structured workflow simplificationThe removal of the interpreter input and related steps, while maintaining the existing security controls (concurrency, timeouts) and moving patchelf to Nix, effectively streamlines the binary build process. This aligns perfectly with the PR objectives.
Consider documenting these changes in the repository's build documentation to help users understand that patchelf is now handled automatically during cross-compilation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/build-binaries.yaml(1 hunks).github/workflows/build.yaml(0 hunks)flake.nix(3 hunks)hoprd/hoprd/build.rs(1 hunks)nix/rust-package.nix(4 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/build.yaml
🔇 Additional comments (6)
.github/workflows/build-binaries.yaml (2)
59-59: Verify patchelf integration in Nix build
The step name change suggests multiple binaries, and the PR objective indicates that patchelf should be integrated into cross-builds. Let's verify this integration is properly handled in the Nix configuration.
✅ Verification successful
Patchelf is properly integrated into Nix cross-compilation
The verification confirms that patchelf is now correctly integrated:
- In
nix/rust-package.nix, patchelf is used in thepreFixupphase for cross-compilation builds to set the interpreter path - The integration is conditional (
lib.optionalString (isCross && targetInterpreter != "")) ensuring it only runs during cross-compilation - Patchelf is also available in the development shell (
nix/shell.nix) for local development needs
The step name change from "Build binary" to "Build ${{ inputs.binary }} binaries" accurately reflects that the build process now handles multiple binary variants through Nix's cross-compilation support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify patchelf integration in Nix build configuration
# Check for patchelf in flake.nix and other Nix configs
echo "Checking for patchelf in Nix configurations..."
rg -l 'patchelf' flake.nix nix/
Length of output: 169
Script:
#!/bin/bash
# Check the content of Nix files to understand patchelf integration
echo "=== Content of nix/rust-package.nix ==="
rg -A 5 -B 5 'patchelf' nix/rust-package.nix
echo -e "\n=== Content of nix/shell.nix ==="
rg -A 5 -B 5 'patchelf' nix/shell.nix
# Check if there are any cross-compilation related configurations
echo -e "\n=== Cross compilation configs ==="
rg 'cross' nix/rust-package.nix
Length of output: 885
62-66: LGTM! Simplified artifact upload with wildcard path
The change to use result/bin/* simplifies the upload step and captures all built binaries. This aligns well with the PR objective of streamlining the workflow.
✅ Verification successful
Let me verify the workflow configuration further to ensure the path is correctly set.
Let me check the build steps to understand how the binary output directory is created.
The wildcard path for binary artifacts is correct and aligns with Nix build output
The workflow is correctly configured. The nix build command creates a result symlink in the workspace, and Nix conventionally places binaries in the bin subdirectory. The wildcard path ${{ github.workspace }}/result/bin/* will correctly capture all binaries produced by the Nix build step. The change simplifies artifact upload while maintaining compatibility with Nix's output structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the binary output structure
# Check the typical binary output structure
echo "Checking Nix build output structure..."
fd -t f . result/bin/ || echo "No result/bin directory found (this is expected if build hasn't run)"
Length of output: 359
Script:
#!/bin/bash
# Check the workflow file for build output path configuration
echo "Checking build output path configuration..."
rg -A 5 "result/bin" .github/workflows/
# Check if there are other paths used for binary outputs
echo -e "\nChecking other binary output paths..."
rg -i "path:.*bin.*" .github/workflows/
Length of output: 451
Script:
#!/bin/bash
# Check the build steps in the workflow
echo "Checking build configuration..."
cat .github/workflows/build-binaries.yaml
# Check for any Nix build configurations
echo -e "\nChecking Nix build configurations..."
rg -B 3 -A 3 "nix build" .github/workflows/
Length of output: 3797
flake.nix (4)
122-122: LGTM! Build arguments updated correctly
The revision info is properly integrated into the build arguments, enabling version tracking in the build process.
151-151: LGTM! Build arguments consistently updated
The revision info is consistently integrated across both hoprd and hopli builds, maintaining build system coherence.
39-39: LGTM! Verify Git revision info capture
The implementation correctly handles both clean and dirty Git states, ensuring robust version tracking for builds.
✅ Verification successful
Git revision info capture verified successfully
The implementation correctly handles Git revision info with toString (self.shortRev or self.dirtyShortRev). The verification confirms:
- Clean Git state with short revision
6d54e9ce - No uncommitted changes (empty
git status --porcelainoutput)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that Git revision info is properly captured
# Test: Check if both shortRev and dirtyShortRev are accessible
# Get the current Git revision info
git rev-parse --short HEAD
# Check if there are uncommitted changes
git status --porcelain
Length of output: 62
Line range hint 39-151: Verify cross-build functionality with version tracking
While the version tracking changes are well-implemented, let's verify the cross-build functionality and patchelf integration.
✅ Verification successful
Cross-compilation and patchelf integration verified successfully
The implementation correctly handles:
- Cross-compilation setup with proper platform detection and toolchain configuration
- patchelf integration for Linux targets with appropriate interpreter paths:
- x86_64:
/lib64/ld-linux-x86-64.so.2 - aarch64:
/lib64/ld-linux-aarch64.so.1 - armv7:
/lib64/ld-linux-armhf.so.3
- x86_64:
- Version tracking through
VERGEN_GIT_SHAenvironment variable - Platform-specific build inputs and hooks for Darwin/non-Darwin targets
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify cross-build configuration and patchelf integration
# Test 1: Check if rust-package.nix exists and contains patchelf references
# Test 2: Verify cross-compilation targets are properly configured
# Test 1: Check rust-package.nix for patchelf integration
echo "Checking for patchelf integration..."
rg -A 5 "patchelf" ./nix/rust-package.nix
# Test 2: Verify cross-compilation targets
echo "Verifying cross-compilation targets..."
# Check if the builders are properly configured
for target in x86_64-linux aarch64-linux armv7l-linux x86_64-darwin aarch64-darwin; do
echo "Checking $target configuration..."
rg "crossSystem.*$target" ./nix/rust-builder.nix
done
Length of output: 1481
Script:
#!/bin/bash
# Let's verify the cross-compilation setup more thoroughly
# Check rust-builder.nix content to understand cross-compilation setup
echo "=== Checking rust-builder.nix content ==="
cat ./nix/rust-builder.nix
# Check rust-package.nix content to understand build process
echo -e "\n=== Checking rust-package.nix content ==="
cat ./nix/rust-package.nix
# Check if the cross-system configurations are properly referenced
echo -e "\n=== Checking cross-system references ==="
rg "crossSystem" ./nix/rust-builder.nix -A 5 -B 5
Length of output: 9437
This ensures users who build cross-platform binaries don't have to perform the
patchelfstep separately. Also the Github actions are simplified that way.