Closed
Description
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?
Metadata
Metadata
Assignees
Labels
No labels
Activity
kevinushey commentedon Apr 12, 2021
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 commentedon Apr 12, 2021
I now have the following on master:
Does that look okay?
Enchufa2 commentedon Apr 12, 2021
LGTM!
hsbadr commentedon Apr 12, 2021
@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 commentedon Apr 13, 2021
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 commentedon Apr 15, 2021
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 commentedon Apr 15, 2021
@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 commentedon Apr 15, 2021
I don't run into those issues b/c I always reinstall the development version of
RcppParallel
, especially after TBB updates. However, loadingRcppParallel
will fail if, for example, it can't find a reference inlibtbb
after version upgrade. This can easily get fixed with relinking to the new TBB library with reinstalling the package.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 commentedon Apr 15, 2021
@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 commentedon Apr 16, 2021
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:
The links are in
lib
, instead oflibs/tbb
. Is this intended? As long as RcppParallel finds them, it doesn't matter much.kevinushey commentedon Apr 16, 2021
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 commentedon Apr 16, 2021
I could update that in the development version of
rstan
/StanHeaders
. But, why do you wanna move the TBB libs?13 remaining items