search

stan-dev / rstantools

Tools for Developing R Packages Interfacing with Stan
https://mc-stan.org/rstantools
GNU General Public License v3.0
51 stars 21 forks source link

Update C++ Standard to C++17 #100

Closed andrjohns closed 1 year ago

andrjohns commented 1 year ago

As discussed over on the forums, CRAN has begun issuing a NOTE when the requested C++ standard is lower than C++17. This PR updates the rstantools-provided Makevars to request the higher standard.

I've run reverse-dependency checks and all packages continue to build and pass. This will break the current rstan 2.26, but I've already opened a PR with the needed patches here

jgabry commented 1 year ago

Thanks @andrjohns! I've tagged @bgoodri to review since this is more in his wheelhouse than mine.

bgoodri commented 1 year ago

Can we run the upstream unit tests under the C++-17 standard once?

andrjohns commented 1 year ago

Can we run the upstream unit tests under the C++-17 standard once?

Which unit tests do you mean sorry?

bgoodri commented 1 year ago

I meant all of them in Stan Math and Stan Library but with -std=c++-17 in the makefile.

jgabry commented 1 year ago

@andrjohns Also, at this point we should make you a coauthor on this package instead of just a contributor. Feel free to change ctb to aut in the DESCRIPTION file as part of this PR.

Also feel free to make branches directly in the repo in the future (if you want). I think you should have permission but if not let me know.

andrjohns commented 1 year ago

I meant all of them in Stan Math and Stan Library but with -std=c++-17 in the makefile.

Ah good point. I made branches of the Math and Stan versions used in rstan 2.21 and added an actions workflow to run the respective tests under std=gnu++-17, all tests passed:

andrjohns commented 1 year ago

And the upstream tests with recent Stan & Math were also run under c++-17 as part of this PR

jgabry commented 1 year ago

@bgoodri Does this change look ok to you? If so I can start preparing the CRAN submission. On the forum @andrjohns said he ran reverse dependency checks and all packages passed.

Not sure if you saw this, but the reason for the approach @andrjohns took is related to this post from user @ edm on the forum (I think username @ecmerkle on GitHub):

Related to this C++17 issue: I uploaded a new package version to CRAN a few days ago and today received the message below. I am reporting back in case it influences the PR. (I edited the message to only include relevant parts; I believe @phcooney also received it)

These are now being compiled with C++17. Unfortunately they make use of Boost headers that are not compliant with that standard, using functions which were deprecated in C++11 and removed in C++17.

One workaround is to declared a dependence on C++14

Reading the headers suggested

PKG_CPPFLAGS = -D_HAS_AUTO_PTR_ETC=0

in src/Makevars[.win] would suffice, and I tested that for DDD (already reported). Or you could add

#if __cplusplus >= 201703L
# define _HAS_AUTO_PTR_ETC 0
#endif

at the top of the files using Boost headers. (It looks like the stan users should get that done upstream.)

cjvanlissa commented 1 year ago

Dear all, is there any progress on this? I'm holding back a necessary update to a package that depends on rstan and rstantools because of the aforementioned CRAN note.

wds15 commented 1 year ago

I concur in that this PR should move ahead ASAP, since all downstream packages really need this upgrade. I just had to workaround things in an ugly way...see here

https://github.com/weberse2/RBesT/blob/1bb60a01f37cdcc613d752a438f16d4f19b5760d/configure

To me it appears that CRAN is now happy with C++17 being used as a new default C++ standard. I have verified with a docker image (rhub/clang16)

https://hub.docker.com/u/rhub

that the fix which is implemented in this PR is indeed what resolves the issues with clang 16 (this extra define thing).

However, when reading the CRAN instructions for package maintainers carefully, they also say that it is strongly recommended to add C++17 to the SystemRequirements field of the DESCRIPTION file whenever C++17 is used (in addition to the src/Makevars). I am not sure if this is a hard check on their end on this, but their manuals imply that one should have this. Maybe even that could be automated with rstantools (that is to add C++17 automatically if no C++ standard has been set)? Or we just leave this.

In any case... this PR would be great to roll out soon.

However, making C++17 the default could be a challenge for some packages which use functions like "size" as I recall. These have compile issues with C++17. Not sure what to do about this other than finally moving to 2.26 where the C++17 back ports have been incorporated as I understood from @andrjohns .

BTW... when building rstan within the clang16 rhub docker image I was forced to define BOOST_ALLOW_DEPRECATED to make things work. If I did not set this, then some phoenix stuff would trigger a compile error. This deprecation happened in the parser bits and pieces of rstan 2.21, which I assume we would get rid of with 2.26

andrjohns commented 1 year ago

However, making C++17 the default could be a challenge for some packages which use functions like "size" as I recall. These have compile issues with C++17.

This isn't an issue for rstan/StanHeaders 2.21 since the templating for the Math library's size() function at the time was not as general. StanHeaders 2.26 has also been patched for handling this with both rstan 2.21 (this PR) and rstan 2.26 (this PR).

BTW... when building rstan within the clang16 rhub docker image I was forced to define BOOST_ALLOW_DEPRECATED to make things work.

I believe this has been addressed by adding -D_HAS_AUTO_PTR_ETC=0 to the PKG_CPPFLAGS in the current PR, as was recommended by CRAN

wds15 commented 1 year ago

Let's see, maybe Dirk can handle the matter with C++17 upstream in BH:

https://github.com/eddelbuettel/bh/issues/92