eddelbuettel / rcppsimdjson

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

Odd-looking macOS errors #6

Closed eddelbuettel closed 4 years ago

eddelbuettel commented 4 years ago

So we've been on CRAN for five days now, and even though we clearly to 'unix-only' (via OS_type: unix) and clearly state C++17, some try and fail, see
https://cloud.r-project.org/web/checks/check_results_RcppSimdJson.html

I am not too concerned about the Solaris-x86 error---that is a pretty rare setup---but I am a little confused about the macOS failures. Ie

    > ### Name: validateJSON
    > ### Title: Validate a JSON file, fast
    > ### Aliases: validateJSON
    >
    > ### ** Examples
    >
    > jsonfile <- system.file("jsonexamples", "twitter.json", package="RcppSimdJson")
    > validateJSON(jsonfile)
    The processor is not supported by simdjson.
    Error in .validateJSON(jsonfile) : Uninitialized
    Calls: validateJSON -> .validateJSON
    Execution halted

IS that failing the chipset test? Doesn't macOS run on recent-ish Intel chips?

@lemire Any guidance you can offer would be much appreciated.

knapply commented 4 years ago

Friendly FYI: it passes on Mac via Github Actions here.

"macOS-latest" is macOS Catalina 10.15.

eddelbuettel commented 4 years ago

Well that is good to know, but it doesn't help with the CRAN issue at hand. If we fail to build there they may decide we are not worthy and toss us off. Are you saying the CRAN setup is behind?

(Apologies for the ignorant musings; I am somewhat blissfully ignorant of all things macOS as stuff is changing and breaking way too fast for me to bother with it.)

knapply commented 4 years ago

I'm also ignorant with Macs, but it seemed a helpful way to hack on GH workflows.

The check flavors page note that both the failing mac versions use clang 4.0.0.

It turns out that simdjson needs "clang 6 or better".

eddelbuettel commented 4 years ago

Nice find, much appreciated! Will document in DESCRIPTION.

lemire commented 4 years ago

Sorry for the delay.

We should support the processor when it is x64. The supports goes back to Westmere... For all practical purposes, that should be as far back as you need to go. People with older processors probably have other issues in mind than trying to get as much performance as possible out of their expensive hardware.

Regarding Xcode (macOS), anything from the last two major releases should be good enough. For all practical purposes, that should be good enough. Note that I am one of the maintainers of simdjson and I work primarily on macOS... so we do support macOS. :-)

lemire commented 4 years ago

Solaris is something else... I don't think we can commit to supporting Solaris.

eddelbuettel commented 4 years ago

Sounds good. I think I may add something to autoconf to check the g++ and clang++ versions.

So clang++ 6, or g++ 7, is what we should test for?

lemire commented 4 years ago

So clang++ 6, or g++ 7, is what we should test for?

It should support any compiler that has sufficient C++17 support and that includes clang++ 6.0 or better as well as GNU GCC 7 or better. It also includes the Intel compiler, Microsoft Visual Studio...

I would expect that you simply should not be able to build with older compilers. That is, my expectation is that you won't get non-functioning code, you will just get a build error of some kind.

eddelbuettel commented 4 years ago

Yes indeed. I wasn't too clear about my intentions. On CRAN we often care about a known there set of compilers, see https://cloud.r-project.org/web/checks/check_results_RcppSimdJson.html

So we could flag the older clang (as noticed by @knapply above) but in general err on the side of caution because some may of course come along with a working compiler we do not know about.

eddelbuettel commented 4 years ago

Actually, I should probably just check for the C++ standard itself.

lemire commented 4 years ago

If you can do that, it would be best.

eddelbuettel commented 4 years ago

autoconf has this: https://www.gnu.org/software/autoconf-archive/ax_compiler_version.html#ax_compiler_version

But I think I answered that myself on StackOverflow once, there is a standard #define for the standard. I just have to go and dig...

lemire commented 4 years ago

It might be as simple as this... https://stackoverflow.com/questions/38456127/what-is-the-value-of-cplusplus-for-c17

See first accepted answer, final part.

eddelbuettel commented 4 years ago

Yep, indeed. My bad for remembering / imagining a #define, but same gist. That's what I was thinking of.

eddelbuettel commented 4 years ago

Came up with something much cuter and cleverer. Thanks to all the wrapping from Rcpp, I can just use

// [[Rcpp::export(.cppVersion)]]
int cppVersion() {
  return __cplusplus;
}

to create a (package-local) function .cppVersion() and I now flag on package startup (politely, with a suppressable Message) via R/init.R if the version is too low. Also protected the main worker call so now the package will build with, say, g++-4.7 but be lobotomized.

Normal functioning continues of course unchanged.

SymbolixAU commented 4 years ago

For what it's worth, my 2009 Mac (gulp!) at home, running El Capitain (or it might be sierra), supports the right compilers and C++17, but I get the execution halted error.

The processor is not supported by simdjson.
    Error in .validateJSON(jsonfile) : Uninitialized
    Calls: validateJSON -> .validateJSON
    Execution halted
eddelbuettel commented 4 years ago

That is the CRAN version, correct?

Would you mind checking with master which should say more, including a warning on package load.

lemire commented 4 years ago

@SymbolixAU You definitively should get an unsupported cpu error...

dcooley commented 4 years ago

@eddelbuettel @lemire sure enough

library(RcppSimdJson) This package requires a recent CPU type and supports several models but not the one on this machine. You will likely experience failure when trying to parse JSON documents.

eddelbuettel commented 4 years ago

Perfect. That is on load. Upon calling validateJSON() you should now get the improved message thanks to better upstream diagnostics.

I will likely ship this once I get back home -- just got to Copenhagen for the 20 years of R 1.0.0 "celebRtion" workshop.

SymbolixAU commented 4 years ago

As an aside (which I may turn into another issue after I do some more test), I get segfaults whenever I try to call next() (https://github.com/lemire/simdjson#navigating-the-parsed-document) on a ParsedJSON object.

lemire commented 4 years ago

@SymbolixAU After successfully parsing a document... or after failing to parse the document?

Can you provide reproducible steps to get a segmentation fault?

SymbolixAU commented 4 years ago

@lemire after successfully parsing. Yes I will keep testing and make an example if I can't solve it.

lemire commented 4 years ago

@SymbolixAU

Ok. So next is a very specific function with relatively limited uses. It cannot be used to visit the whole document. It is only within a given scope. And you should check the boolean returned value and stop when it returns false. It still should not segfault.

lemire commented 4 years ago

(My previous comment was poorly phrased. Edited.)

eddelbuettel commented 4 years ago

This can be closed now under 0.0.3.