Skip to content

Symlinking instead of copying tbb libs #161

Closed
@Enchufa2

Description

@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?

Activity

kevinushey

kevinushey commented on Apr 12, 2021

@kevinushey
Contributor

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

kevinushey commented on Apr 12, 2021

@kevinushey
Contributor

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

Enchufa2 commented on Apr 12, 2021

@Enchufa2
MemberAuthor

LGTM!

hsbadr

hsbadr commented on Apr 12, 2021

@hsbadr
Contributor

@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

Enchufa2 commented on Apr 13, 2021

@Enchufa2
MemberAuthor

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

kevinushey commented on Apr 15, 2021

@kevinushey
Contributor

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

kevinushey commented on Apr 15, 2021

@kevinushey
Contributor

@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

hsbadr commented on Apr 15, 2021

@hsbadr
Contributor

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

hsbadr commented on Apr 15, 2021

@hsbadr
Contributor

@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

Enchufa2 commented on Apr 16, 2021

@Enchufa2
MemberAuthor

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

kevinushey commented on Apr 16, 2021

@kevinushey
Contributor

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

hsbadr commented on Apr 16, 2021

@hsbadr
Contributor

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?

13 remaining items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Symlinking instead of copying tbb libs · Issue #161 · RcppCore/RcppParallel