Bioconductor / Rhtslib

HTSlib high-throughput sequencing library as an R package
https://bioconductor.org/packages/Rhtslib
11 stars 12 forks source link

No support for Makevars #27

Closed drkrynstrng closed 2 years ago

drkrynstrng commented 3 years ago

Similar to previous issues #3, #12, and #15, currently Rhtslib does not support site-wide or user-specified Makevars files where variables like CPPFLAGS and LDFLAGS can be used to point to non-standard install paths. I'm also in an HPC cluster environment where software is installed in non-standard locations using Spack. Supporting the use of Makevars files, such as via a configure script, would make package installation easier in this environment. The package can certainly be installed by modifying the source code Makefiles, but that's both unusual and inefficient. Supporting Makevars would also be useful for customizing optimization options, for example. https://cran.r-project.org/doc/manuals/r-devel/R-admin.html#Customizing-package-compilation

RonRahaman commented 2 years ago

+1 I am on a cluster environment, and supporting a custom compiler option is essential.

hpages commented 2 years ago

Hi,

Things like CC, CFLAGS, CPPFLAGS, LDFLAGS, etc... are controlled by R. You can see their values with R CMD config <VAR>. For example, on the Bioconductor Linux build machines:

biocbuild@nebbiolo2:~$ R CMD config CC
gcc

biocbuild@nebbiolo2:~$ R CMD config CFLAGS
-g -O2 -Wall

biocbuild@nebbiolo2:~$ R CMD config CPPFLAGS
-I/usr/local/include

biocbuild@nebbiolo2:~$ R CMD config LDFLAGS
-L/usr/local/lib

If things like the compilers or external software are in non-standard locations, then your R installation should reflect that. If it doesn't then your R is misconfigured and you will likely have problems installing other packages with native code, not just Rhtslib.

See issue #12 for a more detailed discussion of this topic.

In other words, this is an issue with your R installation, not an Rhtslib issue per se.

H.

RonRahaman commented 2 years ago

There’s stuff that isn’t a consequence of misconfiguration, but rather, are helpful for an R installation with large sets of dependencies on a heterogeneous and highly optimized system. For example, certain optimization flags could break some packages but be really important for others. We also might want different packages to link to different versions of the same library.

I’m only suggest that Rhtslib support these compiling/linking capabilities because R already recommends Makevars precisely for this purpose.

hpages commented 2 years ago

For example, certain optimization flags could break some packages but be really important for others. We also might want different packages to link to different versions of the same library.

Yes, in general. But it would still be nice to provide a compelling case where using a different compiler or different compiler options for Rhtslib vs those used for R is legit. The various requests I've seen so far for supporting a site-wide or user-specified Makevars failed to provide this.

Anyways, this should now be supported in Rhtslib 1.27.1 (see commit f72c8cbe8e91548b9e95b40c70345f3cc46b188e). That's on non-Windows system only for now.

H.

drkrynstrng commented 2 years ago

Thanks, this latest commit worked for me.

RonRahaman commented 2 years ago

Thank you for begrudgingly supporting this :)

hsiaoyi0504 commented 2 years ago

Just saw this. Thank you for the update. Will this be updated to Bioconductor in the near future?

lshep commented 2 years ago

@hsiaoyi0504 Please provide more information. The commit listed above is in Bioconductor as the current release version is 1.28.0 and the current devel version is 1.99.5.
Does it not work for you?
What is your sessionInfo() , which will provide details on your version of R/Bioconductor and OS?

hsiaoyi0504 commented 2 years ago

@lshep I wasn't able to install the current version through renv, but able to install the version here in github through renv

And here is my session info.

R version 4.1.3 beta (2022-02-27 r81833)
Platform: aarch64-apple-darwin20 (64-bit)
Running under: macOS Big Sur 11.6.4

Matrix products: default
LAPACK: /Library/Frameworks/R.framework/Versions/4.1-arm64/Resources/lib/libRlapack.dylib

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

attached base packages:
[1] stats     graphics  grDevices datasets  utils     methods   base     

other attached packages:
[1] xlsx_0.6.5    ggplot2_3.3.5

loaded via a namespace (and not attached):
 [1] magrittr_2.0.2      tidyselect_1.1.2    munsell_0.5.0       colorspace_2.0-3    R6_2.5.1            rlang_1.0.1         fansi_1.0.2         dplyr_1.0.8         tools_4.1.3        
[10] grid_4.1.3          gtable_0.3.0        utf8_1.2.2          cli_3.2.0           withr_2.4.3         ellipsis_0.3.2      digest_0.6.29       tibble_3.1.6        lifecycle_1.0.1    
[19] crayon_1.5.0        rJava_1.0-6         farver_2.1.0        BiocManager_1.30.16 purrr_0.3.4         xlsxjars_0.6.1      vctrs_0.3.8         glue_1.6.2          labeling_0.4.2     
[28] compiler_4.1.3      pillar_1.7.0        generics_0.1.2      scales_1.1.1        renv_0.13.2         pkgconfig_2.0.3    
hsiaoyi0504 commented 2 years ago

I meet a similar issue in a related tool https://github.com/Bioconductor/Rsamtools as well. Both couldn't be installed successfully from Bioconductor through renv but could be installed through pointing renv to use the copy on GitHub.

hpages commented 2 years ago

Hi @hsiaoyi0504 ,

This update to Rhtslib was introduced in BioC 3.15 which was released in April 2022. This is the most current version of Bioconductor.

BioC 3.15 requires R 4.2 (latest R) but you have R 4.1 so you are probably using an older version of Bioconductor. Note that trying to force an installation of BioC 3.15 on top of R 4.1 is a bad idea as it will introduce compatibility problems. Please always install/use the most current version of Bioconductor on top of the most current version of R.

Also always use BiocManager::install() to install Bioconductor packages (also works for CRAN packages). Installing directly from GitHub (via renv or other tools) should be avoided as it will also introduce compatibility problems.

H.

hsiaoyi0504 commented 2 years ago

Oh, that's true. Here is the log of Bioconductor

> library(BiocManager)
Bioconductor version 3.14 (BiocManager 1.30.16), R 4.1.3 beta (2022-02-27 r81833)
Bioconductor version '3.14' is out-of-date; the current release version '3.15' is available with R version '4.2'; see https://bioconductor.org/install

I should update it soon. Thanks for catching that!