Fix patchelf concurrency issues. Show subprocess output in errors (#1410)

* Show subprocess output in error message when run_with_io fails

* Fix concurrency issues in check_dynamic_linkage

Remove nested Threads.@threads over libraries since the outer loop over
files already provides parallelism. Nested threading caused:
1. Race conditions with patchelf/install_name_tool on the same file
2. ConcurrencyViolationError in OutputCollectors when multiple sandbox
   operations ran in parallel

* Revert "Fix concurrency issues in check_dynamic_linkage"

This reverts commit 04d4f81.

* rm OutputCollectors (unused)

* Add Per-file locks for patchelf operations

* update to 1.12

* go back to unbuffer julia-1.7

* Extend patchelf lock to cover file reads

Wrap the entire read-modify-verify cycle in with_patchelf_lock() for
Linux/BSD ELF operations. This prevents concurrent ObjectFile reads
from failing with EOFError when another thread is running patchelf
on the same file.

Split update_linkage into _update_linkage_macho and _update_linkage_elf,
and ensure_soname into _ensure_soname_macho and _ensure_soname_elf for
cleaner platform separation.

* Fix platform detection: check macOS before BSD

Sys.isbsd(platform) returns true for macOS since Darwin is BSD-based,
but macOS uses install_name_tool not patchelf. Check Sys.isapple first.

* Reduce needless code duplication

* Reduce needless diff

---------

Co-authored-by: Mosè Giordano <mose@gnu.org>

Author: IanButterworth

.github/workflows/documentation.yml

@@ -20,7 +20,7 @@ jobs:
       - uses: actions/checkout@v6
       - uses: julia-actions/setup-julia@latest
         with:
-          version: "1.7"
+          version: "1.12"
       - uses: julia-actions/cache@v2
         with:
           cache-registries: "true"

Project.toml

@@ -18,7 +18,6 @@ Libdl = "8f399da3-3557-5675-b5ff-fb832c97cbdb"
 Logging = "56ddb016-857b-54e1-b83d-db4d58db5568"
 LoggingExtras = "e6f89c97-d47a-5376-807f-9c37f3926c36"
 ObjectFile = "d8793406-e978-5875-9003-1fc021f44a92"
-OutputCollectors = "6c11c7d4-943b-4e2b-80de-f2cfc2930a8c"
 Patchelf_jll = "f2cf89d6-2bfd-5c44-bd2c-068eea195c0c"
 Pkg = "44cfe95a-1eb2-52ea-b672-e2afdf69b78f"
 PkgLicenses = "fc669557-7ec9-5e45-bca9-462afbc28879"
@@ -45,7 +44,6 @@ JLLWrappers = "1.2.0"
 JSON = "0.21, 1"
 LoggingExtras = "0.4, 1"
 ObjectFile = "0.4.3"
-OutputCollectors = "0.1"
 Patchelf_jll = "0.14.3"
 PkgLicenses = "0.2"
 Registrator = "1.1"

docs/src/index.md

@@ -4,10 +4,6 @@ The purpose of the [`BinaryBuilder.jl`](https://github.com/JuliaPackaging/Binary
 
 Note that at this time, BinaryBuilder itself runs on Linux `x86_64` and macOS `x86_64` systems only, with Windows support under active development.  On macOS and Windows, you must have `docker` installed as the backing virtualization engine.  Note that Docker Desktop is the recommended version; if you have Docker Machine installed it may not work correctly or may need additional configuration.
 
-!!! warn
-
-    This package currently requires Julia v1.7.  Contribute to [JuliaPackaging/JLLPrefixes.jl#6](https://github.com/JuliaPackaging/JLLPrefixes.jl/issues/6) if you care about supporting newer versions of Julia.
-
 ## Project flow
 
 Suppose that you have a Julia package `Foo.jl` which wants to use a compiled `libfoo` shared library.  As your first step in writing `Foo.jl`, you may compile `libfoo` locally on your own machine with your system compiler, then using `Libdl.dlopen()` to open the library, and `ccall()` to call into the exported functions.  Once you have written your C bindings in Julia, you will naturally desire to share the fruits of your labor with the rest of the world, and this is where `BinaryBuilder` can help you.  Not only will `BinaryBuilder` aid you in constructing compiled versions of all your dependencies, but it will also build a wrapper Julia package (referred to as a [JLL package](jll.md)) to aid in installation, versioning, and build product localization.

src/Auditor.jl

@@ -16,11 +16,46 @@ const AUDITOR_SANDBOX_LOCK = ReentrantLock()
 # Logging is reportedly not thread-safe but guarding it with locks should help.
 const AUDITOR_LOGGING_LOCK = ReentrantLock()
 
+# Per-file locks for patchelf operations. Multiple threads may try to modify
+# the same binary file (e.g., relinking different libraries), so we need to
+# serialize access per file.
+const PATCHELF_FILE_LOCKS = Dict{String,ReentrantLock}()
+const PATCHELF_FILE_LOCKS_LOCK = ReentrantLock()
+
+"""
+    with_patchelf_lock(f, path::AbstractString)
+
+Execute `f()` while holding a lock specific to the file at `path`.
+This prevents concurrent patchelf operations on the same file.
+"""
+function with_patchelf_lock(f, path::AbstractString)
+    # Normalize the path to ensure consistent locking
+    path = realpath(path)
+    
+    # Get or create the lock for this file
+    file_lock = Base.@lock PATCHELF_FILE_LOCKS_LOCK begin
+        get!(PATCHELF_FILE_LOCKS, path) do
+            ReentrantLock()
+        end
+    end
+    
+    # Execute with the file-specific lock held
+    Base.@lock file_lock f()
+end
+
 # Helper function to run a command and print to `io` its invocation and full
-# output (mimim what the sandbox does normally, but outside of it).
+# output (mimic what the sandbox does normally, but outside of it).
 function run_with_io(io::IO, cmd::Cmd; wait::Bool=true)
     println(io, "---> $(join(cmd.exec, " "))")
-    run(pipeline(cmd; stdout=io, stderr=io); wait)
+    output = IOBuffer()
+    proc = run(pipeline(cmd; stdout=output, stderr=output); wait=false)
+    Base.wait(proc)
+    out_str = String(take!(output))
+    print(io, out_str)
+    if wait && !success(proc)
+        error("Command failed: $(cmd)\nOutput:\n$(out_str)")
+    end
+    return proc
 end
 
 include("auditor/instruction_set.jl")

src/BinaryBuilder.jl

@@ -34,7 +34,7 @@ const runshell = BinaryBuilderBase.runshell
 include("Auditor.jl")
 include("Wizard.jl")
 
-using OutputCollectors, BinaryBuilderBase, .Auditor, .Wizard
+using BinaryBuilderBase, .Auditor, .Wizard
 
 # Autocomplete BinaryBuilder.run_wizard
 const run_wizard = Wizard.run_wizard
@@ -44,13 +44,6 @@ include("Declarative.jl")
 include("Logging.jl")
 
 function __init__()
-    if Base.thisminor(VERSION) >= v"1.8" && get(ENV, "JULIA_REGISTRYCI_AUTOMERGE", "false") != "true"
-        error("""
-        BinaryBuilder supports only Julia v1.7.
-        Contribute to JuliaPackaging/JLLPrefixes.jl#6 (<https://github.com/JuliaPackaging/JLLPrefixes.jl/issues/6>)
-        if you care about supporting newer versions of Julia.
-        """)
-    end
     # If we're running on Azure, enable azure logging:
     if !isempty(get(ENV, "AZP_TOKEN", ""))
         enable_azure_logging()

src/Wizard.jl

@@ -1,6 +1,6 @@
 module Wizard
 
-using BinaryBuilderBase, OutputCollectors, ..Auditor
+using BinaryBuilderBase, ..Auditor
 using Random
 using GitHub, LibGit2, Pkg, Sockets, ObjectFile
 import GitHub: gh_get_json, DEFAULT_API

src/auditor/dynamic_linkage.jl

@@ -392,7 +392,9 @@ function relink_to_rpath(prefix::Prefix, platform::AbstractPlatform, path::Abstr
             relink_cmd = `$install_name_tool -change $(old_libpath) @rpath/$(libname) $(rel_path)`
             @lock AUDITOR_SANDBOX_LOCK run(ur, relink_cmd, io; verbose=verbose)
         elseif Sys.islinux(platform) || Sys.isbsd(platform)
-            run_with_io(io, `$(patchelf()) $(patchelf_flags(platform)) --replace-needed $(old_libpath) $(libname) $(path)`)
+            with_patchelf_lock(path) do
+                run_with_io(io, `$(patchelf()) $(patchelf_flags(platform)) --replace-needed $(old_libpath) $(libname) $(path)`)
+            end
         end
     end
 end
@@ -448,57 +450,32 @@ function update_linkage(prefix::Prefix, platform::AbstractPlatform, path::Abstra
         return
     end
 
+    # macOS uses install_name_tool
+    if Sys.isapple(platform)
+        return _update_linkage_macho(prefix, platform, path, old_libpath, new_libpath; verbose, subdir)
+    end
+
+    # For Linux/FreeBSD, wrap the entire read-modify-write cycle in the file lock
+    # to prevent concurrent access while reading rpaths and running patchelf
+    return with_patchelf_lock(path) do
+        _update_linkage_elf(prefix, platform, path, old_libpath, new_libpath; verbose, subdir)
+    end
+end
+
+function _update_linkage_macho(prefix::Prefix, platform::AbstractPlatform, path::AbstractString,
+                               old_libpath, new_libpath; verbose::Bool=false, subdir::AbstractString="")
     ur = preferred_runner()(prefix.path; cwd="/workspace/", platform=platform)
     rel_path = relpath(path, prefix.path)
-
-    normalize_rpath = rp -> rp
-    add_rpath = x -> ``
-    relink = (x, y) -> ``
     install_name_tool = "/opt/bin/$(triplet(ur.platform))/install_name_tool"
-    if Sys.isapple(platform)
-        normalize_rpath = rp -> begin
-            if !startswith(rp, "@loader_path")
-                return "@loader_path/$(rp)"
-            end
-            return rp
-        end
-        add_rpath = rp -> `$install_name_tool -add_rpath $(rp) $(rel_path)`
-        relink = (op, np) -> `$install_name_tool -change $(op) $(np) $(rel_path)`
-    elseif Sys.islinux(platform) || Sys.isbsd(platform)
-        normalize_rpath = rp -> begin
-            if rp == "."
-                return "\$ORIGIN"
-            end
-            if startswith(rp, ".") || !startswith(rp, "/")
-                # Relative paths starting with `.`, or anything which isn't an absolute
-                # path.  It may also be a relative path without the leading `./`
-                return "\$ORIGIN/$(rp)"
-            end
-            return rp
-        end
-        current_rpaths = [r for r in _rpaths(path) if !isempty(r)]
-        add_rpath = rp -> begin
-            # Join together RPaths to set new one
-            rpaths = unique(vcat(current_rpaths, rp))
-
-            # I don't like strings ending in '/.', like '$ORIGIN/.'.  I don't think
-            # it semantically makes a difference, but why not be correct AND beautiful?
-            chomp_slashdot = path -> begin
-                if length(path) > 2 && path[end-1:end] == "/."
-                    return path[1:end-2]
-                end
-                return path
-            end
-            rpaths = chomp_slashdot.(rpaths)
-            # Remove paths starting with `/workspace`: they will not work outisde of the
-            # build environment and only create noise when debugging.
-            filter!(rp -> !startswith(rp, "/workspace"), rpaths)
 
-            rpath_str = join(rpaths, ':')
-            return `$(patchelf()) $(patchelf_flags(platform)) --set-rpath $(rpath_str) $(path)`
+    normalize_rpath = rp -> begin
+        if !startswith(rp, "@loader_path")
+            return "@loader_path/$(rp)"
         end
-        relink = (op, np) -> `$(patchelf()) $(patchelf_flags(platform)) --replace-needed $(op) $(np) $(path)`
+        return rp
     end
+    add_rpath = rp -> `$install_name_tool -add_rpath $(rp) $(rel_path)`
+    relink = (op, np) -> `$install_name_tool -change $(op) $(np) $(rel_path)`
 
     # If the relative directory doesn't already exist within the RPATH of this
     # binary, then add it in.
@@ -507,36 +484,86 @@ function update_linkage(prefix::Prefix, platform::AbstractPlatform, path::Abstra
         libname = basename(old_libpath)
         cmd = add_rpath(normalize_rpath(relpath(new_libdir, dirname(path))))
         with_logfile(prefix, "update_rpath_$(basename(path))_$(libname).log"; subdir) do io
-            if Sys.isapple(platform)
-                @lock AUDITOR_SANDBOX_LOCK run(ur, cmd, io; verbose=verbose)
-            elseif Sys.islinux(platform) || Sys.isbsd(platform)
-                run_with_io(io, cmd)
-            end
+            @lock AUDITOR_SANDBOX_LOCK run(ur, cmd, io; verbose=verbose)
         end
     end
 
     # Create a new linkage that uses the RPATH and/or environment variables to find things.
     # This allows us to split things up into multiple packages, and as long as the
     # libraries that this guy is interested in have been `dlopen()`'ed previously,
     # (and have the appropriate SONAME) things should "just work".
-    if Sys.isapple(platform)
-        # On MacOS, we need to explicitly add `@rpath/` before our library linkage path.
-        # Note that this is still overridable through DYLD_FALLBACK_LIBRARY_PATH
-        new_libpath = joinpath("@rpath", basename(new_libpath))
-    else
-        # We just use the basename on all other systems (e.g. Linux).  Note that using
-        # $ORIGIN, while cute, doesn't allow for overrides via LD_LIBRARY_PATH.  :[
-        new_libpath = basename(new_libpath)
-    end
+    # On MacOS, we need to explicitly add `@rpath/` before our library linkage path.
+    # Note that this is still overridable through DYLD_FALLBACK_LIBRARY_PATH
+    new_libpath = joinpath("@rpath", basename(new_libpath))
     cmd = relink(old_libpath, new_libpath)
     with_logfile(prefix, "update_linkage_$(basename(path))_$(basename(old_libpath)).log"; subdir) do io
-        if Sys.isapple(platform)
-            @lock AUDITOR_SANDBOX_LOCK run(ur, cmd, io; verbose=verbose)
-        elseif Sys.islinux(platform) || Sys.isbsd(platform)
+        @lock AUDITOR_SANDBOX_LOCK run(ur, cmd, io; verbose=verbose)
+    end
+
+    return new_libpath
+end
+
+# This function is called with the patchelf lock already held
+function _update_linkage_elf(prefix::Prefix, platform::AbstractPlatform, path::AbstractString,
+                             old_libpath, new_libpath; verbose::Bool=false, subdir::AbstractString="")
+    normalize_rpath = rp -> begin
+        if rp == "."
+            return "\$ORIGIN"
+        end
+        if startswith(rp, ".") || !startswith(rp, "/")
+            # Relative paths starting with `.`, or anything which isn't an absolute
+            # path.  It may also be a relative path without the leading `./`
+            return "\$ORIGIN/$(rp)"
+        end
+        return rp
+    end
+
+    current_rpaths = [r for r in _rpaths(path) if !isempty(r)]
+
+    add_rpath = rp -> begin
+        # Join together RPaths to set new one
+        rpaths = unique(vcat(current_rpaths, rp))
+        # I don't like strings ending in '/.', like '$ORIGIN/.'.  I don't think
+        # it semantically makes a difference, but why not be correct AND beautiful?
+        chomp_slashdot = path -> begin
+            if length(path) > 2 && path[end-1:end] == "/."
+                return path[1:end-2]
+            end
+            return path
+        end
+        rpaths = chomp_slashdot.(rpaths)
+        # Remove paths starting with `/workspace`: they will not work outside of the
+        # build environment and only create noise when debugging.
+        filter!(rp -> !startswith(rp, "/workspace"), rpaths)
+        rpath_str = join(rpaths, ':')
+        return `$(patchelf()) $(patchelf_flags(platform)) --set-rpath $(rpath_str) $(path)`
+    end
+
+    relink = (op, np) -> `$(patchelf()) $(patchelf_flags(platform)) --replace-needed $(op) $(np) $(path)`
+
+    # If the relative directory doesn't already exist within the RPATH of this
+    # binary, then add it in.
+    new_libdir = abspath(dirname(new_libpath) * "/")
+    if !(new_libdir in _canonical_rpaths(path))
+        libname = basename(old_libpath)
+        cmd = add_rpath(normalize_rpath(relpath(new_libdir, dirname(path))))
+        with_logfile(prefix, "update_rpath_$(basename(path))_$(libname).log"; subdir) do io
             run_with_io(io, cmd)
         end
     end
 
+    # Create a new linkage that uses the RPATH and/or environment variables to find things.
+    # This allows us to split things up into multiple packages, and as long as the
+    # libraries that this guy is interested in have been `dlopen()`'ed previously,
+    # (and have the appropriate SONAME) things should "just work".
+    # We just use the basename on Linux/BSD.  Note that using $ORIGIN, while cute,
+    # doesn't allow for overrides via LD_LIBRARY_PATH.  :[
+    new_libpath = basename(new_libpath)
+    cmd = relink(old_libpath, new_libpath)
+    with_logfile(prefix, "update_linkage_$(basename(path))_$(basename(old_libpath)).log"; subdir) do io
+        run_with_io(io, cmd)
+    end
+
     return new_libpath
 end
 

src/auditor/soname_matching.jl

@@ -65,15 +65,18 @@ function ensure_soname(prefix::Prefix, path::AbstractString, platform::AbstractP
     end
 
     # Otherwise, set the SONAME
-    # Create a new linkage that looks like @rpath/$lib on OSX,
+    # Create a new linkage that looks like @rpath/$lib on OSX
     retval = with_logfile(prefix, "set_soname_$(basename(rel_path))_$(soname).log"; subdir) do io
         if Sys.isapple(platform)
             ur = preferred_runner()(prefix.path; cwd="/workspace/", platform=platform)
             install_name_tool = "/opt/bin/$(triplet(ur.platform))/install_name_tool"
             set_soname_cmd = `$install_name_tool -id $(soname) $(rel_path)`
             @lock AUDITOR_SANDBOX_LOCK run(ur, set_soname_cmd, io; verbose=verbose)
         elseif Sys.islinux(platform) || Sys.isbsd(platform)
-            success(run_with_io(io, `$(patchelf()) $(patchelf_flags(platform)) --set-soname $(soname) $(realpath(path))`; wait=false))
+            # For Linux/FreeBSD, wrap the entire read-modify-verify cycle in the file lock
+            with_patchelf_lock(path) do
+                success(run_with_io(io, `$(patchelf()) $(patchelf_flags(platform)) --set-soname $(soname) $(realpath(path))`; wait=false))
+            end
         end
     end