Mingw tests (32-bit and 64-bit) #1004

lemire · 2020-06-29T00:15:34Z

This adds 32-bit and 64-bit tests under mingw (GCC for Windows). I only compile and run "basictests" because it is slllllooooowwww. If we find a way to run the tests faster, we could easily enable all tests.

You apparently can't have, at the same time, both the 64-bit and the 32-bit GCC compiler. So you need (?) two distinct tests.

Some issues with mingw:

The pow function is busted.
The inttypes header is not standard compliant.

jkeiser · 2020-06-29T20:17:42Z

@lemire thanks for working so hard on this. I'd feel better with it too! And as we're seeing, to the extend simdjson can be used for language bindings, there are benefits to increased support :)

I don't think there are many libraries that try this hard for coverage, but it's a reason simdjson will stretch far (I hope).

lemire · 2020-06-29T20:18:58Z

@jkeiser It should pass now... essentially (this all works on my PC).

jkeiser

I'm fascinated at doing the real tests in github workflows! At the least it's another source of parallel CI, and at best it might be a place we can migrate more and more stuff to, as it has a massive company funding the machines.

A couple of nits and a lot more comments that don't need to be resolved, it seems good otherwise.

jkeiser · 2020-06-29T20:50:50Z

tests/basictests.cpp

@@ -69,6 +71,9 @@ namespace number_tests {
      if(ulp > maxulp) maxulp = ulp;
      if(ulp > 0) {
        std::cerr << "JSON '" << buf << " parsed to " << actual << " instead of " << expected << std::endl;
+        printf("actual: %18.18g\n", actual);


Why both this and std::cerr?

Because C++ has a terrible API for formatting numbers. I will document this choice.

tests/basictests.cpp

jkeiser · 2020-06-29T20:54:11Z

tests/basictests.cpp

@@ -152,18 +157,20 @@ namespace number_tests {
    std::cout << __func__ << std::endl;
    char buf[1024];
    simdjson::dom::parser parser;
-    for (int i = -1000000; i <= 308; ++i) {// large negative values should be zero.
+    for (int i = -307; i <= 308; ++i) {// large negative values should be zero.


Assuming this is for speed? Do you feel we have enough coverage at -307, or should it extend a little further just to test that some out of range values yield 0?

It is because the pow function in mingw is busted. But I will change the code.

Done. In the next commit, I check if it is busted, if it is, I tell the user about it.

(We try to be correct, but the standard library is not correct on mingw.)

tests/basictests.cpp

jkeiser · 2020-06-29T21:04:44Z

tests/numberparsingcheck.cpp

@@ -87,7 +87,11 @@ void found_integer(int64_t result, const uint8_t *buf) {
  char *endptr;
  long long expected = strtoll((const char *)buf, &endptr, 10);
  if ((endptr == (const char *)buf) || (expected != result)) {
+#if (!(__MINGW32__) && !(__MINGW64__))


I think we can avoid the ifdef by making a macro:

#if __MINGW32__ || __MINGW64__ #define SIMDJSON_LLD PRId64 #define SIMDJSON_LLU PRIu64 #else #define SIMDJSON_LLD "lld" #define SIMDJSON_LLU "llu" #endif fprintf(stderr, "Error: parsed %" SIMDJSON_LLD " out of %.32s, ", result, buf);

I'm fine with the 2 ifdefs we have now, but if you mess with common_defs for other reasons, this might be worth it :)

Right. Both PRIu64 and PRId64 are part of the standard so I don't know why they don't work. But at some point, you run out of steam trying to find out why something that is perfectly fine by the standard does not work.

Understandable :) I did find a stackoverflow indicating that you can get PRIu64 by including <inttypes.h>. But who knows if THAT exists in all the compilers we support ... and it's not worth spending more of your time right now.

Oh. Right. I assumed we were importing these in the main library, but I think we do not.

Thanks. (I knew of the header, I just assumed it was there already.)

No. It is there, look in the files. We have cinttypes. It is another instance where mingw is busted.

This is flat out a bug on their part.

jkeiser · 2020-06-29T21:07:28Z

tests/stringparsingcheck.cpp

@@ -12,7 +12,7 @@
 #define JSON_TEST_STRINGS
 #endif

-#ifndef _MSC_VER
+#if (!(_MSC_VER) && !(__MINGW32__) && !(__MINGW64__))


Given how many places we do this,is there a macro that makes sense and covers these three cases? Better yet, perhaps we should have a <simdjson_dirent.h> somewhere, and get rid of the if entirely :)

Eh. For now it's fine, we should really do the simdjson_dirent.h thing regardless of this patch (which suggests it should be done outside of this patch). I'll file an issue.

jkeiser · 2020-06-29T21:09:37Z

windows/getopt.h

@@ -1,4 +1,5 @@
 #ifndef __GETOPT_H__
+/** This file has been modified by D. Lemire for use in simdjson. **/


Will be happy when we get it all in cxxopts :) Thanks for leaving a changelog in the file!

Right now cxxopts is not available when git is unavailable and it is not reasonable to deactivate our tests if networking and git are unavailable.

So if we move everything to cxxopts, I think we need to bring a hard copy of cxxopts in the project. Yeah. I know it is unfortunate, but at some point, we need to build on an isolated machine.

Yeah. It's not terrible as long as we give ourselves a way to check for and update our dependencies (the real reason for a submodule).

windows/dirent_portable.h

jkeiser · 2020-06-29T21:10:38Z

singleheader/CMakeLists.txt

+# Under Windows, exectuting a bash script works, except that you cannot generally
+# do bash C:/path to my script. You need a  "mounted" path: /mnt/c/path
+
+if (BASH AND (NOT WIN32))


Ugh. We're going to just have to get rid of all of our bash scripts at some point.

Well, if we could convert it to a mounted path, it would work. There may already have a really simple way to do this.

With WSL becoming standard... I bet that people know how to do this.

jkeiser · 2020-06-29T21:13:12Z

Holy lord of knickerbockers, it takes over an hour to build and run basictests?

lemire · 2020-06-29T21:31:02Z

Holy lord of knickerbockers, it takes over an hour to build and run basictests?

I have no idea how long it takes, but it takes a really long time.

My bet is that we have some kind of quota and our quota has been exhausted.

jkeiser · 2020-06-29T21:39:40Z

That seems likely, given that there are no logs for the tests that have supposedly been running for over and hour.

lemire · 2020-06-29T22:31:18Z

tests/basictests.cpp

@@ -152,18 +155,25 @@ namespace number_tests {
    std::cout << __func__ << std::endl;
    char buf[1024];
    simdjson::dom::parser parser;
-    for (int i = -1000000; i <= 308; ++i) {// large negative values should be zero.
+
+    bool is_pow_correct{1e-308 == std::pow(10,-308)};


@jkeiser I hope you like this better.

@jkeiser Basically, elsewhere we compare the parsed value of "1e-..." against power(10,...). This works. Except if the pow function is inaccurate. Then we get a failing test... not our fault.

lemire · 2020-06-29T22:31:33Z

tests/basictests.cpp

@@ -1949,6 +1960,11 @@ int main(int argc, char *argv[]) {
  if (simdjson::active_implementation->name() == "unsupported") {
    printf("unsupported CPU\n");
  }
+  // We want to know what we are testing.
+  std::cout << "Running tests against this implementation: " << simdjson::active_implementation->name();


@jkeiser For John!

lemire · 2020-06-29T22:33:43Z

That seems likely, given that there are no logs for the tests that have supposedly been running for over and hour.

This makes GitHub actions somewhat less useful that it could be. Maybe there is a way to ask for more compute time.

…mire/mingw

lemire · 2020-06-30T00:13:05Z

@jkeiser Can you have a look at my actions. The caching does not work. Without caching, these github actions are crazily expensive. I don't understand what is wrong with my configuration.

…jlkjfdslkfsd I am tired of this crap

lemire · 2020-06-30T01:10:44Z

Ok. I figured it out. Merging.

lemire added 17 commits June 28, 2020 20:14

Let us try that...

fc465cb

Trying to trigger the inclusion of the needed function

539d4c5

Remove silly thing.

6422519

Let us try this.

8d8c80a

Try this.

ba39293

More patching.

0baa088

Patching.

617a053

Patching this up.

3cbcabc

Verbose

dd5c515

Typo.

8bca7ce

Disabling some tests.

c3794e9

Patching numerical tests.

31e647c

Some patching.

33b24f2

Being explicit

9c9ceea

Adding 64-bit tests

17e8d1e

Naming things properly

efae4d9

Removing silly addition.

1db047b

lemire changed the title ~~I'd feel better with mingw tests~~ Mingw tests (32-bit and 64-bit) Jun 29, 2020

jkeiser approved these changes Jun 29, 2020

View reviewed changes

A few Keiser fixes

cbbdbb1

lemire commented Jun 29, 2020

View reviewed changes

Daniel Lemire added 2 commits June 29, 2020 19:04

Maybe this will be faster.

d437fc3

Merge branch 'dlemire/mingw' of github.com:simdjson/simdjson into dle…

b8c536d

…mire/mingw

Daniel Lemire added 7 commits June 29, 2020 19:08

Let us try to buffer the 64-bit version too!

372ce10

Documenting

1135aa6

Let us try to build, not test

df1d183

Where is my nobel

33c9e07

Trying again

67c8091

Again

a459afa

Adding tests.

9887468

Daniel Lemire added 4 commits June 29, 2020 20:18

Again, again, always.

8925897

Maybe the threads are the problem.

3ceb719

Again, again, again gdsfsdfdsjkfdsjkfdsjflsdkjfklsd;

b6e4f66

Patching it up againffdsfdsjkfjdsklfjdsklfjdskljfkljklfdsjklfdsjklfds…

59dc76e

…jlkjfdslkfsd I am tired of this crap

lemire merged commit ccc94c9 into master Jun 30, 2020

lemire deleted the dlemire/mingw branch June 30, 2020 01:10

		@@ -1,4 +1,5 @@
		#ifndef __GETOPT_H__
		/ This file has been modified by D. Lemire for use in simdjson. /

Mingw tests (32-bit and 64-bit) #1004

Mingw tests (32-bit and 64-bit) #1004

Uh oh!

Conversation

lemire commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkeiser commented Jun 29, 2020

Uh oh!

lemire commented Jun 29, 2020

Uh oh!

jkeiser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkeiser Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lemire Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkeiser commented Jun 29, 2020

Uh oh!

lemire commented Jun 29, 2020

Uh oh!

jkeiser commented Jun 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lemire Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lemire commented Jun 29, 2020

Uh oh!

lemire commented Jun 30, 2020

Uh oh!

lemire commented Jun 30, 2020

Uh oh!

Uh oh!

lemire commented Jun 29, 2020 •

edited

Loading

jkeiser Jun 29, 2020 •

edited

Loading

lemire Jun 29, 2020 •

edited

Loading

lemire Jun 29, 2020 •

edited

Loading