Compilation fails R4.3.0

[byrongibby]

nloptr fails to compile with the following error

/usr/lib/R/library/testthat/include/testthat/vendor/catch.h:6546:45: error:
size of array ‘altStackMem’ is not an integral constant-expression
 6546 |     char FatalConditionHandler::altStackMem[SIGSTKSZ] = {};
      |                                             ^~~~~~~~
make: *** [/usr/lib64/R/etc/Makeconf:200: test-runner.o] Error 1
ERROR: compilation failed for package ‘nloptr’

platform       x86_64-pc-linux-gnu         
arch           x86_64                      
os             linux-gnu (Arch btw)     
system         x86_64, linux-gnu           
status                                     
major          4                           
minor          3.0                         
year           2023                        
month          04                          
day            21                          
svn rev        84292                       
language       R                           
version.string R version 4.3.0 (2023-04-21)
nickname       Already Tomorrow

compilation-log.txt

[eddelbuettel]

But look at the path: It is a bug in the catch library included in package testthat. Can you talk to them, please?

[byrongibby]

Apologies, I will follow up with them.

[astamm]

Please let us now what happens so that we can close the issue.

[aadler]

For what it is worth, I have consistently been able to build nloptr on R 4.3.x and 4.4.0 (R-devel) fromn source on windows with no issues. I'd happily move us to tinytest but there are C++ tests which testhat runs which I do not think can be run in tinytest.

[eddelbuettel]

but there are C++ tests which testhat runs which I do not think can be run in tinytest.

News to me, and I converted a large number (of my, mostly) packages that way. This one is @astamm's package though and he seems to be more firmly in the other camp.

[aadler]

@eddelbuettel I wanted to convert to tinytest when I wrote the 600 or so unit tests (ok, exaggeration, but it felt like 600!), but I did not know how to convert the files below. I'd be glad to have your expertise on the issue!!

https://github.com/astamm/nloptr/blob/master/tests/testthat/test-cpp.R
https://github.com/astamm/nloptr/blob/master/src/test-C-API.cpp
https://github.com/astamm/nloptr/blob/master/src/test-C-API.h

[eddelbuettel]

For the first (R) file, I skip all the time in tinytest. Picking randomly:

https://github.com/RcppCore/RcppArmadillo/blob/11ce18a1bf16e7016584f640801c49251421d81e/inst/tinytest/test_sparse.R#L20

(The true beauty of tinytest is that each file is a standard R script. So whatever you can do in R, you can do in tinytest.)

The other two I have to long at maybe longer than the current time of day (or night) permits, or maybe you help me but by highlighting a specific aspect. But generally speaking 'Turing-equivalence' still holds: if we can test it in one, we can test it in the other. We are talking C(++) code here, the whole point of Rcpp and alike is to make it possible to access C++ code from R. That said, I just pondered that this eve for another new (still tiny) project of a new header package ...

[astamm]

I was not aware of the tinytest package which indeed seems like a nice lightweight unit testing framework with no additional dependencies. I am happy to move away from testthat in favour of this one. As @eddelbuettel mentions, there is probably a way to test the C++ functions by calling the corresponding R functions from R. I can take a look at that next week.

[eddelbuettel]

Strong endorsement of tinytest. For combining with C++ tests I tend to just do Rcpp::sourceCpp("cpp/file.cpp"). For me tinytest is a game changer as the test files are just scripts you can with Rscript (and I often add library(tinytest); library(packageThisIsPartOf) to make them self-sufficient. Mark sometimes posts charts, the adoption among CRAN packages has been good.

That said, it's not a religious war. If you like testthat better you may have your reasons. I happen to find testthat 'heavier' (and use it quite a bit in a for-work package maintained by a colleague). As always, YMMV.

[aadler]

@eddelbuettel, I'll put it on my todo list. Unfortunately, simply deleting the cpp files gave a whole host of errors when trying to compile. There must be some definitions that are necessary for the rest of the package, but nothing I could see immediately. The non-C tests should be simple to move, except for those where I relied on snapshots due to the complicated nature of the output. I'll have to figure out a way to replicate those.

[astamm]

I was wondering the same thing, that is, what about snapshot tests with tinytest. For the C tests, I can remove them for the time being and we will use the Rcpp::sourceCpp() strategy when the transition will reach the end.

[eddelbuettel]

Agreed. I have not had time to dig through the example file Avi listed in detail but ex ante a simple Rcpp::sourceCpp("cpp/testfile.cpp") should and give us testable entry points against what we want to test here. (And as we can call the C API from C++ there is really no limitation.) Happy to help on concrete questions there are any.

[aadler]

I've started porting the tests here: https://github.com/aadler/nloptr/tree/devel, but I've run into the following bottleneck. It seems that testthat is "greedy" in that it will run even if I moved the tests to tinytest. It doesn't allow me, for example, to use "expect_match" and if I preface that with tinytest::, the test complains. However, if I delete mention of testthat and appropriate files and try compile, I get a GAZILLION errors of the form:

C:\rtools43\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/rtools43/x86_64-w64-mingw32.static.posix/lib/libnlopt.a(global.cc.obj):global.cc:(.text+0xc5e): undefined reference to `operator delete[](void*)'
C:\rtools43\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/rtools43/x86_64-w64-mingw32.static.posix/lib/libnlopt.a(global.cc.obj):global.cc:(.text+0xc81): undefined reference to `operator delete[](void*)'
C:\rtools43\x86_64-w64-mingw32.static.posix\bin/ld.exe: C:/rtools43/x86_64-w64-mingw32.static.posix/lib/libnlopt.a(global.cc.obj):global.cc:(.text+0xc89): undefined reference to `operator delete(void*)'

A bit of digging shows that this is due to having C++ code somewhere in the mix but only linking with gcc and not g++. WHen testthat reigns, it calls g++ itself. I do not know how to fix that.

I will restore the testthat regime for te time being, but would request that someone who knows C/C++ better than I do (@eddelbuettel , @jyypma , @astamm ) take a look and see how to gracefuly extract testthat without causing the build to fail. @jyypma may know best where C++ is "hidden" in the code, but as of now, it's beyond me. Thanks!

[aadler]

I got around the complaint issue by converting the expect_match into its components: expect+true(grepl(…)). However, it still seems that the nlopt library on my machine has C++ in it, and requires linking via g++, but once I erase all the .cpp files (which are only used for testthat, the build process defaults to linking with gcc and it fails. If anyone would like to fork my devel branch and figure out how to gracefully extract testthat without the build failing, I would greatly appreciate it.

[astamm]

Prior to using catch for testing C and C++ files, there was an empty dummy.cpp file under src/ directory (see https://github.com/astamm/nloptr/tree/91049e25218b1322c92ec4d8f3290ee8e554f794/src) the purpose of which is to ensure linking with C++. So I think adding it back will solve the issue. I removed it at the time because I added real cpp files for the testing infra.

And to make sure to extract all things catch-related, it might be good to remove everything I added in c685e96.

[aadler]

Beautiful, @astamm, that did it!

Unfortunately, total coverage now dropped to 96.32 (which is still excellent!) as check.derivatives went from 100 to 80. That's due to the loss of snapshot, but I may be able to bring it back up by adding some more focused tests. nloptr.c is stil over 90% covered. The R-based test suite must be triggering everything that the cpp file would have tested, so we don't need it. What's still uncovered are basically the error messages I cannot get to trigger, such as Error: nlopt_set_vector_storage returned NLOPT_INVALID_ARGS. or case NLOPT_FORCED_STOP: I haven't figured out how to successfully malform an nloptr object just enough to go through and be caught in C and not R. I'll keep working and let y'all know when I'm ready for review and a PR.

[astamm]

Awesome, thanks Avi!

[aadler]

OK, I think I worked out all the bugs. testthat is gone, replaced by tinytest. Coverage is a tad higher now as I was able to get everything to 100% except nlopt.c, as explained above. I also linted all the files so that they are more consistent. I also ran a spell checker on the package which caught a few items. I also gently updated the vignette. After a couple of mishaps on my part, my devel branch passes all 417 unit tests (yes, I wrote a few new ones), passes check --as-cran on my Windows machine, and passes all the Github actions except pkgdown, which it never does as I don't have that set up under my personal repositories.

What now, @astamm? I can merge it to my master and create a pull request, or I can wait until you or @eddelbuettel want to review it while it's still safely in my repo 😆 .

Your call!

[astamm]

Marvelous. Thanks a lot @aadler ! I am more than happy to receive directly your PR and merge it with the main master. Very nice work! And thanks also for the vignette update. Much appreciated!

[astamm]

This should be resolved by #137 even though the original issue was with the catch library.