Symlinking instead of copying tbb libs

[Enchufa2]

The new TBB_INC and TBB_LIB variables are great, but I noticed that RcppParallel copies tbb's .so files into the package directory. Is this really required? This is a problem for system packaging, because RPM generates a unique ID for each binary (see this), and thus copying a shared library makes these IDs collide, and the package cannot be installed. Would it be possible to symlink TBB_LIB instead?

[kevinushey]

That seems like a good idea to me. I need to resubmit for Solaris anyhow so I'll see if we can slipstream that into the next submission.

[kevinushey]

I now have the following on master:

kevinushey@cdrv-2:~/r/pkg/RcppParallel [master]
$ TBB_ROOT=/usr/local/opt/tbb R CMD INSTALL --preclean .
* installing to library ‘/Users/kevinushey/Library/R/4.0/library’
* installing *source* package ‘RcppParallel’ ...
** using staged installation
** preparing to cleanup package 'RcppParallel' ...
** finished cleanup for package 'RcppParallel'
** preparing to configure package 'RcppParallel' ...
*** configured file: 'src/Makevars.in' => 'src/Makevars'
** finished configure for package 'RcppParallel'
** libs
clang++ -mmacosx-version-min=10.13 -std=gnu++11 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I../inst/include -I/usr/local/opt/tbb/include  -I/usr/local/include  -std=gnu++11 -DRCPP_PARALLEL_USE_TBB=1 -DTBB_SUPPRESS_DEPRECATED_MESSAGES=1 -fPIC  -g -O3 -pipe -Wall -pedantic -c init.cpp -o init.o
clang++ -mmacosx-version-min=10.13 -std=gnu++11 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I../inst/include -I/usr/local/opt/tbb/include  -I/usr/local/include  -std=gnu++11 -DRCPP_PARALLEL_USE_TBB=1 -DTBB_SUPPRESS_DEPRECATED_MESSAGES=1 -fPIC  -g -O3 -pipe -Wall -pedantic -c options.cpp -o options.o
(tbb) Using system (Intel/OneAPI) TBB library ...
(tbb) TBB_LIB = /usr/local/opt/tbb/lib
(tbb) TBB_INC = /usr/local/opt/tbb/include
clang++ -mmacosx-version-min=10.13 -std=gnu++11 -dynamiclib -Wl,-headerpad_max_install_names -undefined dynamic_lookup -single_module -multiply_defined suppress -L/Library/Frameworks/R.framework/Resources/lib -L/usr/local/lib -o RcppParallel.so init.o options.o -Wl,-L,/usr/local/opt/tbb/lib -Wl,-rpath,/usr/local/opt/tbb/lib -ltbb -ltbbmalloc -F/Library/Frameworks/R.framework/.. -framework R -Wl,-framework -Wl,CoreFoundation
installing via 'install.libs.R' to /Users/kevinushey/Library/R/4.0/library/00LOCK-RcppParallel/00new/RcppParallel
** R
** inst
** byte-compile and prepare package for lazy loading
** help
*** installing help indices
** building package indices
** testing if installed package can be loaded from temporary location
** checking absolute paths in shared objects and dynamic libraries
** testing if installed package can be loaded from final location
** testing if installed package keeps a record of temporary installation path
* DONE (RcppParallel)
kevinushey@cdrv-2:~/r/pkg/RcppParallel [master]
$ tree ~/Library/R/4.0/library/RcppParallel/libs
/Users/kevinushey/Library/R/4.0/library/RcppParallel/libs
├── RcppParallel.so
└── tbb
    ├── libtbb.dylib -> /usr/local/opt/tbb/lib/libtbb.dylib
    ├── libtbbmalloc.dylib -> /usr/local/opt/tbb/lib/libtbbmalloc.dylib
    └── libtbbmalloc_proxy.dylib -> /usr/local/opt/tbb/lib/libtbbmalloc_proxy.dylib

1 directory, 4 files

Does that look okay?

[Enchufa2]

LGTM!

[hsbadr]

@kevinushey I remember that we copy the libs to keep the version linked, avoiding breaking RcppParallel when the original files are moved, removed, or upgraded to an incompatible version. But, I do prefer the symlinks too. I suggest that we store the md5 hash of the shared files and check that before loading them. If they don't match, we continue with a warning that recommends reinstalling the package. This way, if the new version causes loading failure, the users will know that they've to reinstall and link to the new TBB libraries. Let me know if I can help with fixing the CRAN issues.

[Enchufa2]

I suggest that we store the md5 hash of the shared files and check that before loading them. If they don't match, we continue with a warning that recommends reinstalling the package. This way, if the new version causes loading failure, the users will know that they've to reinstall and link to the new TBB libraries.

That won't happen with system builds, where the .rpm, .deb is always linked to the correct tbb version, but in general it's a good idea. In this way, you can ensure integrity in user installations too.

[kevinushey]

RcppParallel 5.1.2 is now on CRAN, which uses symlinks by default. Let me know if you're still having trouble after that release propagates on CRAN.

[kevinushey]

@hsbadr: would you be willing to file a separate issue for us to track the MD5 hashes of the TBB libraries, so we can detect if they have changed?

[hsbadr]

RcppParallel 5.1.2 is now on CRAN, which uses symlinks by default. Let me know if you're still having trouble after that release propagates on CRAN.

I don't run into those issues b/c I always reinstall the development version of RcppParallel, especially after TBB updates. However, loading RcppParallel will fail if, for example, it can't find a reference in libtbb after version upgrade. This can easily get fixed with relinking to the new TBB library with reinstalling the package.

@hsbadr: would you be willing to file a separate issue for us to track the MD5 hashes of the TBB libraries, so we can detect if they have changed?

Sure. We may also add a function to store the TBB version info (e.g., TBB_INTERFACE_VERSION) at compilation time and check the runtime version.

An example for this is rgdal, which tests the runtime version of GDAL:
https://github.com/cran/rgdal/blob/49b03833bce8ff907a06ced5be0b875eb79f88e1/R/AAA.R#L99-L100

[hsbadr]

@kevinushey #164 is not applicable to bundled dependencies, as @Enchufa2 mentioned. It's important to ensure integrity of the shared objects, before loading them, though. This will also inform the users if the package loading failed due to this issue.

[Enchufa2]

RcppParallel 5.1.2 is now on CRAN, which uses symlinks by default. Let me know if you're still having trouble after that release propagates on CRAN.

It was successfully auto-updated a few hours ago and looks ok. There's a difference with the macOS installation above though, as you can see:

$ ll /usr/local/lib/R/library/RcppParallel/libs/
total 20
-rwxr-xr-x. 1 root root 19816 abr 16 02:41 RcppParallel.so
$ ll /usr/local/lib/R/library/RcppParallel/lib/
total 0
lrwxrwxrwx. 1 root root 34 abr 16 02:41 libtbbmalloc_proxy.so.2 -> /usr/lib64/libtbbmalloc_proxy.so.2
lrwxrwxrwx. 1 root root 28 abr 16 02:41 libtbbmalloc.so.2 -> /usr/lib64/libtbbmalloc.so.2
lrwxrwxrwx. 1 root root 22 abr 16 02:41 libtbb.so.2 -> /usr/lib64/libtbb.so.2

The links are in lib, instead of libs/tbb. Is this intended? As long as RcppParallel finds them, it doesn't matter much.

[kevinushey]

That is intentional. I wanted to move the library paths, but unfortunately some packages (rstan / StanHeaders) hard-code the path to the bundled version of TBB in RcppParallel and so trying to move it breaks those packages. 😞

[hsbadr]

That is intentional. I wanted to move the library paths, but unfortunately some packages (rstan / StanHeaders) hard-code the path to the bundled version of TBB in RcppParallel and so trying to move it breaks those packages. 😞

I could update that in the development version of rstan/StanHeaders. But, why do you wanna move the TBB libs?