Fixes #115

[vtamara]

Credit to @wch

[wch]

@jimhester, @kevinushey Do you guys know if this change will cause problems on platforms that don't support C++17?

[jimhester]

Yes, it definitely will, the CXX17 macro won't be set with the current Windows toolchain for instance.

[jimhester]

Well actually this change doesn't actually affect Windows, since this only changes Makevars and not Makevars.win, but the underlying issue is the same, if R wasn't built with a compiler that supports C++17 then the CXX17 macro won't be set and the install will fail. This is why travis is failing for this PR for instance https://travis-ci.org/github/r-lib/later/jobs/667382735#L1790

[kevinushey]

Yes, I would stick with CXX11 and allow some way to opt-in to using CXX17 if supported by the active compiler. (This unfortunately does imply writing a configure script but it looks like later already has one so you've already accepted that pain)

[wch]

Thanks, guys. There already is a configure script, so that at least reduces the amount of work needed.

[vtamara]

Finally I spent some time trying to improve the configure script.

With this fix I can compile without issue in OpenBSD/adJ 6.6.

In this platform, the test program that I included in configure if compiled with CXX11 gives:

$ c++ -std=c++11 -c test_timespec.cpp
test_timespec.cpp:9:21: error: use of undeclared identifier 'TIME_UTC'
1 error generated.

but compiled with CXX17 gives no error:

$ c++ -std=c++17 -c test_timespec.cpp

@wch I would appreciate if you could please check.

Blessings.

[wch]

Note that there are some platforms that don't support std-=c++11. I'm not even sure that some of the more obscure platforms (like the compiler on Solaris, which is one of the platforms used by CRAN) have the -std option.

[wch]

@vtamara I don't think the test is correct -- on the Ubuntu CI, you can see that it says that C++17 is required, but that hasn't been true in the past. It's probably because it normally would use -std=gnu++11.

Finding the default compiler flags used by R for C++11 can be done by calling:

R CMD config CXX11FLAGS
R CMD config CXX11STD

However, keep in mind that not all platforms that we need to support have C++11 compilers, notably RedHat/Centos 6.

[wch]

Please use spaces instead of tabs.

[vtamara]

@wch What do you think about this one? I tested in OpenBSD/adJ 6.6 as well as Ubuntu 19.10 and looks good. Please feel free to change it to make it more portable.

[wch]

On some platforms, the /bin/sh does not support $() for running commands. You need to use backticks instead. I recently ran into this on Solaris.

[vtamara]

Ok, I changed its use, even in the test that was already there.

[wch]

Two spaces per level of indenting, please.

[wch]

Can you add a comment that explains the logic being used by these checks?

[wch]

@vtamara I've given some feedback, but don't currently have time to test and fix on various platforms for you. I don't want to introduce breakage on platforms that are widely used, or which CRAN requires us to build on. Notable platforms include Mac, Windows, RHEL/Centos 6 (which by default does not have a compiler that supports C++11), and Solaris (which has a very outdated toolchain but which CRAN still uses for checks.)

[wch]

One more thought: it may be simpler to do something like this:

• If C++17 is available, use CXX_STD=CXX17
• else use CXX_STD=CXX11

[vtamara]

@wch I implemented the simpler method you suggested.

For me it works on OpenBSD/adJ 6.6 with R 3.6.1, on Ubuntu 19.10 with R 3.6.1 and on Ubuntu 18.04 with R 3.4.4.

The CI shows it works fine on macOS-latest, windows and ubuntu-16.04 with R 3.6.x, although it has problems on ubuntu-16.04 and R 3.5, 3.4, 3.3 and 3.2.

On the platforms where it doesn't work it shows:

* creating vignettes ... ERROR
Error: processing vignette 'later-cpp.Rmd' failed with diagnostics:
'vignetteInfo' is not an exported object from 'namespace:tools'

So I guess, more things should be tested before setting CXX_STD=CXX17.

Any suggestion on how to test on those platform where it fails or on solaris?

[wch]

I still think the logic isn't quite right. I think it should be something like this:

• If a C++17 compiler is available, we know that <ctime> must provide timespec_get and TIME_UTC (as per https://en.cppreference.com/w/cpp/chrono/c/timespec_get). In this case, we can just use CXX_STD=CXX17, without compiling any test code.
• If a C++17 compiler is NOT available, then <ctime> does not necessarily provide timespec_get and TIME_UTC -- but on all platforms we have actually encountered, it does. So we can just use CXX_STD=CXX11 in this case.

A bit more detail: in the second case (where C++17 is not available), rather than simply using CXX_STD=CXX11, it would be more correct to try to compile the test program to check for timespec_get and TIME_UTC. But again, we haven't actually encountered a platform where this is needed.

I believe that to check for C++17 support, you can run R CMD config CXX17 and check if it's empty, though I'm not 100% sure that's the right way. Documentation is here: https://cran.r-project.org/doc/manuals/R-exts.html#Using-C_002b_002b14-and-later

Just to add a bit more detail of the challenges for platform-specific stuff like this: we need to support not only the platforms that I mentioned earlier (and which we test with continuous integration) but also platforms with older versions of R, and older compilers. I noticed when I read this section of Writing R Extensions that with R 4.0, you can always assume a C++11 compiler is available; however, for older versions of R, that isn't the case.

[vtamara]

@wch As far as I know, in configure scripts it is not advisable to check for tools and version but if the existing tools have the features required. I think this example and the CI results show why.
Although in Ubuntu 16.04 with R 3.5, it seems there is support for C++17, there are problems using that compiler. Also the documentation you reference says in the paragraphs about C++17 "Feature tests can be used (and probably should be as support is still patchy, especially library support)."

Since in most platforms it works with CXX_STD=CXX11, in my humble opinion it is better to change to CXX_STD=CXX17 only when test scripts for every problematic feature pass (of course identifying test script for those problematic features).

[wch]

I just realized this should be CXX17.

[wch]

It's a weird thing about R, that when one sets CXX_STD=CXX11, it doesn't necessarily mean that it will be compiled with a C++11 compiler. It just means that if a C++11 compiler is available, it will use it. (Note that this is different from how R uses CXX14 and CXX17.)

[wch]

I understand your point of view, in trying to be conservative about the changes. If that's the idea, then this logic makes sense to me:

• First try to compile the test program using the CXX11 compiler, if available, and if not, the CXX compiler. (See my other comment about the weird way that R uses CXX_STD=CXX11.)
  • If it passes, then use CXX_STD=CXX11.
  • If the test program fails, then check if a CXX17 compiler is available.
    • If it is available, use CXX17.
    • If not available, Then fall back to CXX11

As I understand your code, it first looks for a C++17 compiler, and if present, it tries to compile a test program that should always pass according to the C++17 spec. The test program is therefore redundant (modulo the possibility of a non-compliant compiler, but the time stuff seems like a pretty basic part of the standard so that seems very unlikely.)

But the current problem with OpenBSD is that we try to use C++11, but that standard doesn't guarantee that the time stuff is available. The test program can fail when compiling the code with a C++11 compiler (this is basically what's happening with the current release version of later), but not a C++17 compiler. However, OpenBSD does have a C++17 compiler that does make the time stuff available.

A few other comments:

• The CI failures in older versions of R are due to some external problems, probably with the setup of the CI system. The tools::vignetteInfo function is only available from R 3.6.0 and up, so it shouldn't even be trying to use it. (See https://hughjonesd.shinyapps.io/rcheology/)
• In the context of a configure script for an R package, I think it's reasonable to check for CXX17. (I also just realized that "${R_HOME}"/bin/R CMD config CXX17 is what should be used to find the compiler to build the test program, since it also uses CXX17STD and CXX17FLAGS.) The time stuff seems like a pretty basic part of the standard, so I would expect it to work.
• I made some comments on the code that haven't been addressed.

Just a heads up: my time is very, very limited these days, and my other responsibilities mean that I can't really spend much more time on this.