CesiumGS / cesium-native

Apache License 2.0
435 stars 214 forks source link

CI broken on new GNU version #949

Open j9liu opened 1 month ago

j9liu commented 1 month ago

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
lilleyse commented 1 month ago

I just ran into this issue locally with GCC 13.1.0.

j9liu commented 1 month ago

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

lilleyse commented 1 month ago

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

j9liu commented 1 month ago

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.

lilleyse commented 1 month ago

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

getSafeNumber returns 18446744073709551616.0 which ought to have thrown.

kring commented 1 month ago

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