Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: Enable 32bit unit test #3529

Closed

Conversation

falbrechtskirchinger
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger commented Jun 11, 2022

In addition to enabling the 32bit unit test where possible (snapshot GCC is built without multilib), I've changed the multi-header default to ON and added a single-header build to ci_test_amalgamation.

JSON_32bitTest can be set to AUTO or SAFE_AUTO (default) in addition to ON/OFF for auto-detection of compiler support. SAFE_AUTO enables the 32bit build based on the size of size_t without changing compiler/linker flags.

Closes #3524.

@falbrechtskirchinger
Copy link
Contributor Author

Meanwhile, this 32bit unit test is a real pain. Not every compiler's multilib support is created equal, as it turns out.

@nlohmann
Copy link
Owner

Meanwhile, this 32bit unit test is a real pain. Not every compiler's multilib support is created equal, as it turns out.

Wouldn't it be sufficient to just add one test case for 32 bit using the latest Clang or GCC?

@falbrechtskirchinger
Copy link
Contributor Author

My original idea was to add the 32bit test to ci.cmake instead of creating a dedicated 32bit unit test. In hindsight that would have been the easier option.

It can't be g++-latest though, as that does not have multilib support. I've now added a blacklist to the failing subsection of ci.cmake and am about to test that next.

@nlohmann
Copy link
Owner

The current approach indeed looks rather complicated.

@falbrechtskirchinger
Copy link
Contributor Author

I was trying a simpler approach and remembered what the problem with that was. We need to build the 32bit test for coverage. That's why I asked if it would be possible to collect coverage data from multiple builds.

Let me see if there's a way to get both coverage and a simpler setup without modifying the coverage test itself.

@nlohmann
Copy link
Owner

For the coverage job we use GNU 9.4.0 which should have multilib support. Wouldn't it be sufficient to just set a flag like -DJSON_32bitTest=ON for that job and use it to guard that one 32-bit test case with fixed -m32 flag?

@falbrechtskirchinger
Copy link
Contributor Author

I would have really liked to include the 32bit test with sanitizer builds and MSVC (accomplished via SAFE_AUTO). The complicated bit right now is AUTO, which invokes the compiler and checks if it was either configured with m32 (GCC) or if running it with -m32 --version has a zero exit code.

The overall simpler approach would remove the dedicated 32bit unit test, keep the 32bit tests behind sizeof(size_t) == 4 checks in the existing unit tests, and then for coverage would build the entire test suit twice (once with -m32).
That would remove a big chunk of the CMake code (including building unit.cpp for 32bit), at the expense of longer compile times for ci_test_coverage and a bit more CMake code in ci.cmake to build with -m32.
The downside of this is either fewer configurations tested, or much longer compile times. (MSVC is still covered, but no sanitizer builds.)

Otherwise, we could leave things as-is, and add the remaining failing compilers to the blacklist until CI passes.

@nlohmann
Copy link
Owner

Given that 32 bit builds are quite a niche, I wouldn't want to spend too much runtime on this with every future build. Or am I overlooking anything?

@falbrechtskirchinger
Copy link
Contributor Author

I agree, and the current setup avoids that by only building a tiny test but for many configurations. This should be a lot faster than building everything once for 32bit.

@falbrechtskirchinger
Copy link
Contributor Author

I think I have found a workable hybrid. Shouldn't take long.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants