CI: Enable 32bit unit test (simplified) #3530
Conversation
| @@ -577,6 +584,13 @@ add_custom_target(ci_test_amalgamation | |||
| COMMAND ${ASTYLE_TOOL} ${ASTYLE_FLAGS} ${INDENT_FILES} | |||
| COMMAND cd ${PROJECT_SOURCE_DIR} && for FILE in `find . -name '*.orig'`\; do false \; done | |||
|
|
|||
| COMMAND CXX=${GCC_TOOL} CXXFLAGS="${GCC_CXXFLAGS}" ${CMAKE_COMMAND} | |||
There was a problem hiding this comment.
Why did you add this to ci_test_amalgamation?
There was a problem hiding this comment.
It seemed related. Amalgamation should be the only reason why the single-header could possibly behave differently than the multi-header version.
There was a problem hiding this comment.
But the test only aims at the structure. The goal is not to detect different behavior, but is to make sure changes are applied to both the include and the single_include folder.
There was a problem hiding this comment.
I guess I just wanted to avoid adding another job to the workflow.
Look forward to an issue (and PR) on json-ci soon. Preview:
The
json-ciimage is about 14GB in size. The time spent initializing containers is 170 seconds (average over ~350 jobs) or almost 3 minutes. That amounts to a ridiculous 2.5 hours per workflow run.
There was a problem hiding this comment.
Oh I so agree! We need to cut off the easy targets from the harder ones. I guess 2-3 Debian images could be sufficient to cover 90% of the compilers. Then we'd only need 1 image for the bleeding edge stuff and the specific tools.
There was a problem hiding this comment.
Do you want a test for the single-header at all?
There was a problem hiding this comment.
Good point. So far, we tested it by chance somewhere, because we did not switch on the multiple-header version in all of the tests. Now that we have the multiple-header version as default, a dedicated test for the amalgamated version should be in order.
| if(NOT "${json_32bit_test}" STREQUAL ONLY) | ||
| # test legacy comparison of discarded values | ||
| json_test_set_test_options(test-comparison_legacy | ||
| COMPILE_DEFINITIONS JSON_USE_LEGACY_DISCARDED_VALUE_COMPARISON=1 |
There was a problem hiding this comment.
Why does this depend on json_32bit_test?
There was a problem hiding this comment.
It depends on JSON_32bitTest=ONLY. No other tests should be built. Technically, that also applies to the CMake tests below.
There was a problem hiding this comment.
This feels over-engineered to me. Why do we need the flag in the first place?
Wouldn't it be sufficient to check, inside the coverage target, to add the 32 bit unit test with -m32 flag?
There was a problem hiding this comment.
It's not possible to "just" build a single unit test with -m32 because that also needs the shared code (test_main) built with -m32. That's what I wanted to avoid in this "simplified" version. Instead, we only build the 32bit test. But I do have another idea. Just add a return() to the CMakeLists.txt and skip everything else in the file. I'll check if that's possible.
Replaces #3529.