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

include <windows.h> to ensure that sentry.h is modularized correctly #935

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

hyp
Copy link
Contributor

@hyp hyp commented Jan 4, 2024

This makes it possible to use this header in Swift

Fixes #934

@hyp
Copy link
Contributor Author

hyp commented Jan 4, 2024

@Swatinem What do you think about this change? I see you previously dropped the use of windows.h in favor of wtypes.h, can we bring it back instead?

@Swatinem
Copy link
Member

Swatinem commented Jan 5, 2024

The problem is that windows.h includes the whole world, and you need a bunch of other defines to contain the blast radius a bit. We previously had some problems related to including windows.h, thats why it was removed.

@supervacuus hopefully knows more about this, and can also guide us to a solution here. Lets wait a bunch more days for people to return from winter break :-)

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (164da79) 82.65% compared to head (b2c450a) 82.65%.
Report is 1 commits behind head on master.

❗ Current head b2c450a differs from pull request most recent head 3a89a13. Consider uploading reports for the commit 3a89a13 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #935   +/-   ##
=======================================
  Coverage   82.65%   82.65%           
=======================================
  Files          53       53           
  Lines        7376     7376           
  Branches     1186     1186           
=======================================
  Hits         6097     6097           
  Misses       1171     1171           
  Partials      108      108           

@compnerd
Copy link

compnerd commented Jan 5, 2024

@Swatinem totally understandable that Windows.h is pretty broad

I think that the documentation does clearly state that you should use Windows.h most of the time (there are a few headers which can be imported without Windows.h). Adding a wrapping header to define all the control macros locally is an option that you might consider. The other possible alternative might be to start pulling in random headers in addition to this particular header (in some aspects re-creating a Windows.h as if you had defined the other macros). Unfortunately, many of the headers are not standalone nor do they include what they use.

Copy link
Collaborator

@supervacuus supervacuus left a comment

Choose a reason for hiding this comment

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

A short check with /showIncludes reveals that we indirectly include windows.h via rpc.h anyway, so I see no reason to block this. This change is not only relevant to making the Native SDK work with clang modules but also makes explicit that we depend on windows.h.

There is currently no way around this without hiding the platform-specific context types in the header via some abstraction, and this is not a priority given that we still need to expose and track machine- and platform-dependent interfaces somewhere (but it might be the right choice down the road).

So, LGTM, but please add a changelog entry. Thanks!

This makes it possible to use this header in Swift

Fixes getsentry#934
@hyp
Copy link
Contributor Author

hyp commented Jan 8, 2024

Thanks, I updated the release notes!

@hyp hyp requested a review from supervacuus January 8, 2024 23:37
@supervacuus supervacuus merged commit b8fadf9 into getsentry:master Jan 9, 2024
19 checks passed
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.

[windows] including only wtypes in sentry.h makes Swift fail to import sentry.h
4 participants