statgen / savvy

Interface to various variant calling formats.
Mozilla Public License 2.0
26 stars 5 forks source link

Build failure related to crossplatform sprintf: expected ')' before 'PRId64' #23

Closed pettyalex closed 2 years ago

pettyalex commented 2 years ago

I'm running into another build problem, this time on a normal x86_64 Ubuntu 20.04 server. savvy is being built as a dependency of SAIGE.

It looks like in some environments, to use <cinttypes>, then __STDC_FORMAT_MACROS must be defined. I've seen this across a few projects. I don't see anywhere where this is defined in savvy. I do not encounter this problem when I'm building savvy by itself using cget build, which makes me think that there may be some configuration set somewhere I cannot find. My build environment is a bone standard Ubuntu 20.04 on a plain Intel Xeon CPU.

This could be solved by either #defining __STDC_FORMAT_MACROS above where <cinttypes> is included in the source files, or setting the config globally with add_definitions(-D__STDC_FORMAT_MACROS) in CMakeLists.txt. I'm working on a minimal reproduction of this to see if I can understand why it's working sometimes but not others.

Other projects encountering this: https://github.com/pytorch/pytorch/issues/5217 https://github.com/zdevito/ATen/issues/183

Log of the failure build failure when compiling with SAIGE:

../thirdParty/cget/include/savvy/typed_value.hpp: In member function 'void savvy::typed_value::serialize_vcf(std::size_t, char*&, char) const':
../thirdParty/cget/include/savvy/typed_value.hpp:2955:40: error: expected ')' before 'PRId64'
 2955 |       else out += std::sprintf(out, "%" PRId64, v);
      |                               ~        ^~~~~~~
      |                                        )
pettyalex commented 2 years ago

Hmm. It seems like this is potentially only happening inside of the conda environment I created for SAIGE / savvy.

jonathonl commented 2 years ago

I'm guessing that an old version of gcc (less than v4.8) is being used in your conda environment. Can you check if this is the case?

pettyalex commented 2 years ago

In conda it's building with: x86_64-conda-linux-gnu-c++ -std=gnu++11 which is x86_64-conda-linux-gnu-c++ (GCC) 9.4.0 against R 4.1.2

Outside of Conda with system packages, it's building with g++ (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0 against R 3.6.3

I'm poking around for other potential differences. we really want this in a conda environment for consistence across my group and environments.

pettyalex commented 2 years ago

The full compile call inside of conda, which fails with this error:

x86_64-conda-linux-gnu-c++ -std=gnu++11 -I"/condaenvs/envs/newsaige/lib/R/include" -DNDEBUG -I ../thirdParty/cget/include -I ../thirdParty/cget/lib -I ../thirdParty/cget/lib64 -D SQLITE_ENABLE_COLUMN_METADATA -O3 -fpic -Wall -Wextra -pedantic  -DARMA_64BIT_WORD=1 -I'/condaenvs/envs/newsaige/lib/R/library/Rcpp/include' -I'/condaenvs/envs/newsaige/lib/R/library/RcppArmadillo/include' -I'/condaenvs/envs/newsaige/lib/R/library/RcppParallel/include' -I'/condaenvs/envs/newsaige/lib/R/library/data.table/include' -I'/condaenvs/envs/newsaige/lib/R/library/SPAtest/include' -I'/condaenvs/envs/newsaige/lib/R/library/RcppEigen/include' -I'/condaenvs/envs/newsaige/lib/R/library/Matrix/include' -I'/condaenvs/envs/newsaige/lib/R/library/methods/include' -I'/condaenvs/envs/newsaige/lib/R/library/BH/include' -I'/condaenvs/envs/newsaige/lib/R/library/optparse/include' -I'/condaenvs/envs/newsaige/lib/R/library/SKAT/include' -I'/condaenvs/envs/newsaige/lib/R/library/MetaSKAT/include' -I'/condaenvs/envs/newsaige/lib/R/library/qlcMatrix/include' -I'/condaenvs/envs/newsaige/lib/R/library/RhpcBLASctl/include' -I'/condaenvs/envs/newsaige/lib/R/library/RSQLite/include' -I'/condaenvs/envs/newsaige/lib/R/library/dplyr/include' -DNDEBUG -D_FORTIFY_SOURCE=2 -O2 -isystem /condaenvs/envs/newsaige/include -I/condaenvs/envs/newsaige/include -Wl,-rpath-link,/condaenvs/envs/newsaige/lib   -fpic  -fvisibility-inlines-hidden  -fmessage-length=0 -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem /condaenvs/envs/newsaige/include -fdebug-prefix-map=/home/conda/feedstock_root/build_artifacts/r-base-split_1642327566152/work=/usr/local/src/conda/r-base-4.1.2 -fdebug-prefix-map=/condaenvs/envs/newsaige=/usr/local/src/conda-prefix  -c VCF.cpp -o VCF.o

The full compile command against system packages, which works:

g++ -std=gnu++11 -I"/usr/share/R/include" -DNDEBUG -I ../thirdParty/cget/include -I ../thirdParty/cget/lib -I ../thirdParty/cget/lib64 -D SQLITE_ENABLE_COLUMN_METADATA -O3 -fpic -Wall -Wextra -pedantic  -DARMA_64BIT_WORD=1 -I"/usr/local/lib/R/site-library/Rcpp/include" -I"/usr/local/lib/R/site-library/RcppArmadillo/include" -I"/usr/local/lib/R/site-library/RcppParallel/include" -I"/usr/lib/R/site-library/data.table/include" -I"/usr/local/lib/R/site-library/SPAtest/include" -I"/usr/local/lib/R/site-library/RcppEigen/include" -I"/usr/local/lib/R/site-library/Matrix/include" -I"/usr/lib/R/library/methods/include" -I"/usr/lib/R/site-library/BH/include" -I"/usr/local/lib/R/site-library/optparse/include" -I"/usr/local/lib/R/site-library/SKAT/include" -I"/usr/local/lib/R/site-library/MetaSKAT/include" -I"/usr/local/lib/R/site-library/qlcMatrix/include" -I"/usr/local/lib/R/site-library/RhpcBLASctl/include" -I"/usr/local/lib/R/site-library/RSQLite/include" -I"/usr/local/lib/R/site-library/dplyr/include"   -fpic  -g -O2 -fdebug-prefix-map=/build/r-base-jbaK_j/r-base-3.6.3=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g  -c VCF.cpp -o VCF.o

I'm testing different R versions to see if that could be contributing.

jonathonl commented 2 years ago

Are you aware that both Savvy and SAIGE are already on Bioconda? Or are there specific updates to SAIGE that you need but are not yet released on Bioconda? You may get some insight from the existing recipe (see https://github.com/bioconda/bioconda-recipes/blob/master/recipes/r-saige).

pettyalex commented 2 years ago

SAIGE has done a large update with a v1.0.0 (and beyond) release series with a whole set of new features. Apparently the bioconda distribution isn't planned to be updated, so it's currently being recommended to install it from source: https://saigegit.github.io//SAIGE-doc/docs/Installation_bioconda.html

pettyalex commented 2 years ago

Thanks for all this help, by the way. I really appreciate it. I know that it can be hard to get across context and nuance on Github, but my group is very excited to work with the new version of SAIGE, even though current SAIGE is pinned to a development, non-release version of Savvy and I am grateful for the troubleshooting assistance.

jonathonl commented 2 years ago

Ok, I recommend adding a conda patch which modifies the target_compile_definitions in Savvy's CMakeLists.txt to include -D__STDC_FORMAT_MACROS. I believe it should look something like target_compile_definitions(savvy INTERFACE -DSAVVY_VERSION="${PROJECT_VERSION}" -D__STDC_FORMAT_MACROS). This should be the quickest path forward for you and prevent you from having to investigate further. I intend to release a new version of Savvy on Bioconda soon. If I run into this issue, I can spend the time investigating.

pettyalex commented 2 years ago

Closing.

This was not a savvy issue at all. This can be worked around by reordering #includes so that savvy is imported before any Rcpp* headers.

The root cause is that Rcpp directly includes <inttypes.h>, and if it is included without setting __STDC_FORMAT_MACROS then it won't define these macros.

I'm going to work around this in SAIGE by reordering imports, and attempt to fix this in Rcpp by including <cinttypes> instead of <inttypes.h>