-
Notifications
You must be signed in to change notification settings - Fork 50
fix: Bun Feature Failure on Apple Silicon ARM64 + Feature Hardening #149
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
base: main
Are you sure you want to change the base?
fix: Bun Feature Failure on Apple Silicon ARM64 + Feature Hardening #149
Conversation
|
Would appreciate a review + merge on this to clear the related issue |
|
Hi Trevor! Thanks for your PR, and sorry for the delay with the review, I'm being busy lately. I'm going to review it by the end of this week |
koralowiec
left a 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.
To be honest, I think that there's too much logging, and maybe too much logic in some places.
Generally, the solution which you provided looks nice, but I would like to get rid of the logging part. So please remove the log and debug_log functions, and their execution. If you feel something has to be output, use echo for now.
Like I mentioned in one of the comments, I'm going to think about adding the logging functions to the shared script (#165)
| # Helper function for consistent logging (robust for container environments) | ||
| log() { | ||
| local level="$1" | ||
| shift | ||
| local message="$*" | ||
| local timestamp | ||
|
|
||
| # Try to get timestamp, fallback if date command fails | ||
| if timestamp=$(date '+%Y-%m-%d %H:%M:%S' 2>/dev/null); then | ||
| timestamp="[$timestamp]" | ||
| else | ||
| timestamp="[$(date 2>/dev/null || echo 'unknown')]" | ||
| fi | ||
|
|
||
| case "$level" in | ||
| INFO) echo "$timestamp INFO: $message" ;; | ||
| SUCCESS) echo "$timestamp SUCCESS: $message" ;; | ||
| WARNING) echo "$timestamp WARNING: $message" ;; | ||
| ERROR) echo "$timestamp ERROR: $message" >&2 ;; | ||
| *) echo "$timestamp $level: $message" ;; | ||
| esac | ||
| } | ||
|
|
||
| # Debug logging helper (enabled when BUN_FEATURE_DEBUG is set) | ||
| debug_log() { | ||
| if [ -n "$BUN_FEATURE_DEBUG" ]; then | ||
| log INFO "$@" | ||
| fi | ||
| } | ||
|
|
||
| log INFO "Starting Bun installation" | ||
|
|
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.
Having a more robust logging functions is not a bad idea (along with having a debug option). But I think that this shouldn't be introduced on a single feature level. It probably belongs to library_scripts.sh which is a "shared" functions library. For now I'm not sure if I want to introduce the functions you provided in this exact shape into that library_scripts.sh, I think I need to go over it (#165)
So for now, I would like you to not define/use that functions
| detect_libc() { | ||
| # Method 1: Check for apk (Alpine) | ||
| if [ -x "/sbin/apk" ]; then | ||
| log INFO "Detected musl libc (Alpine package manager found)" >&2 | ||
| echo "-musl" | ||
| return 0 | ||
| fi | ||
|
|
||
| # Method 2: Check ldd output for musl | ||
| if command -v ldd >/dev/null 2>&1; then | ||
| if ldd --version 2>&1 | grep -qi musl; then | ||
| log INFO "Detected musl libc (ldd output)" >&2 | ||
| echo "-musl" | ||
| return 0 | ||
| fi | ||
| fi | ||
|
|
||
| # Method 3: Check for musl library files | ||
| if [ -f "/lib/ld-musl-x86_64.so.1" ] || [ -f "/lib/ld-musl-aarch64.so.1" ]; then | ||
| log INFO "Detected musl libc (library files)" >&2 | ||
| echo "-musl" | ||
| return 0 | ||
| fi | ||
|
|
||
| # Method 4: Check /proc/mounts for musl | ||
| if grep -qi musl /proc/mounts 2>/dev/null; then | ||
| log INFO "Detected musl libc (proc mounts)" >&2 | ||
| echo "-musl" | ||
| return 0 | ||
| fi | ||
|
|
||
| # Default to glibc | ||
| log INFO "Using glibc (default)" >&2 | ||
| echo "" | ||
| } |
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.
I think it also makes sense to keep it in the library script, but let's keep it here for now, as it's crucial for the installation
| # Check if bun binary was actually installed | ||
| if [ -f "/usr/local/bin/bun" ]; then | ||
| log INFO "Bun binary found at /usr/local/bin/bun" | ||
| log INFO "Bun binary permissions: $(ls -la /usr/local/bin/bun)" | ||
| else | ||
| log ERROR "Bun binary NOT found at /usr/local/bin/bun after installation" | ||
| log ERROR "Checking /usr/local/bin contents: $(ls -la /usr/local/bin/ 2>/dev/null || echo 'Directory not accessible')" | ||
|
|
||
| # Try to find where bun might have been installed | ||
| log INFO "Searching for bun binary in common locations..." | ||
| find /usr/local -name "*bun*" -type f 2>/dev/null || log INFO "No bun binaries found in /usr/local" | ||
| fi |
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.
I think it's redundant check - We actually verify the installation in the tests. I think we can omit it here
| # Verify the installation worked | ||
| log INFO "Verifying Bun installation..." | ||
| if [ -x "/usr/local/bin/bun" ]; then | ||
| log INFO "Bun binary is executable at /usr/local/bin/bun" | ||
| if bun_binary_version=$(/usr/local/bin/bun --version 2>&1); then | ||
| log INFO "Bun version check successful: $bun_binary_version" | ||
| else | ||
| log INFO "Bun binary exists but version check failed - this may be environment-specific" | ||
| log INFO "Binary info: $(file /usr/local/bin/bun 2>/dev/null || echo 'file command failed')" | ||
| log INFO "Library dependencies: $(ldd /usr/local/bin/bun 2>/dev/null || echo 'ldd command failed')" | ||
|
|
||
| # Try to run with explicit library path for debugging | ||
| if [ -d "/lib" ] && [ -d "/usr/lib" ]; then | ||
| log INFO "Attempting to run bun with library path for debugging..." | ||
| if bun_binary_version=$(LD_LIBRARY_PATH=/lib:/usr/lib:/usr/local/lib /usr/local/bin/bun --version 2>&1); then | ||
| log INFO "Bun version check successful with LD_LIBRARY_PATH: $bun_binary_version" | ||
| else | ||
| log INFO "Bun version check still failed with LD_LIBRARY_PATH" | ||
| fi | ||
| fi | ||
| fi | ||
| else | ||
| log ERROR "Bun binary not found or not executable at /usr/local/bin/bun" | ||
| log ERROR "Checking if bun exists: $([ -f "/usr/local/bin/bun" ] && echo 'YES' || echo 'NO')" | ||
| if [ -f "/usr/local/bin/bun" ]; then | ||
| log ERROR "Bun file exists but is not executable. Permissions: $(ls -la /usr/local/bin/bun)" | ||
| log ERROR "Attempting to make executable..." | ||
| chmod +x /usr/local/bin/bun | ||
| if [ -x "/usr/local/bin/bun" ]; then | ||
| log INFO "Fixed: Bun binary is now executable" | ||
| else | ||
| log ERROR "Failed to make Bun binary executable" | ||
| fi | ||
| else | ||
| log ERROR "Bun binary file does not exist at /usr/local/bin/bun" | ||
| fi | ||
| fi |
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.
Again, let's remove it
Problem
The Bun feature failed to install on Apple Silicon Mac hosts when creating
arm64Linux containers.Users encountered this error:
no matches found for asset regex: ^bun-linux-aarch64-baseline\.zip$Root Cause
The Bun feature was attempting to download
bun-linux-aarch64-baseline.zip, but Bun does not publish"baseline" variants for the ARM64 architecture. For linux /
arm64, Bun only publishes:bun-linux-aarch64.zip(glibc)bun-linux-aarch64-musl.zip(musl)This worked on linux /
amd64-based DevContainers because baseline variants exist there, but failed onarm64-based DevContainers.Solution
1.
arm64Asset Pattern Fixarm64now uses standard builds instead of non-existent baseline variants2. Comprehensive Feature Enhancements
Robust libc Detection
/sbin/apkcheck to 4 detection methods:ldd --versionoutput analysis/proc/mountsanalysis/proc/mountsmusl check to avoid broken grep pipelineEnhanced Architecture Support
aarch64andarm64aliasesBetter Error Handling & Diagnostics
Improved Logging & User Experience
[2024-01-15 10:30:15] INFO: ...INFO,SUCCESS,WARNING,ERRORBUN_FEATURE_DEBUG=1Performance Optimizations
Version Handling Enhancements
1.2.3,v1.2.3,bun-v1.2.3Benefits
For Apple Silicon Users
arm64asset selectionFor All Users
Files Changed
src/bun/install.shsrc/bun/NOTES.mdarm64supportTesting
References