Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

with_build_tools fails on CRAN when building the prophet package #54

Closed
bgoodri opened this issue Oct 19, 2018 · 8 comments
Closed

with_build_tools fails on CRAN when building the prophet package #54

bgoodri opened this issue Oct 19, 2018 · 8 comments

Comments

@bgoodri
Copy link

bgoodri commented Oct 19, 2018

I have started using pkgbuild internally in rstan, not to build a package but to build dynamic loaded shared objects via inline::cxxfunction. This looks like it will be a big help for Windows users and it is working fine in most cases, including in many packages that depend on rstan.

However, the prophet package maintained by @seanjtaylor and @bletham is the one package that imports rstan that is failing to build on CRAN for all flavors of Windows, with

WARNING: Rtools is required to build R packages, but is not currently installed.
Please download and install Rtools 3.5 from http://cran.r-project.org/bin/windows/Rtools/.
Error: Could not find tools necessary to compile a package

  • removing 'd:/Rcompile/CRANpkg/lib/3.6/prophet'
  • restoring previous 'd:/Rcompile/CRANpkg/lib/3.6/prophet'

For the full-log see, e.g.
https://www.r-project.org/nosvn/R.check/r-devel-windows-ix86+x86_64/prophet-00install.html

The prophet package has an unusual build process that utilizes a src/install_libs.Rfile that calls rstan::stan_model
https://github.com/facebook/prophet/blob/master/R/src/install.libs.R#L14
which ultimately calls pkgbuild::with_build_tools(inline::cxxfunction(...))
https://github.com/stan-dev/rstan/blob/develop/rstan/rstan/R/cxxfunplus.R#L152

But I have no idea why with_build_tools would think RTools is not installed on the Windows server that tests R packages.

@bgoodri
Copy link
Author

bgoodri commented Oct 19, 2018

I think I have a viable workaround for the prophet case in rstan, so resolving this issue is not that urgent.

@bletham
Copy link

bletham commented Oct 23, 2018

I'm not familiar with pkgbuild and don't really know where to start at resolving this issue. What's happening in src/install_libs.R is that we ship the model as Stan code, and then during install compile the model and save the model binary. This way when the package is imported, the model can be used immediately without having to wait for compile every time.

src/install_libs.R is run in a separate environment, but it does still seem odd that that environment wouldn't have access to Rtools.

Is your workaround something we can easily adjust in our build process?

@bgoodri
Copy link
Author

bgoodri commented Oct 24, 2018 via email

@bgoodri
Copy link
Author

bgoodri commented Nov 16, 2018

Following up, the aforementioned workaround does allow prophet to install on CRAN's Windows servers, but there is still a warning. The issue seems to be related to multiarch

*** arch - i386
make[1]: Entering directory `/cygdrive/d/temp/RtmpEHHdpj/R.INSTALL4658362f674e/prophet/src-i386'
make[1]: Leaving directory `/cygdrive/d/temp/RtmpEHHdpj/R.INSTALL4658362f674e/prophet/src-i386'
make[1]: Entering directory `/cygdrive/d/temp/RtmpEHHdpj/R.INSTALL4658362f674e/prophet/src-i386'
make[1]: Leaving directory `/cygdrive/d/temp/RtmpEHHdpj/R.INSTALL4658362f674e/prophet/src-i386'
installing via 'install.libs.R' to d:/Rcompile/CRANpkg/lib/3.6/prophet
Compiling model (this will take a minute...)
Writing model to: d:/Rcompile/CRANpkg/lib/3.6/prophet/libs/i386
Compiling using binary: D:/RCompile/recent/R/bin/x64
Registered S3 methods overwritten by 'ggplot2':
  method         from 
  [.quosures     rlang
  c.quosures     rlang
  print.quosures rlang
WARNING: Rtools is required to build R packages, but is not currently installed.

Please download and install Rtools 3.5 from http://cran.r-project.org/bin/windows/Rtools/.
Scanning R CMD config CC...
cc_path: d:/Compiler/gcc-4.9.3/mingw_64/bin/gcc 
  Architecture doesn't match
Scanning path...
ls: D:\compiler\bin\ls.exe 
gcc_path:  
'D:/compiler/mingw_32/bin/gcc.exe.exe' does not exist
Scanning registry...
WARNING: Rtools is required to build R packages, but is not currently installed.

Full log:
https://www.r-project.org/nosvn/R.check/r-devel-windows-ix86+x86_64/prophet-00install.html

@seanjtaylor
Copy link

We're about to submit Prophet 0.4 to CRAN. I'm going to link to this issue in the submission comments and hopefully that satisfies Uwe.

@seanjtaylor
Copy link

Just a heads up that we have been unable to satisfy the CRAN maintainers with the WARNING on windows due to this issue. We currently can't update the Prophet package on CRAN. We're switching our docs to recommend using devtools for installation for the time being.

jimhester added a commit to jimhester/prophet that referenced this issue Dec 19, 2018
@jimhester
Copy link
Member

jimhester commented Dec 19, 2018

The issue is largely stemming because you are trying to compile from within install.iibs.R

Notably this function is run from the 64 bit version of R for both architectures on Windows, but the R_ARCH environment variable is changed when installing the 32 bit libraries, so pkgbuild:::gcc_arch() returns 32 and that does not match what R CMD config CC returns, so the toolchain is not found.

Options to remedy this are

  1. Do the stan compilation elsewhere than install.libs.R so you don't run into this behavior. You could call a R script from the Makevars file for instance.
  2. The 'WARNING' text is actually coming from a message(), and the compilation works fine even with the messages, so adding suppressMessages() around the rstan calls will work as well.

The first option is probably the cleanest, but the second is simpler to implement. I checked this approach with win-builder and it fixes the WARNING you have observed, although there seem to be additional test failures unrelated to this problem. I opened a PR with this approach facebook/prophet#776

@seanjtaylor
Copy link

Thanks @jimhester this is super helpful. I really appreciate you looking into this. We'll use (1) for now and move to (2) in our next release. I think we always knew install.libs.R wasn't quite the right way to do this so it's good to have a recommendation for how to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants