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

Fixes #115 #121

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fixes #115 #121

wants to merge 2 commits into from

Conversation

vtamara
Copy link

@vtamara vtamara commented Mar 26, 2020

Credit to @wch

@wch
Copy link
Member

wch commented Mar 31, 2020

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

@jimhester
Copy link
Member

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

@jimhester
Copy link
Member

jimhester commented Mar 31, 2020

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
Copy link

kevinushey commented Mar 31, 2020

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
Copy link
Member

wch commented Mar 31, 2020

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

@vtamara vtamara force-pushed the fix-cxx17 branch 2 times, most recently from 23a1645 to 1dd40c8 Compare June 1, 2020 19:04
@vtamara
Copy link
Author

vtamara commented Jun 1, 2020

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.

configure Outdated
}

}
" | ${CXX} -x c++ -std=c++11 - -o /dev/null > /dev/null 2>&1
Copy link
Member

@wch wch Jun 1, 2020

Choose a reason for hiding this comment

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

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
Copy link
Member

wch commented Jun 1, 2020

@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.

configure Outdated

namespace std {

int main()
Copy link
Member

Choose a reason for hiding this comment

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

Please use spaces instead of tabs.

@vtamara vtamara force-pushed the fix-cxx17 branch 2 times, most recently from eabcaec to e714e81 Compare June 1, 2020 20:05
@vtamara
Copy link
Author

vtamara commented Jun 2, 2020

@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.

configure Outdated
@@ -20,9 +20,50 @@ else
EXTRA_PKG_LIBS=-latomic
fi

CXX=$("${R_HOME}"/bin/R CMD config CXX)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

configure Outdated
CXX=$("${R_HOME}"/bin/R CMD config CXX)
CXX11FLAGS=$("${R_HOME}"/bin/R CMD config CXX11FLAGS)
if [ "$CXX" != "" -a "$CXX11FLAGS" != "" ]; then
# Detect whether if c++11 is enough
Copy link
Member

Choose a reason for hiding this comment

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

Two spaces per level of indenting, please.

configure Outdated
@@ -20,9 +20,50 @@ else
EXTRA_PKG_LIBS=-latomic
fi

CXX=$("${R_HOME}"/bin/R CMD config CXX)
CXX11FLAGS=$("${R_HOME}"/bin/R CMD config CXX11FLAGS)
if [ "$CXX" != "" -a "$CXX11FLAGS" != "" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

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

@wch
Copy link
Member

wch commented Jun 2, 2020

@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
Copy link
Member

wch commented Jun 2, 2020

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 vtamara force-pushed the fix-cxx17 branch 2 times, most recently from acc0ac8 to e61c967 Compare June 3, 2020 16:40
@vtamara
Copy link
Author

vtamara commented Jun 3, 2020

@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
Copy link
Member

wch commented Jun 3, 2020

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
Copy link
Author

vtamara commented Jun 3, 2020

@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).

configure Outdated
CXX_STD=CXX11

# But if it works, use CXX17
CXX=`"${R_HOME}"/bin/R CMD config CXX`
Copy link
Member

Choose a reason for hiding this comment

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

I just realized this should be CXX17.

configure Outdated
echo "Using c++17."
CXX_STD=CXX17
else
echo "Cannot use c++17 using default configuration for c++11"
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

wch commented Jun 3, 2020

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.

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

Successfully merging this pull request may close these issues.

4 participants