cesium-native icon indicating copy to clipboard operation
cesium-native copied to clipboard

CI broken on new GNU version

Open j9liu opened this issue 1 year ago • 6 comments

Looks like CI is broken for ubuntu-latest/gcc/RelWithDebInfo on multiple branches. Have not tried it on main but I wouldn't be surprised if it failed. However, it looks like the Github runner has not changed versions; it's something to do with the build process.

I compared a working job with a failed job on the same branch and noticed this difference:

Successful:

-- The CXX compiler identification is GNU 11.4.0
-- The C compiler identification is GNU 11.4.0

Failed:

-- The CXX compiler identification is GNU 13.2.0
-- The C compiler identification is GNU 13.2.0

This new version results in this failure:

190: -------------------------------------------------------------------------------
190: JsonValue::getSafeNumber() throws if narrowing conversion error would occur
190:   2^64 - 1 cannot be converted back to a double
190: -------------------------------------------------------------------------------
190: /home/runner/work/cesium-native/cesium-native/CesiumGltf/test/TestJsonValue.cpp:44
190: ...............................................................................
190: 
190: /home/runner/work/cesium-native/cesium-native/CesiumGltf/test/TestJsonValue.cpp:46: FAILED:
190:   REQUIRE_THROWS( value.getSafeNumber<double>() )
190: because no exception was thrown where one was expected:
190: 
190: ===============================================================================
190: test cases: 1 | 1 failed
190: assertions: 4 | 3 passed | 1 failed

j9liu avatar Sep 26 '24 14:09 j9liu

I just ran into this issue locally with GCC 13.1.0.

lilleyse avatar Sep 26 '24 15:09 lilleyse

Turns out the actual culprit is an OS update -- the runner is now using ubuntu-24 instead of ubuntu-22.

I wonder if the narrowing conversion error is related to the test failures we encountered earlier with mac: #867

j9liu avatar Sep 26 '24 20:09 j9liu

Locally I got the test to pass by changing it to 2^64 - 2. Weird because both 2^64 - 1 and 2^64 - 2 are not representable as double. 🤷

https://github.com/CesiumGS/cesium-native/blob/d7fd72dec56e8d889eca6e5871563dc966b83feb/CesiumGltf/test/TestJsonValue.cpp#L42-L47

lilleyse avatar Sep 26 '24 20:09 lilleyse

Interesting... 🤔 @lilleyse could you see what getSafeNumber is returning when the test doesn't throw? (for 2^64 - 1).

In the past I've gotten weird errors with uint64_t that resulted in some weird undefined behavior in some cases. It almost souns like an overflow is happening under-the-hood that makes the value wrap around to 0 or a smaller number... but since the value is being returned by std::numeric_limits I feel like that shouldn't be right.

j9liu avatar Sep 26 '24 21:09 j9liu

Even stranger, it passes in debug mode but not release.

getSafeNumber returns 18446744073709551616.0 which ought to have thrown.

lilleyse avatar Sep 27 '24 14:09 lilleyse

Moved this out of yesterday's release because we worked around it by temporarily downgrading to Ubuntu 22.04 on CI.

kring avatar Oct 01 '24 21:10 kring

Closing as this seems to have been fixed.

azrogers avatar Jul 03 '25 19:07 azrogers