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

feat: Update Crashpad and register WER handler #735

Merged
merged 28 commits into from
Sep 15, 2022
Merged

Conversation

Swatinem
Copy link
Member

Crashpad added a new WER (Windows Error Reporting) handler which needs
to be manually registered first in the Windows Registry, and then with
the crashpad client.

This depends on getsentry/crashpad#69

@Swatinem Swatinem requested review from a team July 18, 2022 11:10
@Swatinem Swatinem self-assigned this Jul 18, 2022
@Swatinem
Copy link
Member Author

Good think our CI caught this:

  • The Windows SDK used to build the projects needs to be at least version 19041 to provide the required header struct definitions.
  • Other functions that crashpad calls are available since Windows 7.

We should be able to make all this conditional depending on the exact version customers want to target.
We had a couple of requests and PRs to support XP, but officially we don’t really guarantee support, so things could potentially break at any time if people are targetting XP.

Either way, I would wait for @supervacuus to be back to help with the correct cmake and ifdefs for conditional compilation here.

Swatinem and others added 5 commits August 2, 2022 22:08
Crashpad added a new WER (Windows Error Reporting) handler which needs
to be manually registered first in the Windows Registry, and then with
the crashpad client.
@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2022

Codecov Report

Merging #735 (ef24d88) into master (58cbac4) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head ef24d88 differs from pull request most recent head 9cdd9bc. Consider uploading reports for the commit 9cdd9bc to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #735      +/-   ##
==========================================
- Coverage   83.09%   83.05%   -0.04%     
==========================================
  Files          53       53              
  Lines        5017     5017              
  Branches     1100     1100              
==========================================
- Hits         4169     4167       -2     
- Misses        737      738       +1     
- Partials      111      112       +1     

...by removing the database directory before each test's first example run. In some test-runs we get false positives, because a left-over envelop from a previous totally unrelated (and probably failed) test is picked up and sent during initialization.
@supervacuus supervacuus marked this pull request as ready for review September 14, 2022 16:45
...in the context of fast-fail crashes on Windows.
@@ -6,7 +6,7 @@


def enumerate_unittests():
regexp = re.compile("XX\((.*?)\)")
regexp = re.compile(r"XX\((.*?)\)")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see deprecation warnings with every test I run because support for regex strings without the r prefix is soonish unsupported. It requires python 3.6, though (released almost six years ago). I know we focus on compatibility, but maybe it is less complicated here, since this is only used by devs? Fine to change this in another PR.

Copy link
Member Author

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those are some nice testsuite improvements!

README.md Outdated Show resolved Hide resolved
examples/example.c Outdated Show resolved Hide resolved
@Swatinem Swatinem merged commit b0fab10 into master Sep 15, 2022
@Swatinem Swatinem deleted the feat/crashpad-wer branch September 15, 2022 14:34
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.

None yet

4 participants