search

biocro / biocro

https://biocro.github.io/
Other
41 stars 19 forks source link

Remove cxx11 #81

Closed eloch216 closed 5 months ago

eloch216 commented 8 months ago

There is an R CMD check note about requiring C++11. I looked into it a bit, and it seems that we do not need this anymore since we are not providing support for R v3.5 or lower: https://www.tidyverse.org/blog/2023/03/cran-checks-compiled-code/

eloch216 commented 8 months ago

Looks like we do need C++11. Removing it causes a lot of problems. Some of the warnings are due to the use of deprecated functions in the boost library. We could probably address these issues, but it seems simpler to just continue requiring C++11. What do you think?

gsrohde commented 7 months ago

Looks like we do need C++11. Removing it causes a lot of problems. Some of the warnings are due to the use of deprecated functions in the boost library. We could probably address these issues, but it seems simpler to just continue requiring C++11. What do you think?

Where are you seeing these warnings? Are they coming from the compiler when you do R CMD INSTALL? And it seems slightly odd to me that a version of Boost not that old (less than five years) is triggering deprecation warnings. There may be a -W flag that would turn these off, but I'd have to know what they are and I'm not finding them. Or is R doing its own C++ code checking when you run R CMD check? I'm not seeing any C++-related warnings there either.

justinmcgrath commented 7 months ago

Maybe what's happening is by removing the flag to specify the standard, it's using a newer one, and Boost is using something old? I know that one of the later standards removed things added in 11.

I wouldn't care to address such issues in Boost, but you could post them as an issue and maybe one day they'll handle it. If it was a change that didn't require any extra work from us, it would be fine, but in the meantime, I don't see any harm specifying C++11. Specifying the standard means you know exactly what you're getting, and CRAN adding a note saying they don't like it seems like make-work.

gsrohde commented 7 months ago

in the meantime, I don't see any harm specifying C++11. Specifying the standard means you know exactly what you're getting, and CRAN adding a note saying they don't like it seems like make-work.

For what it's worth, the TidyVerse article @eloch216 cited states the following:

"In (the forthcoming) R 4.3, the default C++ standard is C++17 where available. R CMD check now raises a NOTE if anything older than the default is specified in SystemRequirements: or CXX_STD in the various src/Makevars* files. This NOTE will block submission to CRAN — if the standard you specify is necessary for your package you will likely need to explain why."

If this is true, it seems to me we should make any changes needed for being able to compile with later C++ standards without warning unless the required changes really are truly burdensome.

One other note: The TidyVerse article talks about setting the CXX_STD variable in the Makevars files. But we don't do so, and yet the compilation flags used nevertheless seem to track with what the setting in the DESCRIPTION file is: if SystemRequirements: C++11 is used, the -std=gnu++11 flag is used during compilation; if SystemRequirements: C++17 is used, the -std=gnu++17 flag is used during compilation; if no standard is specified in the DESCRIPTION file, the default -std=gnu++14 flag is added (I'm using R 4.2.0).

justinmcgrath commented 7 months ago

This NOTE will block submission to CRAN — if the standard you specify is necessary for your package you will likely need to explain why."

Where is he basing that on? The news files don't say it will block CRAN submission. It doesn't make any sense to block usage of an old standard specification or to even consider it.

gsrohde commented 7 months ago

Where is he basing that on?

I have no idea—I was wondering the same thing.

That said, C++11 is getting pretty dated by now, and all things being equal, it would be nice not to have to confine ourselves to the features it supports.

eloch216 commented 7 months ago

Where are you seeing these warnings?

@gsrohde I was referring to the check failures for this PR. You can find some deprecated warnings in the check for windows-latest using R version release. You can find severe compilation errors in the check for windows-latest using R version 3.6.0.

eloch216 commented 7 months ago

Compilation warnings on MacOS-latest using R version release that cause an R CMD check failure:

Found the following significant warnings:
  ../inc/boost/numeric/ublas/storage.hpp:79:30: warning: 'construct' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/storage.hpp:79:30: warning: 'construct<unsigned long, unsigned long>' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/detail/iterator.hpp:204:21: warning: 'iterator<boost::numeric::ublas::dense_random_access_iterator_tag, double>' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/storage.hpp:152:40: warning: 'construct' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/storage.hpp:152:40: warning: 'construct<double, double &>' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/storage.hpp:158:40: warning: 'construct' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/storage.hpp:158:40: warning: 'construct<double, double &>' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/storage.hpp:162:40: warning: 'construct' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/storage.hpp:162:40: warning: 'construct<double, const double &>' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/storage.hpp:177:40: warning: 'construct' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/storage.hpp:177:40: warning: 'construct<double, double>' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/storage.hpp:193:36: warning: 'destroy' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/storage.hpp:79:30: warning: 'construct<double, double>' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/storage.hpp:152:40: warning: 'construct<unsigned long, unsigned long &>' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/storage.hpp:158:40: warning: 'construct<unsigned long, unsigned long &>' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/storage.hpp:162:40: warning: 'construct<unsigned long, const unsigned long &>' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/storage.hpp:177:40: warning: 'construct<unsigned long, unsigned long>' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/detail/iterator.hpp:149:21: warning: 'iterator<boost::numeric::ublas::sparse_bidirectional_iterator_tag, double>' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/detail/iterator.hpp:204:21: warning: 'iterator<boost::numeric::ublas::packed_random_access_iterator_tag, double>' is deprecated [-Wdeprecated-declarations]

Compilation warnings on Windows-latest using R version release that trigger an R CMD check failure:

Found the following significant warnings:
  ../inc/boost/numeric/ublas/detail/iterator.hpp:111:21: warning: 'template<class _Category, class _Tp, class _Distance, class _Pointer, class _Reference> struct std::iterator' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/detail/iterator.hpp:149:21: warning: 'template<class _Category, class _Tp, class _Distance, class _Pointer, class _Reference> struct std::iterator' is deprecated [-Wdeprecated-declarations]
  ../inc/boost/numeric/ublas/detail/iterator.hpp:204:21: warning: 'template<class _Category, class _Tp, class _Distance, class _Pointer, class _Reference> struct std::iterator' is deprecated [-Wdeprecated-declarations]

There are too many warnings for Windows-latest using R version 3.6.0 to list them all here. They are weird, such as these two:

In file included from C:/Rtools/mingw_64/x86_64-w64-mingw32/include/c++/unordered_map:35:0,

                 from framework/state_map.h:4,

                 from framework/module_creator.h:6,

                 from framework/R_helper_functions.h:6,

                 from R_system_derivatives.cpp:5:

C:/Rtools/mingw_64/x86_64-w64-mingw32/include/c++/bits/c++0x_warning.h:32:2: error: #error This file requires compiler and library support for the ISO C++ 2011 standard. This support is currently experimental, and must be enabled with the -std=c++11 or -std=gnu++11 compiler options.

 #error This file requires compiler and library support for the \

  ^

In file included from framework/dynamical_system_helper_functions.h:11:0,

                 from framework/dynamical_system.h:13,

                 from R_system_derivatives.cpp:7:

framework/constants.h:21:1: warning: identifier 'constexpr' is a keyword in C++11 [-Wc++0x-compat]

 constexpr double eps_deriv = 1e-5;

 ^
eloch216 commented 7 months ago

For what it's worth, I also tried using v1.84.0 of the boost libraries (which was just released in December) on my computer (Windows 10, R v4.3.2). Here's what I found:

From this, it seems like keeping C++11 will also pose problems for newer versions of boost. I will try changing to C++14 here and see what happens.

gsrohde commented 7 months ago

Compilation warnings on MacOS-latest using R version release that cause an R CMD check failure:

I'm trying out using pragmas as suggested here: https://stackoverflow.com/questions/13459602/how-can-i-get-rid-of-deprecated-warnings-in-deprecated-functions-in-gcc

I looks like you're trying out the other idea I had, which is to explictly require C++14 (so that C++17 isn't used).

There are too many warnings for Windows-latest using R version 3.6.0 to list them all here. They are weird, such as these two:

My hunch is that R 3.6.0 falls back to C++98 if a later version is not required.

gsrohde commented 7 months ago

I'm trying out using pragmas

It looks like this fixes the problem for the R release versions on macOS and Windows (see my get_rid_of_deprecation_warnings branch). I pretty sure it won't fix the problem with R 3.6.0 on Windows. Is it allowed to specify SystemRequirements: >= C++14?

eloch216 commented 7 months ago

After specifying C++14, the only check failure is for Windows using R v3.6.0. The error message is:

Error in .shlib_internal(args) : 

  C++14 standard requested but CXX14 is not defined

The version of Rtools that is used with R v3.6.0 is Rtools 3.5, which comes with gcc 4.9.3. This version of gcc supports C++14, but it apparently does it in a nonstandard way: https://stackoverflow.com/questions/31965413/compile-c14-code-with-g

There might be a way to address this in the makefile. I don't know much about how makefiles work, but I am trying to look into it.

Update: I haven't figured anything out.

eloch216 commented 7 months ago

Apparently it's possible to specify a version of Rtools in the setup-r action: https://www.tidyverse.org/blog/2022/06/actions-2-0-0/

I believe Rtools 40 is the first version with support for C++14. I wonder if we can specify this version of Rtools when testing Windows / R v3.6.0? Not sure if this will work.

justinmcgrath commented 7 months ago

I believe Rtools 40 is the first version with support for C++14. I wonder if we can specify this version of Rtools when testing Windows / R v3.6.0? Not sure if this will work.

It lists Rtools 4.0 as supporting R versions 4.0.0 to 4.1.3, so I don't think that will work.

justinmcgrath commented 7 months ago
  • If I remove C++ specifications from SystemRequirements, I get deprecated warnings from boost files

Based on this, it appears that even in Boost 1.84.0 they are using features that are now deprecated. To use the latest standard without those warnings, they will need to address this. I don't see that getting our package to compile with C++14 is any better than having it compiled with C++11. In either case, we specify a standard, which if that other guy is right about them rejecting a standards requirement, then I don't see why specifying one would be any different from specifying another.

gsrohde commented 7 months ago

There might be a way to address this in the makefile. I don't know much about how makefiles work, but I am trying to look into it.

I'm looking at https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Portable-C-and-C_002b_002b-code, which seems like it may be relevant.

eloch216 commented 7 months ago

I believe Rtools 40 is the first version with support for C++14. I wonder if we can specify this version of Rtools when testing Windows / R v3.6.0? Not sure if this will work.

It lists Rtools 4.0 as supporting R versions 4.0.0 to 4.1.3, so I don't think that will work.

It seems to have not worked.

gsrohde commented 7 months ago

I believe Rtools 40 is the first version with support for C++14. I wonder if we can specify this version of Rtools when testing Windows / R v3.6.0? Not sure if this will work.

It lists Rtools 4.0 as supporting R versions 4.0.0 to 4.1.3, so I don't think that will work.

It seems to have not worked.

Setting "rtools-version": "40" in the strategy matrix won't do anything, since the code that uses the strategy matrix doesn't (currently) expect or use this key.

The relevant code is the "Set up R" step in R-CMD-check-backend.yaml. This uses the r-lib/actions/setup-r@v2 action, which is documented here: https://github.com/r-lib/actions/tree/v2-branch/setup-r

There is an optional "rtools-version" input for this action, but since, according to the documentation, "Default uses latest suitable rtools for the given version of R.", I don't think this will help us.

gsrohde commented 7 months ago

From this, it seems like keeping C++11 will also pose problems for newer versions of boost. I will try changing to C++14 here and see what happens.

I think I've come up with a suitable solution which would keep using C++11 with older R versions on Windows but would use C++14 otherwise. I'll make a new pull request with an implementation and then let you poke holes in it.

justinmcgrath commented 7 months ago

The warnings you list are all from ublas. From their issues page, they are aware of this, and have considered moving to C++17 for several years, but clearly it hasn't happened yet. We're basing the need to stop specifying a standard on a blog post. I think that we should base the need on what CRAN tells us, and even in the blog post he says that you'll need to justify including the standard, not that it will never be accepted. That seems straightforward, because we can say that we use a publicly available library that depends on C++11, and when they update, we'll remove our requirement.

gsrohde commented 7 months ago

After specifying C++14, the only check failure is for Windows using R v3.6.0. The error message is:

Error in .shlib_internal(args) : 

  C++14 standard requested but CXX14 is not defined

The problem with Windows using R v3.6.0 could easily be solved by using a Makevars.ucrt file in addition to Makevars.win as I do in PR #86 to address this issue. Forcing the use of C++14 even when C++17 is available is one way to avoid the "deprecated declaration" warnings , an alternative to my use of pragmas in PR #86, which @justinmcgrath somehow doesn't like. To my mind, it would be easier to justify to CRAN limiting the standard to C++14 than limiting it to C++11.

Just to be clear, in this proposed solution, C++14 would be removed from the SystemRequirements field, CXX_STD = CXX14 would be added to both Makevars and Makevars.ucrt instead, and CXX_STD = CXX11 would be added to Makevars.win (hence only used by Window platforms using old versions of R that don't recognize Makevars.ucrt).

gsrohde commented 7 months ago

We're basing the need to stop specifying a standard on a blog post.

Well, not only that, but also from the NOTE that gets issued by R CMD check.

That said, this statement in the blog post raised my eyebrows: "This NOTE will block submission to CRAN — if the standard you specify is necessary for your package you will likely need to explain why." Is this true of all NOTEs? If so, that's news to me. If not, is this one singled out by CRAN as more significant, in which case, how would the blog poster know this?

I think that we should base the need on what CRAN tells us, and even in the blog post he says that you'll need to justify including the standard, not that it will never be accepted. That seems straightforward, because we can say that we use a publicly available library that depends on C++11, and when they update, we'll remove our requirement.

While ideally we can have a nice give-and-take with CRAN if any issues come up regarding this, my impression has been—and I may be completely wrong about this—that the process of getting a package accepted becomes interminably slower if you don't get things mostly right on the first shot. I don't know if I heard this from one of you or read it somewhere, and if the latter, I don't remember where. So take that for what it's worth, if anything.

justinmcgrath commented 7 months ago

Well, not only that, but also from the NOTE that gets issued by R CMD check

The NOTE says to drop the specification unless essential, and we have a library that was written for C++11, so that is easy to explain.

I also think that pushing back on bad policies makes things easier for us in the long term. Telling people they can't specify a standard is absurd. Pegging your code to specific versions of software ensures reliability. You can guarantee that people installing your software are able to compile it properly. That is impossible if you can't specify versions. The inability to do that easily with R packages makes reliability a PITA, and them trying to force that same chaotic mind set on the C++ code is a bad choice.

eloch216 commented 7 months ago

Based on the discussion here and in two other PRs (#85 and #86), it seems like the best course of action for now is to keep the C++11 specification. We can justify it because we need it for our version of the boost library.

At some point, we will upgrade our version of boost. When that happens, we might need to also update the specification to C++14. We'll cross that bridge when we get to it.

Adding pragmas to ignore compiler warnings isn't my favorite option. I'm hoping that boost libraries will eventually stop using deprecated code.

gsrohde commented 7 months ago

Based on the discussion here and in two other PRs (#85 and #86), it seems like the best course of action for now is to keep the C++11 specification. We can justify it because we need it for our version of the boost library.

At some point, we will upgrade our version of boost. When that happens, we might need to also update the specification to C++14. We'll cross that bridge when we get to it.

Adding pragmas to ignore compiler warnings isn't my favorite option. I'm hoping that boost libraries will eventually stop using deprecated code.

Just want to make sure you noticed my alternative proposal to use C++11 only for R 3.6 and C++14 for everything else: https://github.com/biocro/biocro/pull/81#issuecomment-1944363266 This doesn't require pragmas.

That said, I don't see a strong reason to do anything, unless perhaps CRAN would be more amenable to a general C++14 restriction than a C++11 one: we're still limited to using only C++11 features as long as we continue to offer support for R 3.6.

gsrohde commented 5 months ago

Current plan is to keep the C++11 requirement in DESCRIPTION (even though it will require our justifying the resulting NOTE) with a longer-term goal of moving to C++17-compliant code.