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

Loading cpp11 has no effect with 3.4.0? #683

Closed
epipping opened this Issue Apr 25, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@epipping
Contributor

epipping commented Apr 25, 2017

Please consider the following sample code:

#include <Rcpp.h>

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

// [[Rcpp::export]]                                                                       
double myhypot(double x, double y) {
  return std::hypot(x,y);
}

I believe with R 3.3.3, this worked as expected: The function std::hypot is new with C++11 but loading the cpp11 plugin made it available.

Now I'm on R 3.4.0 and the code does not work anymore. Instead, here's what I get:

> Rcpp::sourceCpp('foo.cc')
foo.cc: In function ‘double myhypot(double, double)’:
foo.cc:7:10: error: ‘hypot’ is not a member of ‘std’
   return std::hypot(x,y);
          ^
foo.cc:7:10: note: suggested alternative:
In file included from /usr/include/features.h:374:0,
                 from /usr/include/x86_64-linux-gnu/c++/4.9/bits/os_defines.h:39,
                 from /usr/include/x86_64-linux-gnu/c++/4.9/bits/c++config.h:430,
                 from /usr/include/c++/4.9/cmath:41,
                 from /home/mi/pipping/dune/inst/R-3.4.0/lib/R/library/Rcpp/include/Rcpp/platform/compiler.h:100,
                 from /home/mi/pipping/dune/inst/R-3.4.0/lib/R/library/Rcpp/include/Rcpp/r/headers.h:48,
                 from /home/mi/pipping/dune/inst/R-3.4.0/lib/R/library/Rcpp/include/RcppCommon.h:29,
                 from /home/mi/pipping/dune/inst/R-3.4.0/lib/R/library/Rcpp/include/Rcpp.h:27,
                 from foo.cc:1:
/usr/include/x86_64-linux-gnu/bits/mathcalls.h:162:1: note:   ‘hypot’
 __MATHCALL (hypot,, (_Mdouble_ __x, _Mdouble_ __y));
 ^
make: *** [foo.o] Error 1
g++  -I/home/mi/pipping/dune/inst/R-3.4.0/lib/R/include -DNDEBUG   -I"/home/mi/pipping/dune/inst/R-3.4.0/lib/R/library/Rcpp/include" -I"/home/mi/pipping" -I/usr/local/include   -fpic  -g -O2  -c foo.cc -o foo.o
/home/mi/pipping/dune/inst/R-3.4.0/lib/R/etc/Makeconf:166: recipe for target 'foo.o' failed
Error in Rcpp::sourceCpp("foo.cc") : 
  Error 1 occurred building shared library.
> 

In other words, the function std::hypot is not found. There could be many reasons for that. My first reaction was that I need to include <cmath> now. But that doesn't make the error go away. Instead, what does make the error go away is loading cpp14 instead of cpp11. What also makes the error go away is manually setting

Sys.setenv("PKG_CXXFLAGS"="-std=c++11")

So my impression is that Rcpp somehow things my C++ compiler is already in C++11 mode by default so that the cpp11 plugin needn't do anything. But that's not so.

This is the compiler that I'm using (provided by the g++ package on debian 8.7):

% g++ --version
g++ (Debian 4.9.2-10) 4.9.2
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Finally, my session info:

> sessionInfo()
R version 3.4.0 (2017-04-21)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Debian GNU/Linux 8 (jessie)

Matrix products: default
BLAS: /home/mi/pipping/dune/inst/R-3.4.0/lib/R/lib/libRblas.so
LAPACK: /home/mi/pipping/dune/inst/R-3.4.0/lib/R/lib/libRlapack.so

locale:
 [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C              
 [3] LC_TIME=en_US.UTF-8        LC_COLLATE=en_US.UTF-8    
 [5] LC_MONETARY=en_US.UTF-8    LC_MESSAGES=en_US.UTF-8   
 [7] LC_PAPER=en_US.UTF-8       LC_NAME=C                 
 [9] LC_ADDRESS=C               LC_TELEPHONE=C            
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

loaded via a namespace (and not attached):
[1] compiler_3.4.0
> 

Update: It could not reproduce this issue on macOS 10.12, only linux; I would assume that /usr/bin/c++ indeed defaults to C++11 mode there already.

@kevinushey

This comment has been minimized.

Show comment
Hide comment
@kevinushey

kevinushey Apr 25, 2017

Contributor

Here's how the cpp11 plugin is defined:

> Rcpp:::.plugins$cpp11
function ()
{
    if (getRversion() >= "3.1")
        list(env = list(USE_CXX1X = "yes"))
    else if (.Platform$OS.type == "windows")
        list(env = list(PKG_CXXFLAGS = "-std=c++0x"))
    else list(env = list(PKG_CXXFLAGS = "-std=c++11"))
}

We probably need to change that to USE_CXX11 rather than USE_CXX1X for compatibility with R 3.4.0.

Contributor

kevinushey commented Apr 25, 2017

Here's how the cpp11 plugin is defined:

> Rcpp:::.plugins$cpp11
function ()
{
    if (getRversion() >= "3.1")
        list(env = list(USE_CXX1X = "yes"))
    else if (.Platform$OS.type == "windows")
        list(env = list(PKG_CXXFLAGS = "-std=c++0x"))
    else list(env = list(PKG_CXXFLAGS = "-std=c++11"))
}

We probably need to change that to USE_CXX11 rather than USE_CXX1X for compatibility with R 3.4.0.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 25, 2017

Member

From a first casual glance I think I can confirm it. It does seem to affect the outcome for me as my default compiler is g++ 6.2.* which defaults to C++11. And I set CXX=g++-4.9 then it does indeed fail as the C++11 flag is not added.

Member

eddelbuettel commented Apr 25, 2017

From a first casual glance I think I can confirm it. It does seem to affect the outcome for me as my default compiler is g++ 6.2.* which defaults to C++11. And I set CXX=g++-4.9 then it does indeed fail as the C++11 flag is not added.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 25, 2017

Member

Spot on, @kevinushey. I recall a recent email by Martyn Plummer about this and had some unease about side-effects. This may be one.

Member

eddelbuettel commented Apr 25, 2017

Spot on, @kevinushey. I recall a recent email by Martyn Plummer about this and had some unease about side-effects. This may be one.

@epipping

This comment has been minimized.

Show comment
Hide comment
@epipping

epipping Apr 25, 2017

Contributor

@kevinushey I can confirm that

Sys.setenv("USE_CXX1X" = "yes")

has no effect on the compilation while

Sys.setenv("USE_CXX11" = "yes")

makes it work. So the plugin should be setting both then -- one for 3.4.0 and one for earlier versions?

Contributor

epipping commented Apr 25, 2017

@kevinushey I can confirm that

Sys.setenv("USE_CXX1X" = "yes")

has no effect on the compilation while

Sys.setenv("USE_CXX11" = "yes")

makes it work. So the plugin should be setting both then -- one for 3.4.0 and one for earlier versions?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 25, 2017

Member

The plugin is already conditional as @kevinushey showed. And the fix is good. Will commit this is a second:

# built-in C++11 plugin
.plugins[["cpp11"]] <- function() {
    if (getRversion() >= "3.4")         # with recent R versions, R can decide
        list(env = list(USE_CXX11 = "yes"))
    else if (getRversion() >= "3.1")    # with recent R versions, R can decide
        list(env = list(USE_CXX1X = "yes"))
    else if (.Platform$OS.type == "windows")
        list(env = list(PKG_CXXFLAGS = "-std=c++0x"))
    else                                # g++-4.8.1 or later
        list(env = list(PKG_CXXFLAGS ="-std=c++11"))
}
Member

eddelbuettel commented Apr 25, 2017

The plugin is already conditional as @kevinushey showed. And the fix is good. Will commit this is a second:

# built-in C++11 plugin
.plugins[["cpp11"]] <- function() {
    if (getRversion() >= "3.4")         # with recent R versions, R can decide
        list(env = list(USE_CXX11 = "yes"))
    else if (getRversion() >= "3.1")    # with recent R versions, R can decide
        list(env = list(USE_CXX1X = "yes"))
    else if (.Platform$OS.type == "windows")
        list(env = list(PKG_CXXFLAGS = "-std=c++0x"))
    else                                # g++-4.8.1 or later
        list(env = list(PKG_CXXFLAGS ="-std=c++11"))
}
@epipping

This comment has been minimized.

Show comment
Hide comment
@epipping

epipping Apr 26, 2017

Contributor

I've dug around the R source a bit to understand how it came to this:

The change wch/r-source@45899db turned, among other things, CXX1X into CXX11, USE_CXX1X into USE_CXX11, and so forth, everywhere; in particular in etc/Makeconf.in and src/library/tools/R/install.R.

This change was not backward-compatible: Prior to the change you had to set one, afterwards you had to set the other, and failing to adjust caused the setting to have no effect at all.

Before the 3.4.0 release then there was another commit, wch/r-source@628448b, where this issue was recognised and addressed. This change, however, affected only variables set in etc/Makeconf.in (like CXX11) and not those in src/library/tools/R/install.R (like USE_CXX11).

I cannot comment on whether this was an oversight or deliberate but had the latter been given a similar treatment, this issue would not have come up.

@eddelbuettel I've tested your feature branch and it fixes my problem, thank you. This issue can now be closed from my point of view but since you might prefer to leave it open until a merge, a release, or something else, I'll refrain from closing it myself.

Contributor

epipping commented Apr 26, 2017

I've dug around the R source a bit to understand how it came to this:

The change wch/r-source@45899db turned, among other things, CXX1X into CXX11, USE_CXX1X into USE_CXX11, and so forth, everywhere; in particular in etc/Makeconf.in and src/library/tools/R/install.R.

This change was not backward-compatible: Prior to the change you had to set one, afterwards you had to set the other, and failing to adjust caused the setting to have no effect at all.

Before the 3.4.0 release then there was another commit, wch/r-source@628448b, where this issue was recognised and addressed. This change, however, affected only variables set in etc/Makeconf.in (like CXX11) and not those in src/library/tools/R/install.R (like USE_CXX11).

I cannot comment on whether this was an oversight or deliberate but had the latter been given a similar treatment, this issue would not have come up.

@eddelbuettel I've tested your feature branch and it fixes my problem, thank you. This issue can now be closed from my point of view but since you might prefer to leave it open until a merge, a release, or something else, I'll refrain from closing it myself.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 26, 2017

Member

Nice digging, and there also was an announcement by Martyn. The 'not backwards compatible' is a bit unusual. OTOH the main setting via CXX_STD is unaffected, so it is only these env vars which we hit via Rcpp Attributes and sourceCpp() etc but not from packages.

And yes, plan to merge now that I have a thumbs-up from another Rcpp team member (thanks as always, @kevinushey).

Member

eddelbuettel commented Apr 26, 2017

Nice digging, and there also was an announcement by Martyn. The 'not backwards compatible' is a bit unusual. OTOH the main setting via CXX_STD is unaffected, so it is only these env vars which we hit via Rcpp Attributes and sourceCpp() etc but not from packages.

And yes, plan to merge now that I have a thumbs-up from another Rcpp team member (thanks as always, @kevinushey).

eddelbuettel added a commit that referenced this issue Apr 26, 2017

Merge pull request #684 from RcppCore/bugfix/issue683
Correcting CXX11 etc env vars for R 3.4.0 or later (closes #683)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment