search

renozao / RcppOctave

Seamless Interface to Octave -- and Matlab code
18 stars 9 forks source link

Octave 4.3.0+ compatibility update #17

Closed git-steb closed 7 years ago

git-steb commented 7 years ago 
- Addressing build error: ‘OCTINTERP_API’ does not name a type
- inserted #include <octave/config.h> in swig_octave_version.h
git-steb commented 7 years ago

Hello @renozao,

With this branch it is now possible to install RcppOctave for a current octave version using R -e "library('devtools'); devtools::install_github('git-steb/RcppOctave',ref='develop')"

It took a bit of effort to get Octave 4.3.0+ to work with R. Issues that have been addressed:

I'm offering this pull request for usage in the package you maintain. I have not looked into the Travis CI failures, as I believe they were in the develop branch before. Let me know, if you have any questions or further requests about this.

Cheers, Steven

renozao commented 7 years ago

Thank you very much for this! Have you tested this on Octave versions prior 4.3 as well?

Re-travis, I updated the travis config file in develop, maybe rebase and push your branch to see if the check now runs correctly, since it is now failing on the OCTINTERP_API issue in Octave 3.8.1 (current version on Travis).

I am getting errors on 4.2:

g++ -std=gnu++11 -I/usr/share/R/include -DNDEBUG `"/usr/lib/R/bin/Rscript" -e "Rcpp:::CxxFlags()"` -I/usr/local/include/octave-4.2.0/octave/.. -I/usr/local/include/octave-4.2.0/octave -I/usr/local/include -DOCT_POST_3_4_0=1  -I"/home/renaud/R/x86_64-pc-linux-gnu-library/3.4/Rcpp/include"    -fpic  -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g -c rcpp_octave.cpp -o rcpp_octave.o
rcpp_octave.cpp:130:16: error: conflicting declaration ‘typedef void octave::application’
   typedef void application;
                ^
In file included from rcpp_octave.cpp:45:0:
/usr/local/include/octave-4.2.0/octave/../octave/octave.h:183:23: note: previous declaration as ‘class octave::application’
   class OCTINTERP_API application
                       ^
/usr/lib/R/etc/Makeconf:168: recipe for target 'rcpp_octave.o' failed
make: *** [rcpp_octave.o] Error 1

I would like to keep the package as much backward compatible as possible.

renozao commented 7 years ago

Thanks a lot again. Will look into the PR and check the package with it.

Couple of comments:

git-steb commented 7 years ago

I also left comments in the related places of the proposed commits.

git-steb commented 7 years ago

I've edited the commit log for my fork of this branch, simply putting your changes before mine, to make merging easier. You may have to re-pull the branch.

With how my origin was set up, I was able to proceed using

git fetch
git reset origin/develop --hard
git-steb commented 7 years ago

The Octave v3.8.1 build succeeds as of the last update, where I've added output of error messages, whose wording fails to match in the test, so that the test can be updated via copy & paste from the Travis bot output.

Build setup for v4.2 on Travis is not working, yet: The command "eval sudo apt-get install -y liboctave4.2-dev " failed 3 times.

Octave v4.0.0 installs on the bot, but when running R CMD build . it fails:

[...]
Saving output to ‘/tmp/Rtmp0re3UR/Rbuild4fb75cf2a45/RcppOctave/build/RcppOctave.pdf’ ...
Done
* creating vignettes ... ERROR
Quitting from lines 322-326 (RcppOctave.Rnw) 
Error: processing vignette 'RcppOctave.Rnw' failed with diagnostics:
RcppOctave - error in Octave function `exist`:
  caught execution error in library function
Execution halted

This is for a back-ported build of octave v4.0.0 on Ubuntu precise on Travis.

Locally on Ubuntu Xenial, I was able to downgrade to a similar version using apt install octave=4.0.0-3ubuntu9.1 for octave and related packages. However, I cannot reproduce the issue - the build succeeds just fine, including v4.0.0, v4.0.2, v.4.2.1, v4.3.0+.

Do you have any hints on how to move forward towards finishing off this PR?

git-steb commented 7 years ago

@renozao,

With the commits leading up to 2e0fb08 I've revised the Travis-CI setup to get the build to pass for all relevant versions of Octave. The source code and tests were fine already, but there seemed to be some incompatibility of the libraries on the build bot that kept failing the build. The new setup is using layered Docker images, where the base image is served from https://hub.docker.com/r/docsteb/trusty-r-texlive built from RcppOctave/dockerfiles/trusty-r-texlive. (It would be nice to make this base image smaller by installing only the required latex packages and fonts.) On top of this image, the Octave version specific dockerfiles finalize the setup by adding the repo checkout and RcppOctave specific R dependencies.

Overall, I'm quite happy about the new setup, because it enables people to locally use the exact same environment as the build bots.

A remaining minor issue - the initialization of the embedded interpreter in src/rcpp_octave.cpp could be improved using the hints that John and Mike gave in the related bug report I filed at https://savannah.gnu.org/bugs/index.php?50974

I've already updated master in my fork of this repo. I hope you still find the PR here to be a useful contribution towards the maintenance of RcppOctave.

vermouthmjl commented 7 years ago

I was able to install the package with Octave 4.2.1. Thank you both so much for your work!

renozao commented 7 years ago

Thank you very much for all this work, notably the travis dockers.

Will review the PR and merge it in. Note that I will most probably revert all changes made on the documentation and NAMESPACE due to discrepancy with the custom roxygen version I use.

git-steb commented 7 years ago

Sounds good. You could delete 96980f7 to revert the auto-generated documentation changes, before applying your method.

Also, if you have an account on https://hub.docker.com, I can give you write permissions to the container image that is used for the builds, in case it needs to change in the future.

renozao commented 7 years ago

I merged the compatibility fixes into develop. I left the Dockerfiles out for now, as they appear to only build the package, but do not check it. Or I missed something. It would be great to get the check in stock R environment to work. I am getting other types of errors now, related to standard libraries not being found. Weird.

git-steb commented 7 years ago

Thanks for moving this forward. You might be able to resolve the missing standard libraries with some of the setup you find in dockerfiles/octave4.2/Dockerfile. I abandoned the stock R setup, because it seemed very difficult to get all three versions of octave to work, but you might be more successful.

There were errors in the recent builds regarding 'qlibrary' not being an exported object from 'namespace:pkgmaker', e.g. see https://travis-ci.org/renozao/RcppOctave/jobs/246099992 - for now resolved by removing this import in two places. Also, I finally realized that Octave returns error messages in the output stream rather than the execution_exception variable and noticed that you've resolved this problem now yourself.

This PR has a number of commits that GitHub keeps a count of, I wonder if it is possible to preserve most of them in the merge and simply commit improving changes on top. Good luck!

renozao commented 7 years ago

I managed to get stock R Travis working, by adding some missing libraries. I think there has been some changes on Travis trusty images, which although missing those libraries somehow resolved the libgraphicsmagick conflict.

All has been pushed to both branches develop and master. Thanks again for your contribution.