emit an -rpath for tbb in LdFlags() on macos (#271)

* emit an -rpath for tbb in LdFlags() on macos

The TBB libraries RcppParallel ships on macOS record an '@rpath'-relative
install name, but LdFlags() emitted only a '-L' search path. That satisfies
the linker, so downstream packages built fine, but the resulting binary
carried no LC_RPATH at all: dyld could resolve '@rpath/libtbb.dylib' only
when RcppParallel -- and hence TBB -- had already been loaded into the
process. Loading such a binary on its own failed with "Library not loaded:
@rpath/libtbb.dylib".

Pair the '-L' with a matching '-rpath', as the TBB_LIB branch just above
already does. A stale rpath (e.g. in a binary package built where
RcppParallel lived at a different path) is harmless: dyld skips a path that
does not resolve and falls back to matching the already-loaded image, which
is the current behaviour.

Fixes #209.

* test that a downstream package loads standalone

The flag-level check only asserts that LdFlags() looks right. Add a test
that exercises the behaviour: build a package against those flags, then
load its shared object from a fresh R process that never loads
RcppParallel.

That last part is the whole point. Loading such a package the usual way
goes through its NAMESPACE, whose importFrom(RcppParallel, ...) pulls TBB
into the process first and so succeeds either way -- as does the install,
since the missing '-rpath' only affects load time, not linking. Only a
direct dyn.load() in a clean process distinguishes the two, failing with
"Library not loaded: @rpath/libtbb.dylib" without the fix.

The test is CI-gated, as it needs a toolchain, and runs only where TBB is
linked into the client explicitly (macOS, or a system TBB); elsewhere the
symbols are deliberately resolved from the libraries RcppParallel loads.
That gate mirrors the branches of tbbLdFlags() rather than inspecting its
output, since gating on the '-rpath' under test would skip the very
configuration that regressed.

Author: kevinushey

NEWS.md

@@ -1,5 +1,13 @@
 # RcppParallel (development version)
 
+* On macOS, `RcppParallel::LdFlags()` now also emits an `-rpath` entry for the
+  directory containing the TBB libraries. The libraries record an
+  `@rpath`-relative install name, so packages linking against them previously
+  produced binaries with no runtime search path for TBB; those binaries could
+  only be loaded when RcppParallel (and hence TBB) already happened to be
+  loaded into the process, and failed with "Library not loaded:
+  @rpath/libtbb.dylib" otherwise. (#209)
+
 * Fixed an issue where compiling code including `tbb/parallel_for_each.h`
   could fail with toolchains that accept `-std=c++20` but provide a
   pre-C++20 standard library, with errors of the form "no member named

R/tbb.R

@@ -121,8 +121,15 @@ tbbLdFlags <- function() {
 
    # explicitly link on macOS
    # https://github.com/RcppCore/RcppParallel/issues/206
+   #
+   # the bundled libraries record an '@rpath'-relative install name (e.g.
+   # '@rpath/libtbb.dylib'), so '-L' alone is not enough: the client library
+   # ends up with no runtime search path for TBB at all, and only loads
+   # because RcppParallel -- and hence TBB -- normally happens to be loaded
+   # into the process first. Emit a matching '-rpath' so the client can
+   # resolve TBB on its own. (#209)
    if (is_mac()) {
-      fmt <- "-L%s -l%s -l%s"
+      fmt <- "-L%1$s -Wl,-rpath,%1$s -l%2$s -l%3$s"
       return(sprintf(fmt, asBuildPath(tbbLibraryPath()), TBB_NAME, TBB_MALLOC_NAME))
    }
 

tests/test-downstream-load.R

@@ -0,0 +1,140 @@
+
+# Check that a package linking against RcppParallel's TBB can be loaded on its
+# own, in a process that has not already loaded RcppParallel.
+#
+# The TBB libraries we ship on macOS record an '@rpath'-relative install name,
+# so LdFlags() must emit an '-rpath' alongside its '-L'. With only the '-L' the
+# link still succeeds -- so the package builds, and loads fine via its
+# NAMESPACE, whose importFrom(RcppParallel, ...) pulls TBB into the process
+# first -- but the shared object itself carries no runtime search path, and
+# dyld fails with "Library not loaded: @rpath/libtbb.dylib" for anything that
+# loads it directly. Hence the load below runs in a fresh R process which never
+# touches RcppParallel; loading it from here would prove nothing.
+#
+# See also tests/test-ld-flags.R, the cheap flag-level check that still runs
+# where no toolchain is available.
+#
+# https://github.com/RcppCore/RcppParallel/issues/209
+
+RcppParallel:::test_init()
+
+# building the test package requires a toolchain, so keep this test
+# off of CRAN's machines
+ci <- nzchar(Sys.getenv("CI")) || identical(Sys.getenv("NOT_CRAN"), "true")
+if (!ci) {
+   writeLines("Not running on CI; skipping downstream load test.")
+   quit(save = "no")
+}
+
+if (!TBB_ENABLED) {
+   writeLines("TBB is not enabled; skipping downstream load test.")
+   quit(save = "no")
+}
+
+# Windows has no rpath: the loader resolves DLLs through the process search
+# path, and downstream packages link '-lRcppParallel' rather than TBB itself
+if (is_windows()) {
+   writeLines("Windows resolves DLLs via the search path; skipping.")
+   quit(save = "no")
+}
+
+# A standalone load can only work where TBB is linked into the client
+# explicitly: macOS (#206), and any platform configured against a system TBB.
+# Elsewhere the TBB symbols are deliberately left undefined at link time and
+# resolved from the libraries RcppParallel loads, so a standalone load is
+# expected to fail. Note this mirrors the branches in tbbLdFlags() rather than
+# inspecting its output: gating on the '-rpath' we are checking for would skip
+# precisely the configuration that regressed.
+tbbLib <- Sys.getenv("TBB_LINK_LIB", Sys.getenv("TBB_LIB", unset = TBB_LIB))
+if (!is_mac() && !nzchar(tbbLib)) {
+   writeLines("TBB is not linked into downstream packages here; skipping.")
+   quit(save = "no")
+}
+
+# generate the test package
+pkgRoot <- file.path(tempdir(), "downstreamtest")
+dir.create(file.path(pkgRoot, "src"), recursive = TRUE, showWarnings = FALSE)
+
+writeLines(con = file.path(pkgRoot, "DESCRIPTION"), c(
+   "Package: downstreamtest",
+   "Type: Package",
+   "Title: Test Loading a Package Linked Against RcppParallel's TBB",
+   "Version: 0.1.0",
+   "Author: RcppParallel Authors",
+   "Maintainer: RcppParallel Authors <noreply@example.com>",
+   "Description: Confirms that such packages can be loaded standalone.",
+   "License: GPL-2",
+   "Imports: RcppParallel"
+))
+
+writeLines(con = file.path(pkgRoot, "NAMESPACE"), c(
+   "useDynLib(downstreamtest)",
+   "importFrom(RcppParallel, RcppParallelLibs)"
+))
+
+writeLines(con = file.path(pkgRoot, "src", "Makevars"), c(
+   'PKG_CXXFLAGS = $(shell "${R_HOME}/bin/Rscript" -e "RcppParallel::CxxFlags()")',
+   'PKG_LIBS = $(shell "${R_HOME}/bin/Rscript" -e "RcppParallel::RcppParallelLibs()")'
+))
+
+# call into TBB proper, so that the shared object records a real load-time
+# dependency on the TBB library rather than merely being linked against it
+writeLines(con = file.path(pkgRoot, "src", "concurrency.cpp"), c(
+   "#include <tbb/task_arena.h>",
+   "",
+   "#include <R.h>",
+   "#include <Rinternals.h>",
+   "",
+   'extern "C" SEXP downstream_max_concurrency(void)',
+   "{",
+   "   return Rf_ScalarInteger(tbb::this_task_arena::max_concurrency());",
+   "}"
+))
+
+# install it, making sure child processes resolve this RcppParallel
+libDir <- file.path(tempdir(), "library")
+dir.create(libDir, recursive = TRUE, showWarnings = FALSE)
+Sys.setenv(R_LIBS = paste(.libPaths(), collapse = .Platform$path.sep))
+
+rExe <- file.path(R.home("bin"), "R")
+args <- c("CMD", "INSTALL", "--no-multiarch", paste0("--library=", shQuote(libDir)), shQuote(pkgRoot))
+
+output <- suppressWarnings(system2(rExe, args, stdout = TRUE, stderr = TRUE))
+writeLines(output)
+
+status <- attr(output, "status")
+if (is.numeric(status) && status != 0L)
+   stop("error installing test package (status code ", status, ")")
+
+# locate the shared object that was just built
+pattern <- paste0("^downstreamtest\\", .Platform$dynlib.ext, "$")
+shlib <- list.files(
+   file.path(libDir, "downstreamtest", "libs"),
+   pattern    = pattern,
+   recursive  = TRUE,
+   full.names = TRUE
+)
+
+assert(length(shlib) == 1L)
+
+# load it from a process which has never loaded RcppParallel; deparse() is used
+# to embed the path as a properly-quoted R string literal
+script <- file.path(tempdir(), "downstream-load.R")
+writeLines(con = script, c(
+   'if ("RcppParallel" %in% loadedNamespaces())',
+   '   stop("RcppParallel is loaded; this check would prove nothing")',
+   "",
+   sprintf("dll <- dyn.load(%s)", deparse(shlib)),
+   'value <- .Call(getNativeSymbolInfo("downstream_max_concurrency", dll))',
+   'cat("max concurrency:", value, "\\n")',
+   "",
+   'if (!is.integer(value) || is.na(value) || value < 1L)',
+   '   stop("unexpected concurrency: ", value)'
+))
+
+rscript <- file.path(R.home("bin"), "Rscript")
+output <- suppressWarnings(system2(rscript, c("--vanilla", shQuote(script)), stdout = TRUE, stderr = TRUE))
+writeLines(output)
+
+status <- attr(output, "status")
+assert(is.null(status) || status == 0L)

tests/test-ld-flags.R

@@ -0,0 +1,38 @@
+# Unit tests for tbbLdFlags(), the linker flags handed to downstream packages
+# via RcppParallel::LdFlags().
+#
+# On macOS, the TBB libraries RcppParallel ships record an '@rpath'-relative
+# install name (e.g. '@rpath/libtbb.dylib'), so a '-L' search path alone only
+# satisfies the linker: the resulting binary carries no runtime search path for
+# TBB, and dyld can resolve it only when RcppParallel has already pulled TBB
+# into the process. Every '-L' we emit must therefore be paired with a matching
+# '-rpath'. (#209)
+#
+# This is only a check on the shape of the flags, so that something still runs
+# where no toolchain is available (e.g. CRAN). tests/test-downstream-load.R
+# covers the behaviour itself: it builds a package against these flags and
+# loads it in a process that never loads RcppParallel.
+
+RcppParallel:::test_init()
+
+flags <- tbbLdFlags()
+
+assert(is.character(flags))
+assert(length(flags) == 1L)
+assert(!is.na(flags))
+
+if (is_mac()) {
+
+   parts <- strsplit(flags, "\\s+")[[1L]]
+
+   libPaths <- sub("^-L", "", grep("^-L", parts, value = TRUE))
+   rpaths <- sub("^-Wl,-rpath,", "", grep("^-Wl,-rpath,", parts, value = TRUE))
+
+   # we always link TBB explicitly on macOS, so there is a search path to check
+   assert(length(libPaths) > 0L)
+   assert(all(nzchar(libPaths)))
+
+   # and each one must also be searched at load time
+   assert(all(libPaths %in% rpaths))
+
+}