knausb / vcfR

Tools to work with variant call format files
248 stars 54 forks source link

Undefined symbol error at compilation #75

Closed knausb closed 7 years ago

knausb commented 7 years ago

At some point vcfR stopped compiling cleanly. It appears to have something to do with the build workflow. Typically I use RStudio because it does 'magic' which is great, until the magic breaks. So now I'm going through the workflow to learn some of the things RStudio manages for me to see if I can fix the magic.

First, we appear to still work using the standard CRAN workflow.

wget https://github.com/knausb/vcfR/archive/master.zip
unzip master.zip
R CMD build vcfR-master/
R
R> install.packages("vcfR_1.5.0.9000.tar.gz", repos = NULL, type="source")

Works fine.

The RStudio workflow uses devtools::build() to compile the package. This is different from above. The devtools::build() function appears to have a non-exported function compile_rcpp_attributes() that calls Rcpp::compileAttributes(). If we add this to our workflow it creates problems.

cd vcfR-master/
R
R> Rcpp::compileAttributes()
R> q()
cd ..
R CMD build vcfR-master/

Now generates an error.

Error: package or namespace load failed for ‘vcfR’ in dyn.load(file, DLLpath = DLLpath, ...):
 unable to load shared object '/tmp/RtmpGjcysL/Rinst19bf28e29923/vcfR/libs/vcfR.so':
  /tmp/RtmpGjcysL/Rinst19bf28e29923/vcfR/libs/vcfR.so: undefined symbol: vcfR_AD_frequency
Error: loading failed
Execution halted
ERROR: loading failed

This is the same error I receive when using devtools::build().

cd vcfR-master/
R
R> devtools::build()

Generates the following error.

Error: package or namespace load failed for ‘vcfR’ in dyn.load(file, DLLpath = DLLpath, ...):
 unable to load shared object '/tmp/RtmpyL991v/Rinst1be047a02362/vcfR/libs/vcfR.so':
  /tmp/RtmpyL991v/Rinst1be047a02362/vcfR/libs/vcfR.so: undefined symbol: vcfR_AD_frequency
Error: loading failed
Execution halted
ERROR: loading failed
* removing ‘/tmp/RtmpyL991v/Rinst1be047a02362/vcfR’
      -----------------------------------
ERROR: package installation failed
Error: Command failed (1)

I think I need to diff src/RcppExports.cpp with versions of src/RcppExports.cpp from before and after Rcpp::compileAttributes() to find the changes.

knausb commented 7 years ago

Found: RcppCore/Rcpp#733 which appears relevant. Although using Rcpp::compileAttributes() twice does not appear to solve my problem.

knausb commented 7 years ago

Using the GitHub version of Rcpp.

R
R> devtools::install_github("RcppCore/Rcpp")
R> q()
R -e 'Rcpp::compileAttributes()'
cd ..
R CMD build vcfR-master/

resulted in

Error: package or namespace load failed for ‘vcfR’ in dyn.load(file, DLLpath = DLLpath, ...):
 unable to load shared object '/tmp/RtmpOfLDOT/Rinst1f512f2d461a/vcfR/libs/vcfR.so':
  /tmp/RtmpOfLDOT/Rinst1f512f2d461a/vcfR/libs/vcfR.so: undefined symbol: vcfR_AD_frequency
Error: loading failed
Execution halted
ERROR: loading failed
* removing ‘/tmp/RtmpOfLDOT/Rinst1f512f2d461a/vcfR’
      -----------------------------------
ERROR: package installation failed

And running Rcpp::compileAttributes() twice does not resolve the issue. I'll revert to the production version of Rcpp (currently Rcpp_0.12.12).

knausb commented 7 years ago

If I use knausb/GeneticDiff and rm the RcppExports files it builds clean with devtools::build() and other tests. This builds an argument that this is a vcfR matter.

knausb commented 7 years ago

In https://github.com/RcppCore/Rcpp/issues/733#issuecomment-320780944 it is pointed out that mixed calls using .Call() with the Rcpp attributes is a bad idea. I'm guilty of this as an artefact of learning to use Rcpp. I think this means that vcfR is due for a code review.

knausb commented 7 years ago

And it has now thrown an error on Travis https://github.com/knausb/vcfR/commit/582eb9a085cf79b1b970af3f02389bf4dbab2421. Although, not on the master branch.

knausb commented 7 years ago

I've removed all .Call() statements and now either call the function from its RcppExports name or have made the functions invisible. THis is now on the branch rev17b bc750395ffafdaea8030576db2e5f9dcb985a31a. Hoping to get this merged to master soon, but for now I'll call it fixed.