Disable OpenMP on macOS + Vignette Fix #168

coatless · 2017-08-26T23:50:59Z

No description provided.

kevinushey · 2017-08-27T05:05:37Z

R/inline.R.in

@@ -19,11 +19,11 @@ inlineCxxPlugin <- function(...) {
    plugin <-
        Rcpp::Rcpp.plugin.maker(
                   include.before = "#include <RcppArmadillo.h>",
-                   libs           = "$(SHLIB_OPENMP_CFLAGS) $(LAPACK_LIBS) $(BLAS_LIBS) $(FLIBS)",
+                   libs           = "@HAVE_OPENMP_R_CFLAG@ $(LAPACK_LIBS) $(BLAS_LIBS) $(FLIBS)",


Making this a package configure-time check doesn't feel right to me, IMHO. This implies that a package built on CRAN might detect OpenMP support, and that assumption will remain in place when the Rcpp binary is later served to the user. However, that assumption will fail if the user has not installed the CRAN LLVM toolchain.

I would prefer that we just checked this at runtime, rather than configure time.

Yes, but how do you suggest we go about it?

I started down the road of 'grep'ing CXXFLAGS and looking for -fopenmp. As you know that fails on macOS.

No matter what, we do need a compile time check to set the src/Makevars appropriately. This was evident with the clang-sans-openmp check Ripley is running. Regarding the build check, I do agree that would be beneficial; however, there really isn't a good way to detect OpenMP on macOS reliably as support for OpenMP is not listed within capabilities().

My only thought to that line of detection is the possibility of checking for the directory existence of /usr/local/clang4 on macOS. Other platforms have a tendency to respect setting -- or unsetting -- SHLIB_OPENMP_* where appropriate.

eddelbuettel · 2017-08-27T12:42:57Z

.travis.yml

@@ -13,7 +13,7 @@ before_install:
  - ./run.sh bootstrap

 install:
-  - ./run.sh install_aptget  r-cran-rcpp r-cran-runit r-cran-matrix r-cran-pkgkitten r-cran-jsonlite python-scipy
+  - ./run.sh install_aptget  r-cran-rcpp r-cran-runit r-cran-matrix r-cran-pkgkitten r-cran-jsonlite python-scipy r-cran-highlight


This comingles unrelated issues.

All Suggests are skipped by default. This worked (for me) for years.

Ah, wait, maybe this is different as we didn't use highlight before -- because we also forgot it as vignette engine.

eddelbuettel · 2017-08-27T12:49:48Z

DESCRIPTION

@@ -23,6 +23,6 @@ License: GPL (>= 2)
 LazyLoad: yes
 LinkingTo: Rcpp
 Imports: Rcpp (>= 0.11.0), stats, utils, methods
-Suggests: RUnit, Matrix, pkgKitten, reticulate
+Suggests: RUnit, Matrix, pkgKitten, reticulate, highlight


That is you deciding you need to throw this in.

As I said yesterday, it does not throw a warning or note for me when checking, nor did CRAN comment on it. As such it is not clearly needed.

It surely does not belong into a hotfix-PR about OpenMP.

This is to address the build error happening on Ripley's machine. When we discussed this, the conclusion was to add it alongside of the OpenMP fix to save a PR.

I guess the slogan should be:

No PR shalt be spared!

😆

I doubt that. When we talked about the BDR failure we concluded it was a segfault neither one of us understood or was able to replicate.

In any event, the two commits were right and good, and I took it as opportunity to learn some more magit and cherry-picked them into a distinct PR which by now is in master. So the vignette issue is taken care of.

Leaves

the macOS clusterfuck, and the valid point by @kevinushey

the unexplicable fail on clang-on-fedora which I plan to simply ignore unless someone shows how to replicate

kevinushey · 2017-08-27T21:19:15Z

Re: detecting OpenMP support at runtime, I think we could just try (on macOS only, probably) calling R CMD SHLIB on some (empty, or dummy) C++ file:

R CMD SHLIB test.cpp

If that fails, we could detect whether OpenMP is the culprit based on the compiler output:

kevin@cdrv:~/scratch
$ R CMD SHLIB test.cpp
clang++  -I/Library/Frameworks/R.framework/Resources/include -DNDEBUG   -I/usr/local/include   -fPIC  -fopenmp -c test.cpp -o test.o
clang: error: unsupported option '-fopenmp'
make: *** [test.o] Error 1

And then override the necessary environment variables, e.g. by unsetting these environment variables (and possibly others)

SHLIB_OPENMP_CFLAGS   
SHLIB_OPENMP_CXXFLAGS 
SHLIB_OPENMP_FCFLAGS  
SHLIB_OPENMP_FFLAGS   
MAIN_LDFLAGS

Alternatively, we just document this behavior for macOS users and say "sorry, here's how you can work around it".

eddelbuettel · 2017-08-27T21:27:13Z

In essence, the true configure tests for OpenMP do just that.

We could be super-clever, launch an environment on package startup and tests once per session (on Darwin / macOS) and then re-use that in inline.R. That would bring that part back to run-time.

But we do need a compile-time / installation time switch as well. Or, simpler still, we just Eff It and turn OpenMP off on macOS (and document for the four motivated and capable users how to locally override).

eddelbuettel · 2017-08-27T21:28:09Z

But all this is just so ... fragile. So my preference (right now) is to just turn it off. Sorry, macOS.

kevinushey · 2017-08-27T21:31:02Z

I'm fine with just turning it off. Enterprising users can do the work to re-enable it and install the custom LLVM toolchain if they so desire.

coatless · 2017-08-27T21:42:32Z

At this stage, I'm more of a fan of just disabling OpenMP on macOS. The alternative options:

a compile check
within .onLoad()

would be either add additional (read: unnecessary) delay to compiling or impact the load time of the package for macOS users. Needless to say, it would also introduce a new point of failure.

Furthermore, due to the procedure of configure.ac writing in RcppArmadilloConfigGenerated.h.in a statement to completely disable OpenMP for Armadillo, even doing the compile time check would fail unless that is also written dynamically... As I've found out, modifying a system file without explicitly obtaining consent from a user is a big no-no for CRAN.

Having said this, the implication of the #define ARMA_DONT_USE_OPENMP 1 also takes out the ability for Rcpp attributes to exploit:

// [[Rcpp::plugins(openmp)]]

So, really the best solution is just to ship a disabled copy for macOS until the toolchain is appropriately detectable.

eddelbuettel · 2017-08-27T22:01:40Z

It think that is the right approach.

I just carried the first half of your tests over in a (much simpler) version. The core chunk now reads:

## Check for broken systems produced by a corporation based in Cupertino
AC_MSG_CHECKING([for macOS])
RSysinfoName=$("${R_HOME}/bin/Rscript" --vanilla -e 'cat(Sys.info()[["sysname"]])')
if test x"${RSysinfoName}" == x"Darwin"; then
   AC_MSG_RESULT([found])
   AC_MSG_WARN([OpenMP unavailable and turned off.])
   have_openmp="#define ARMA_DONT_USE_OPENMP 1"
else
   AC_MSG_RESULT([not found as on ${RSysinfoName}])
   ## Check for OpenMP
   AC_MSG_CHECKING([for OpenMP])
   ## if R has -fopenmp we should be good
   allldflags=$(${R_HOME}/bin/R CMD config --ldflags)
   hasOpenMP=$(echo ${allldflags} | grep -- -fopenmp)
   if test x"${hasOpenMP}" == x""; then
      AC_MSG_RESULT([missing])
      have_openmp="#define ARMA_DONT_USE_OPENMP 1"
   else   
      AC_MSG_RESULT([found])
      have_openmp="#define ARMA_USE_OPENMP 1"
   fi
fi

I still need to put in the change you worked out to 'blank' the use of $(SHLIB_OPENMP_CXXFLAGS) in src/Makevars, making this a src/Makevars.in.

In R/inline.R it is simpler as we can rely on Sys.info[["sysname"]] as well.

Hopefully that'll do.

eddelbuettel · 2017-08-27T22:46:27Z

With apologies to @coatless for trampling over / ignoring this PR (off which I cherry-picked two commits though) I have a new branch that does the castration of OpenMP under macOS.

Can you fancy people with macOS machines give it a spin? It is here and it "works for me" but then I don't have the exqusite machinery it is aiming at. No hurry, we've been at this long enough.

coatless · 2017-08-27T23:38:18Z

Works! Killing this. Please PR the other.

> devtools::install_github("rcppcore/rcpparmadillo", ref="feature/openmp_off_on_macos")
Downloading GitHub repo rcppcore/rcpparmadillo@feature/openmp_off_on_macos
from URL https://api.github.com/repos/rcppcore/rcpparmadillo/zipball/feature/openmp_off_on_macos
Installing RcppArmadillo
'/Library/Frameworks/R.framework/Resources/bin/R' --no-site-file --no-environ --no-save --no-restore --quiet  \
  CMD INSTALL  \
  '/private/var/folders/b0/vt_1hj2d6yd8myx9lwh81pww0000gn/T/Rtmpbsz2gq/devtools1757a2cd66c3c/RcppCore-RcppArmadillo-793e45b'  \
  --library='/Library/Frameworks/R.framework/Versions/3.4/Resources/library' --install-tests 

* installing *source* package ‘RcppArmadillo’ ...
checking whether the C++ compiler works... yes
checking for C++ compiler default output file name... a.out
checking for suffix of executables... 
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C++ compiler... yes
checking whether /usr/local/clang4/bin/clang++ accepts -g... yes
checking how to run the C++ preprocessor... /usr/local/clang4/bin/clang++ -E
checking whether we are using the GNU C++ compiler... (cached) yes
checking whether /usr/local/clang4/bin/clang++ accepts -g... (cached) yes
checking whether g++ version is sufficient... checking LAPACK_LIBS... fallback LAPACK from R 3.3.0 or later used
checking for macOS... found
configure: WARNING: OpenMP unavailable and turned off.
configure: creating ./config.status
config.status: creating inst/include/RcppArmadilloConfigGenerated.h
config.status: creating src/Makevars
** libs
/usr/local/clang4/bin/clang++ -std=gnu++11 -I/Library/Frameworks/R.framework/Resources/include -DNDEBUG  -I"/Library/Frameworks/R.framework/Versions/3.4/Resources/library/Rcpp/include" -I/usr/local/include  -I../inst/include  -fPIC  -Wall -g -O2 -c RcppArmadillo.cpp -o RcppArmadillo.o
/usr/local/clang4/bin/clang++ -std=gnu++11 -I/Library/Frameworks/R.framework/Resources/include -DNDEBUG  -I"/Library/Frameworks/R.framework/Versions/3.4/Resources/library/Rcpp/include" -I/usr/local/include  -I../inst/include  -fPIC  -Wall -g -O2 -c RcppExports.cpp -o RcppExports.o
/usr/local/clang4/bin/clang++ -std=gnu++11 -I/Library/Frameworks/R.framework/Resources/include -DNDEBUG  -I"/Library/Frameworks/R.framework/Versions/3.4/Resources/library/Rcpp/include" -I/usr/local/include  -I../inst/include  -fPIC  -Wall -g -O2 -c fastLm.cpp -o fastLm.o
/usr/local/clang4/bin/clang++ -std=gnu++11 -dynamiclib -Wl,-headerpad_max_install_names -undefined dynamic_lookup -single_module -multiply_defined suppress -L/Library/Frameworks/R.framework/Resources/lib -L/usr/local/clang4/lib -o RcppArmadillo.so RcppArmadillo.o RcppExports.o fastLm.o -L/Library/Frameworks/R.framework/Resources/lib -lRlapack -L/Library/Frameworks/R.framework/Resources/lib -lRblas -L/usr/local/gfortran/lib/gcc/x86_64-apple-darwin15/6.1.0 -L/usr/local/gfortran/lib -lgfortran -lquadmath -lm -F/Library/Frameworks/R.framework/.. -framework R -Wl,-framework -Wl,CoreFoundation
ld: warning: directory not found for option '-L/usr/local/gfortran/lib/gcc/x86_64-apple-darwin15/6.1.0'
installing to /Library/Frameworks/R.framework/Versions/3.4/Resources/library/RcppArmadillo/libs
** R
** inst
** tests
** preparing package for lazy loading
** help
*** installing help indices
** building package indices
** installing vignettes
** testing if installed package can be loaded
* DONE (RcppArmadillo)

eddelbuettel · 2017-08-28T00:04:43Z

What about

ld: warning: directory not found for option '-L/usr/local/gfortran/lib/gcc/x86_64-apple-darwin15/6.1.0'

coatless added 5 commits August 26, 2017 16:59

Block macOS system files from entering repo

22ae851

Determine the appropriate OpenMP flags in configure.ac

be50006

Fix for the vignette engine hiccup

6921ff5

Add gitignore for vignette files...

7272da3

Adds highlight dependency in travis

4627019

kevinushey reviewed Aug 27, 2017

View reviewed changes

eddelbuettel reviewed Aug 27, 2017

View reviewed changes

eddelbuettel mentioned this pull request Aug 27, 2017

Feature/vignette engine #169

Merged

coatless closed this Aug 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable OpenMP on macOS + Vignette Fix #168

Disable OpenMP on macOS + Vignette Fix #168

coatless commented Aug 26, 2017

kevinushey Aug 27, 2017 •

edited

Loading

eddelbuettel Aug 27, 2017

coatless Aug 27, 2017

eddelbuettel Aug 27, 2017

eddelbuettel Aug 27, 2017

eddelbuettel Aug 27, 2017

coatless Aug 27, 2017 •

edited

Loading

eddelbuettel Aug 27, 2017

kevinushey commented Aug 27, 2017

eddelbuettel commented Aug 27, 2017 •

edited

Loading

eddelbuettel commented Aug 27, 2017

kevinushey commented Aug 27, 2017

coatless commented Aug 27, 2017

eddelbuettel commented Aug 27, 2017 •

edited

Loading

eddelbuettel commented Aug 27, 2017

coatless commented Aug 27, 2017

eddelbuettel commented Aug 28, 2017

Disable OpenMP on macOS + Vignette Fix #168

Disable OpenMP on macOS + Vignette Fix #168

Conversation

coatless commented Aug 26, 2017

kevinushey Aug 27, 2017 • edited Loading

Choose a reason for hiding this comment

eddelbuettel Aug 27, 2017

Choose a reason for hiding this comment

coatless Aug 27, 2017

Choose a reason for hiding this comment

eddelbuettel Aug 27, 2017

Choose a reason for hiding this comment

eddelbuettel Aug 27, 2017

Choose a reason for hiding this comment

eddelbuettel Aug 27, 2017

Choose a reason for hiding this comment

coatless Aug 27, 2017 • edited Loading

Choose a reason for hiding this comment

eddelbuettel Aug 27, 2017

Choose a reason for hiding this comment

kevinushey commented Aug 27, 2017

eddelbuettel commented Aug 27, 2017 • edited Loading

eddelbuettel commented Aug 27, 2017

kevinushey commented Aug 27, 2017

coatless commented Aug 27, 2017

eddelbuettel commented Aug 27, 2017 • edited Loading

eddelbuettel commented Aug 27, 2017

coatless commented Aug 27, 2017

eddelbuettel commented Aug 28, 2017

kevinushey Aug 27, 2017 •

edited

Loading

coatless Aug 27, 2017 •

edited

Loading

eddelbuettel commented Aug 27, 2017 •

edited

Loading

eddelbuettel commented Aug 27, 2017 •

edited

Loading