getsentry / sentry-native

Sentry SDK for C, C++ and native applications.
MIT License
404 stars 170 forks source link

fix: look up `GetSystemTime()` implementations at runtime. #1051

Closed supervacuus closed 1 month ago

supervacuus commented 1 month ago

This is a fix for the issue raised here: https://github.com/getsentry/sentry-native/pull/1039#issuecomment-2398218262

The problem is that the previous build-time condition only checks if the SDK supports the new API, but the application could later run on a Windows without support.

We look up GetSystemTimePreciseAsFileTime() and runtime (during sentry_init, at which point it is cached) and fall back to GetSystemTimeAsFileTime() if we can't find it.

@dacap and @ShawnCZek, please look to see if that works for you.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 81.76%. Comparing base (21c6d0a) to head (331f8fc). Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1051 +/- ## ========================================== - Coverage 81.81% 81.76% -0.05% ========================================== Files 53 53 Lines 6363 6363 Branches 1207 1207 ========================================== - Hits 5206 5203 -3 - Misses 1045 1047 +2 - Partials 112 113 +1 ```
supervacuus commented 1 month ago

This works perfectly fine for me. Thank you for the quick action!

Great!

I am just surprised there is an incentive to support such an old OS

I don't particularly appreciate breaking changes (and Sentry agrees). If the effort of implementing and maintaining a fallback is within acceptable bounds and the 99 percentile users are not affected by it, then there is no reason to break.

dacap commented 1 month ago

@supervacuus thanks for this patch, I've just tested 331f8fc91004a716af7a9136314420d8ffc8ea18 on Windows 7 and it worked 👍