HenrikBengtsson / matrixStats

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

Re-add support for gcc 4.8.5 (2015) [sic!] #225

Closed HenrikBengtsson closed 1 year ago

HenrikBengtsson commented 1 year ago

Package fails to compile with gcc 4.8.5 (2015), which is the default on RHEL/CentOS 7.9, because:

$ gcc --version
gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44)
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$ R CMD build ...
gcc -I"/software/c4/cbi/software/R-4.2.2-gcc10/lib64/R/include" -DNDEBUG   -I/usr/local/include   -fpic  -g -O2  -c naming.c -o naming.o
naming.c: In function ‘setNames’:
naming.c:14:5: error: ‘for’ loop initial declarations are only allowed in C99 mode
     for (R_xlen_t i = 0; i < length; i++) {
     ^
naming.c:14:5: note: use option -std=c99 or -std=gnu99 to compile your code
naming.c: In function ‘setDimnames’:
naming.c:47:5: error: ‘for’ loop initial declarations are only allowed in C99 mode
     for (R_xlen_t i = 0; i < nrows; i++) {
     ^
naming.c:69:7: error: ‘for’ loop initial declarations are only allowed in C99 mode
       for (R_xlen_t i = 0; i < ncols; i++) {
       ^
naming.c: In function ‘set_rowDiffs_Dimnames’:
naming.c:109:5: error: ‘for’ loop initial declarations are only allowed in C99 mode
     for (R_xlen_t i = 0; i < nrows; i++) {
     ^
naming.c:129:7: error: ‘for’ loop initial declarations are only allowed in C99 mode
       for (R_xlen_t i = (ncols - ncol_ans); i < ncols; i++) {
       ^
naming.c:135:7: error: ‘for’ loop initial declarations are only allowed in C99 mode
       for (R_xlen_t i = (ncols - ncol_ans); i < ncols; i++) {
       ^
naming.c: In function ‘set_colDiffs_Dimnames’:
naming.c:173:7: error: ‘for’ loop initial declarations are only allowed in C99 mode
       for (R_xlen_t i = (nrows - nrow_ans); i < nrows; i++) {
       ^
naming.c:179:7: error: ‘for’ loop initial declarations are only allowed in C99 mode
       for (R_xlen_t i = (nrows - nrow_ans); i < nrows; i++) {
       ^
naming.c:201:5: error: ‘for’ loop initial declarations are only allowed in C99 mode
     for (R_xlen_t i = 0; i < ncols; i++) {
     ^

This shouldn't be hard to fix. It's worth fixing this to remove friction for users on RHEL/CentOS 7.9.

PS. I think this is a regression that happened during the last couple of years.

HenrikBengtsson commented 1 year ago

There's also:

gcc -I"/software/c4/cbi/software/R-4.2.2-gcc10/lib64/R/include" -DNDEBUG   -I/usr/local/include   -fpic  -g -O2  -c rowRanksWithTies
.c -o rowRanksWithTies.o
rowRanksWithTies.c: In function ‘SHUFFLE_INT’:
rowRanksWithTies.c:28:10: error: ‘for’ loop initial declarations are only allowed in C99 mode
          for (size_t k = i; k < j; k++) {
          ^
HenrikBengtsson commented 1 year ago

... and

gcc -I"/software/c4/cbi/software/R-4.2.2-gcc10/lib64/R/include" -DNDEBUG   -I/usr/local/include   -fpic  -g -O2  -c validateIndices.c -o validateIndices.o
validateIndices.c: In function ‘validate’:
validateIndices.c:164:5: error: ‘for’ loop initial declarations are only allowed in C99 mode
     for (R_xlen_t i = 0; i < ansNidxs; i++) {
     ^
validateIndices.c:164:5: note: use option -std=c99 or -std=gnu99 to compile your code
validateIndices.c:192:7: error: ‘for’ loop initial declarations are only allowed in C99 mode
       for (R_xlen_t i = 0; i < ansNidxs; i++) {
       ^
validateIndices.c:204:7: error: ‘for’ loop initial declarations are only allowed in C99 mode
       for (R_xlen_t i = 0; i < ansNidxs; i++){
       ^
make: *** [validateIndices.o] Error 1
HenrikBengtsson commented 1 year ago

The origin of these C99 specifications:

Thus, the problem has been there since matrixStats 0.55.0.

I just verified; matrixStats 0.54.0 (2018-07-23) installs with GCC 4.8.5.

HenrikBengtsson commented 1 year ago

I've fixed this the develop branch (commit 67563de)

yaccos commented 1 year ago

I consider this more a problem of the compiler flags rather than our source code. Indeed, the source code of the R interpreter requires C99 to compile and uses ‘for’ loop initial declarations just as we did. Additionally, users with gcc 4.8.5 will struggle with all the other R packages which uses features introduced in C99 if they use the same setup. Hence, a better solution is adding -std=c99 to CFLAGS in the Makeconf file for the users who struggle with the problem.

HenrikBengtsson commented 1 year ago

Thanks. Yes, we could have done that too. Since we didn't have one, I just decide to "get things done" quickly.

FWIW, not that there are lots of people out there on RHEL/CentOS 7 running modern R version built with modern GCC (via devtoolset SCLs), who still have GCC 4.8.5 as their default. Many of them struggle with installing some R packages, spending hours and days figuring it out, and loading admin support with questions. I figured, matrixStats shouldn't be one of those packages. I only noticed this because I happened to run revdep checks on a machine without SCL support.