search

grimbough / Rhdf5lib

Distribution of the HDF5 library in an R package
https://bioconductor.org/packages/Rhdf5lib/
6 stars 14 forks source link

Passing AR and RANLIB to configure for szip/hdf5 #41

Closed miesav closed 3 years ago

miesav commented 3 years ago

HI,

Made some small modifications to cover the case where R was built with a compiler that is not in a standard location. The change introduces the variables AR and RANLIB to the configure steps for hdf5 and szip. This should not cause any issues regardless if R was built with LTO or not. Rhdf5lib compiles fine with or without -flto.

As you are probably aware, R 4.1.0 provides flags to control -flto when compiling packages. There is only one small case to cover after all this changes:

If you use --no-use-LTO, -flto is not set when compiling Rhdf5lib. Nevertheless, it is always set when building hdf5 and szip. I am not very sure how R controls this. So if R is built to use LTO, it will always compile hdf5 and szip with -flto. If there is someone using R with LTO support but not enabled unless you specify --use-LTO, szip and hdf5 will not be built with -flto.

Hope this makes sense and covers these edge cases.

Cheers.

PeteHaitch commented 3 years ago

@grimbough for context, this PR should hopefully fix #40 and is authored by our sysadmin

PeteHaitch commented 3 years ago

These edits may need to be made to configure.ac rather than configure, I think?

miesav commented 3 years ago

AR and RANLIB can go there, would be cleaner, I just added what I tested to make it compile :)

Forgot to mention, if LTO is to be avoided all-together (probably won't affect szip and hdf5 configure steps), UseLTO: false can be added to the DESCRIPTION file. It can still be overridden by --use-LTO.

miesav commented 3 years ago

Moved to configure.ac. Just needs an update with autoconf.

grimbough commented 3 years ago

Thanks for the pull request. I'll give it a test on my machines and then push to Bioconductor.

I seem to remember running into some problems with R CMD config AR when developing rhdf5filters (hence this workaround https://github.com/grimbough/rhdf5filters/blob/31a97e1630333e0a2d6fa9d9ae089f093832230c/configure.ac#L29-L31) but hopefully that's something that's been fixed in more recent R versions.