eddelbuettel / rcppsimdjson

Rcpp Bindings for the 'simdjson' Header Library
115 stars 13 forks source link

Clarify C++17 status #74

Closed eddelbuettel closed 3 years ago

eddelbuettel commented 3 years ago

CRAN sent us (and three others) a note:

'Writing R Extensions' says

"A requirement of C++17 or later should always be declared in the ‘SystemRequirements’ field (as well as in src/Makevars or src/Makefile) so this is shown on the package’s summary pages on CRAN or similar."

RcppSimdJson [three others removed]

do not currently do so: please add at the next update.

and while I replied that it is more varied (as we test in configure and drop down to C++11 if we must) I adda note to SystemRequirements in DESCRIPTION to express this (also mentioning the conditionality).

eddelbuettel commented 3 years ago

Suggested wording (before I go all PR on this):

SystemRequirements: A C++17 compiler as an optional requirement detected in configure, with C++11 as a fallback

Good enough?

lemire commented 3 years ago

Note that the current requirement for simdjson is C++11. We worked hard to wave the C++17 requirement.

eddelbuettel commented 3 years ago

I am, as you know, aware of that. What would help here is how to say that for CRAN, esp when the package they use is (by construction) something older than your GH repo.

lemire commented 3 years ago

@eddelbuettel Yes. I understand.

eddelbuettel commented 3 years ago

I put a commit in to get it done and off everybody's plate. If it needs refining that will be easy enough to do too.

knapply commented 3 years ago

The package is not currently installable without C++17, so I'd recommend simply putting SystemRequirements: C++17. If memory serves, SystemRequirements: C++17 should also make the configure script unnecessary.

eddelbuettel commented 3 years ago

Whoops. I missed that but you are right. With C++11 hardwired:

edd@rob:~/git/rcppsimdjson(feature/check_cxx11)$ install.r 
* installing *source* package found in current working directory ...
* installing *source* package ‘RcppSimdJson’ ...
** using staged installation
** libs
ccache g++  -std=gnu++11 -I"/usr/share/R/include" -DNDEBUG  -I'/usr/local/lib/R/site-library/Rcpp/include'   -DSIMDJSON_NO_COMPUTED_GOTO -I../inst/include -fopenmp -fpic  -g -O3 -Wall -pipe -pedantic  -c RcppExports.cpp -o RcppExports.o
ccache g++  -std=gnu++11 -I"/usr/share/R/include" -DNDEBUG  -I'/usr/local/lib/R/site-library/Rcpp/include'   -DSIMDJSON_NO_COMPUTED_GOTO -I../inst/include -fopenmp -fpic  -g -O3 -Wall -pipe -pedantic  -c deserialize.cpp -o deserialize.o
deserialize.cpp:6:1: error: ‘SEXP’ does not name a type
    6 | SEXP deserialize(SEXP       json,
      | ^~~~
deserialize.cpp:85:1: error: ‘SEXP’ does not name a type
   85 | SEXP load(const Rcpp::CharacterVector& json,
      | ^~~~
deserialize.cpp: In function ‘bool exceptions_enabled()’:
deserialize.cpp:167:5: error: ‘Rcpp’ has not been declared
  167 |     Rcpp::stop("`SIMDJSON_EXCEPTIONS` not defined");
      |     ^~~~
deserialize.cpp:169:12: error: ‘SIMDJSON_EXCEPTIONS’ was not declared in this scope
  169 |     return SIMDJSON_EXCEPTIONS == 1;
      |            ^~~~~~~~~~~~~~~~~~~
make: *** [/usr/lib/R/etc/Makeconf:177: deserialize.o] Error 1
ERROR: compilation failed for package ‘RcppSimdJson’
* removing ‘/usr/local/lib/R/site-library/RcppSimdJson’
* restoring previous ‘/usr/local/lib/R/site-library/RcppSimdJson’
Warning message:
In install.packages(pkgs = f, lib = lib, repos = if (isMatchingFile(f)) NULL else repos) :
  installation of package ‘.’ had non-zero exit status
edd@rob:~/git/rcppsimdjson(feature/check_cxx11)$ 
eddelbuettel commented 3 years ago

I corrected that -- much appreciate the heads-up.

Another reason to get a new version up :)

knapply commented 3 years ago

Sorry I didn't catch the issue sooner!