resolve the tbb library path against the installed package on windows (#273)

tbbLibraryPath() returned NULL on Windows (#270).

The cause is the set of filenames it looks for: the static archives
'libtbb12.a' and 'libtbb.a'. Neither is ever installed with the package. TBB
is linked statically into RcppParallel.dll on Windows, and the only TBB
library installed alongside it is the tbb.dll stub, so look for that first.
The archives are kept as later candidates, for a TBB_LIB pointed at an Rtools
tree at runtime.

Note this is not the TBB_LIB problem the report proposed, though the report is
right that TBB_LIB is also wrong here. The archive lookup fails even when the
search directory is correct: a bundled-oneTBB build records no TBB_LIB, so
tbbRoot() already answered with the package's own lib directory, and
tbbLibraryPath("tbb") still returned NULL. The lookup only ever succeeded when
the package happened to run on the machine that built it against an Rtools TBB
-- and then answered with a path into Rtools, a build dependency, rather than
with anything the installed package uses.

Separately, tbbLibraryPath(NULL) and tbbRoot() return the search directory
itself, with no existence check, so there the configure-time TBB_LIB really
was the defect: for a pre-built binary it names a directory on the build
machine ('D:/rtools45/...' in the report). Answer with our own library
directory on Windows instead. This is already how tbbLdFlags() and
loadTbbLibrary() behave; tbbRoot() was the last place trusting TBB_LIB.

Scoped to Windows deliberately. Off Windows a configured TBB_LIB names where
the libraries genuinely are -- the package symlinks them into its own lib
directory rather than copying -- and tbbLdFlags() emits it as an -rpath.

tbbLibraryPath("tbbmalloc") now returns NULL on Windows, which is honest: no
separate tbbmalloc library is installed there.

Author: kevinushey

NEWS.md

@@ -8,6 +8,24 @@
   loaded into the process, and failed with "Library not loaded:
   @rpath/libtbb.dylib" otherwise. (#209)
 
+* Fixed `RcppParallel::tbbLibraryPath()` returning `NULL` on Windows. It looked
+  for the static archives `libtbb12.a` / `libtbb.a`, which are never installed
+  with the package: TBB is linked statically into `RcppParallel.dll` there, and
+  the only TBB library installed alongside it is the `tbb.dll` stub, which is
+  what it now finds. Previously it succeeded only when the package happened to
+  be running on the machine that built it against an Rtools TBB, and even then
+  answered with a path into Rtools -- a build dependency -- rather than with
+  anything the installed package uses. Note that `tbbLibraryPath("tbbmalloc")`
+  returns `NULL` on Windows, as no separate tbbmalloc library is installed
+  there. (#270)
+
+* `RcppParallel::tbbLibraryPath()` called with no arguments, and the internal
+  `tbbRoot()`, no longer report the Rtools directory recorded when the package
+  was configured. Unlike the named-library form, these return their answer
+  without checking that it exists, so for a pre-built binary (e.g. the CRAN
+  build) they named a directory on the machine that built the package. On
+  Windows both now report the package's own library directory. (#270)
+
 * 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

@@ -22,9 +22,13 @@ tbbLibraryPath <- function(name = NULL) {
       return(tbbRoot)
 
    # form library names
+   #
+   # on Windows the library we install is the 'tbb.dll' stub -- TBB itself is
+   # inside RcppParallel.dll -- so look for that first. the static archives are
+   # still tried afterwards, for a TBB_LIB pointed at an Rtools tree at runtime
    tbbLibNames <- list(
       "Darwin"  = paste0("lib", name, ".dylib"),
-      "Windows" = paste0("lib", name, c("12", ""), ".a"),
+      "Windows" = c(paste0(name, ".dll"), paste0("lib", name, c("12", ""), ".a")),
       "SunOS"   = paste0("lib", name, ".so"),
       "Linux"   = paste0("lib", name, c(".so.2", ".so"))
    )
@@ -146,6 +150,15 @@ tbbLdFlags <- function() {
 
 tbbRoot <- function() {
 
+   # on Windows, always answer with our own library directory. TBB is linked
+   # statically into RcppParallel.dll there, and the only TBB library we
+   # install is the tbb.dll stub, so that directory is the whole story. TBB_LIB
+   # is worse than useless here: it records the Rtools tree of whichever
+   # machine ran configure, which for a pre-built binary is a path that need
+   # not exist on the user's machine at all (#270)
+   if (is_windows())
+      return(archSystemFile("lib"))
+
    if (nzchar(TBB_LIB))
       return(TBB_LIB)
 

R/zzz.R

@@ -21,8 +21,9 @@ loadTbbLibrary <- function(name) {
    if (is_windows()) {
 
       # NOTE: resolve against the package's own library directory, where
-      # install.libs.R places the stub; tbbRoot() would prefer TBB_LIB,
-      # which on Windows points at Rtools and holds only static libraries
+      # install.libs.R places the stub. this is what tbbRoot() reports on
+      # Windows too; spell it out here so that loading during .onLoad does
+      # not depend on the configured TBB_LIB in any way
       path <- archSystemFile("lib", paste0(name, ".dll"))
       if (!file.exists(path)) {
          verboseMessage("tbb library '%s' not found in package 'lib' folder; skipping", name)

tests/test-tbb-library-path.R

@@ -0,0 +1,70 @@
+# Unit tests for tbbRoot() and tbbLibraryPath(), which resolve the TBB library
+# that the *installed* package uses.
+#
+# Regression tests for #270: on Windows these consulted TBB_LIB, the library
+# directory recorded when configure ran. For a package built from source that
+# happens to be a real path, but for a pre-built binary it names the Rtools
+# tree of the build machine -- so tbbRoot() reported a directory that need not
+# exist on the user's machine (a 'D:/rtools45/...' path, in the report), and
+# tbbLibraryPath() then found nothing and returned NULL.
+#
+# The invariant these tests pin down is that both functions describe the
+# installation actually in use: whatever they return must exist.
+
+RcppParallel:::test_init()
+
+# tbbRoot() must never report a directory that isn't there. system.file()
+# returns "" when it cannot resolve a path, so that sentinel is allowed; any
+# non-empty answer has to be a real directory.
+root <- tbbRoot()
+assert(is.character(root))
+assert(length(root) == 1L)
+if (nzchar(root))
+   assert(dir.exists(root))
+
+# on Windows the answer is always the package's own library directory, since
+# TBB is statically linked into RcppParallel.dll and the tbb.dll stub is the
+# only TBB library installed. in particular it must not depend on TBB_LIB.
+if (is_windows()) {
+
+   assert(identical(root, archSystemFile("lib")))
+
+   saved <- Sys.getenv("TBB_LIB", unset = NA)
+   Sys.setenv(TBB_LIB = "Z:/nonexistent/rtools/lib")
+   assert(identical(tbbRoot(), archSystemFile("lib")))
+   if (is.na(saved)) Sys.unsetenv("TBB_LIB") else Sys.setenv(TBB_LIB = saved)
+
+}
+
+# tbbLibraryPath(NULL) reports the directory, and agrees with tbbRoot() when
+# TBB_LIB is not set in the environment
+if (is.na(Sys.getenv("TBB_LIB", unset = NA)))
+   assert(identical(tbbLibraryPath(), root))
+
+# a named library either resolves to a file that exists, or isn't found at all.
+# it must never name a file that isn't there -- that was the shape of #270.
+for (name in c("tbb", "tbbmalloc", "tbbmalloc_proxy")) {
+
+   path <- tbbLibraryPath(name)
+   if (is.null(path))
+      next
+
+   assert(is.character(path))
+   assert(length(path) == 1L)
+   assert(file.exists(path))
+
+}
+
+# the user-visible symptom from #270: tbbLibraryPath("tbb") returned nothing
+# from a perfectly good installation. assert this only on Windows, where the
+# installed layout is known exactly -- install.libs.R always builds the
+# tbb.dll stub when TBB is enabled, and fails the install if it cannot. off
+# Windows the library may legitimately be unresolvable, e.g. a system TBB
+# whose distro package ships no unversioned 'libtbb.so' development symlink.
+if (is_windows() && TBB_ENABLED) {
+   tbb <- tbbLibraryPath("tbb")
+   assert(!is.null(tbb))
+   assert(file.exists(tbb))
+}
+
+writeLines("all tbbLibraryPath() tests passed")