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

fix: vsnprintf usage on macOS #3079

Merged
merged 3 commits into from
Jan 24, 2024
Merged

fix: vsnprintf usage on macOS #3079

merged 3 commits into from
Jan 24, 2024

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Jan 23, 2024

vsnprintf was used incorrectly on macOS (used the windows version) because this code was copied from sentry-unity where it didn't run on mac, only Windows and Linux.

Copy link

codecov bot commented Jan 23, 2024

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (947dac4) 76.43% compared to head (b08727e) 76.45%.

Files Patch % Lines
src/Sentry/Platforms/Native/CFunctions.cs 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3079      +/-   ##
==========================================
+ Coverage   76.43%   76.45%   +0.01%     
==========================================
  Files         351      351              
  Lines       13259    13259              
  Branches     2644     2644              
==========================================
+ Hits        10135    10137       +2     
+ Misses       2447     2446       -1     
+ Partials      677      676       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vaind vaind marked this pull request as ready for review January 23, 2024 16:42
@vaind vaind enabled auto-merge (squash) January 23, 2024 19:12
Copy link
Collaborator

@jamescrosswell jamescrosswell left a comment

Choose a reason for hiding this comment

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

Could we get some context in the PR summary for what this change is about? Does it relate to an Issue? What was the problem and how does this solve it?

@vaind
Copy link
Collaborator Author

vaind commented Jan 24, 2024

says it all in the PR title, really, but OK, I'll add the same to the description

@jamescrosswell
Copy link
Collaborator

says it all in the PR title, really, but OK, I'll add the same to the description

That's helpful. I wasn't aware that vsnprintf was windows only (I've never heard of it before) or that the code we had was copied from Unity. So the whole PR was just mysterious to me.

@vaind vaind merged commit cabcf44 into main Jan 24, 2024
23 checks passed
@vaind vaind deleted the fix/native-aot-macos branch January 24, 2024 09:21
@vaind
Copy link
Collaborator Author

vaind commented Jan 24, 2024

. I wasn't aware that vsnprintf was windows only

It's not, but we have a windows-only variant defined in the class (IIRC it has different arguments), and since the code switched on linux vs rest of the world (which was just Windows in Unity), macOS was trying to use that windows variant too.

https://github.com/getsentry/sentry-dotnet/pull/3079/files#diff-3d53f32802f41fc554a4ab868d6df5531d920abf89f38886d41e004618fed766R416

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.

2 participants