HenrikBengtsson / matrixStats

R package: Methods that Apply to Rows and Columns of Matrices (and to Vectors)
https://cran.r-project.org/package=matrixStats
206 stars 34 forks source link

CRAN: STRICT_R_HEADERS is removed in recent R-devel versions #271

Open HenrikBengtsson opened 1 week ago

HenrikBengtsson commented 1 week ago

From: Prof Brian Ripley Date: Sun, Nov 10, 2024 at 4:33 AM Subject: CRAN package matrixStats To: Henrik Bengtsson henrikb@braju.com Cc: CRAN CRAN@r-project.org

You have comments like

/ With strict headers (becoming the default),   the prefixed variants R_Calloc and R_Free   must be used instead of Calloc and Free. However, the prefixed variants   do not exist prior to R 3.4.0, so we check whether strict headers are used and   apply the legacy functions if not.   If future versions of R remove the un-prefixed variants   and no longer display the macro STRICT_R_HEADERS, this workaround will fail.   In such a case, we must enforce the prefixed variants and increase the version   requirement of the package to R 3.4.0.     */

which is making a false assumption (and I have no idea why you still care about such ancient versions of R and did not test the R version).

R-devel's NEWS currently says

     • Strict R headers are now the default.  This removes the legacy        definitions of PI, Calloc, Realloc and Free: use M_PI, R_Calloc,        R_Realloc or R_Free instead.

       Pro tem STRICT_R_HEADERS is defined in header R_ext/RS.h if not        already defined.  This is not used in R itself and will be        removed before release.

so please adapt as soon as possible (the second para was added to avoid large-scale CRAN check failures).

-- Brian D. Ripley,                  ripley@stats.ox.ac.uk Emeritus Professor of Applied Statistics, University of Oxford

yaccos commented 1 week ago

This relates to our discussion back in issue #266. At that time, we based on decision on the assumption that STRICT_R_HEADERS would still be defined in the future versions of R. Appearently, the R core team has relatively recently documented that the flag itself will go away, thus breaking our check for strict headers.

yaccos commented 1 week ago

As I view this situation, we must remove the macro definitions of R_CALLOC and R_FREE and replace them with the R_Calloc and R_Free, respectively before R 4.5.0 is released. As a consequence, the R version requirement for the package must be raised to 3.4.0 as there is no feasible way to support both R <3.4.0 and R 4.5.0 with the same codebase. Do you agree?

yaccos commented 1 week ago

On the flip side rasing the version requirement will help us clean up the code. We no longer need to define R_XLEN_T_MAX (added to base R in version 3.0.0) and we can deprecate and eventually remove anyMissing.

HenrikBengtsson commented 1 week ago

Yeah, let's accept it and bump the version requirement. I like that it'll allow us to clean things up.

About anyMissing(), see https://github.com/HenrikBengtsson/matrixStats/issues/243#issuecomment-1850765272

HenrikBengtsson commented 1 week ago

FYI, Rversion.h defines R_MAJOR and R_MINOR, and R_PATCH, so one can test for the R version, as BDR mentions, but I'm fine with moving to R (>=3.4.0).

HenrikBengtsson commented 1 week ago

Before fixing, I was trying to reproduce using rhub::rhub_check(platforms = "gcc14"), but it turns out they've added a special patch for us (https://github.com/wch/r-source/commit/9819471c4b110efc5af1239828b8f46bd25b0fa4).

HenrikBengtsson commented 1 week ago

FYI, Rversion.h defines R_MAJOR and R_MINOR, and R_PATCH, so one can test for the R version, as BDR mentions, but I'm fine with moving to R (>=3.4.0).

Actually, these are strings, not integers, so we cannot just do:

#if (R_MAJOR > 4) || (R_MAJOR == 4 && R_MINOR >= 5)

But, we can do:

#if (R_VERSION >= R_Version(4, 5, 0))

I pushed this minimal fix (commit 90174b0).

We can still do the full cleanup, but I figured this is at least a step forward.

yaccos commented 1 week ago

Your fix does not work as intended. Compling with STRICT_R_HEADERS on R versions prior to 4.5.0 will lead to a compilation error.

yaccos commented 1 week ago

It turns out there is an additional challenge of compiling with strict headers under 4.4.1: DBL_MAX is undefined. The reason behind this is that the header float.h containing this definition is only included if STRICT_R_HEADERS is undefined. For R version 4.5.0, this has changed to unconditionally including this header file.

HenrikBengtsson commented 1 week ago

It's okay to ignore old versions of R-devel; those were work in progress versions