Skip to content

Disable OpenMP on macOS + Vignette Fix #168

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Disable OpenMP on macOS + Vignette Fix #168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
22ae851
Block macOS system files from entering repo
coatless Aug 26, 2017
be50006
Determine the appropriate OpenMP flags in configure.ac
coatless Aug 26, 2017
6921ff5
Fix for the vignette engine hiccup
coatless Aug 26, 2017
7272da3
Add gitignore for vignette files...
coatless Aug 26, 2017
4627019
Adds highlight dependency in travis
coatless Aug 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
.Rproj.user
.Rhistory
.RData
.DS_Store
src/*.o
src/*.so
src/*.dll
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comingles unrelated issues.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- ./run.sh install_r reticulate

script:
Expand Down
2 changes: 1 addition & 1 deletion DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@coatless coatless Aug 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

😆

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

URL: http://dirk.eddelbuettel.com/code/rcpp.armadillo.html
BugReports: https://github.com/RcppCore/RcppArmadillo/issues
4 changes: 2 additions & 2 deletions R/inline.R → R/inline.R.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)",
Copy link
Contributor

@kevinushey kevinushey Aug 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

package = "RcppArmadillo"
)
settings <- plugin()
settings$env$PKG_CPPFLAGS <- "-I../inst/include $(SHLIB_OPENMP_CXXFLAGS)"
settings$env$PKG_CPPFLAGS <- "-I../inst/include @HAVE_OPENMP_R_CXXFLAG@"
settings
}

4 changes: 3 additions & 1 deletion cleanup
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ rm -f config.log config.status confdefs.h \
vignettes/*~ \
*/*~ *~ \
inst/include/RcppArmadilloLapack.h \
inst/include/RcppArmadilloConfigGenerated.h
inst/include/RcppArmadilloConfigGenerated.h \
src/Makevars \
R/inline.R

rm -rf autom4te.cache/ \
vignettes/auto/
Loading