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: add support for Xbox Series X/S. #1100

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

supervacuus
Copy link
Collaborator

fixes #1068.

Copy link

github-actions bot commented Dec 9, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 10c73c5

Copy link

codecov bot commented Dec 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.59%. Comparing base (19ba6d5) to head (10c73c5).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1100      +/-   ##
==========================================
+ Coverage   82.57%   82.59%   +0.02%     
==========================================
  Files          53       53              
  Lines        7889     7889              
  Branches     1232     1232              
==========================================
+ Hits         6514     6516       +2     
+ Misses       1263     1261       -2     
  Partials      112      112              

Copy link
Member

@JoshuaMoelans JoshuaMoelans left a comment

Choose a reason for hiding this comment

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

LGTM (didn't inspect cmake files in detail, as discussed in DMs)

CMakeLists.txt Show resolved Hide resolved
Copy link
Collaborator

@PlasmaDev5 PlasmaDev5 left a comment

Choose a reason for hiding this comment

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

All look good to me.
Any improvements i had already have pending reviews to be moved over to this repo after this merge has gone in.

@bruno-garcia
Copy link
Member

All look good to me. Any improvements i had already have pending reviews to be moved over to this repo after this merge has gone in.

Reminder that this is the public SDK so only changes that rely on public GDK or public samples repo can be added here.

set(CMAKE_TRY_COMPILE_PLATFORM_VARIABLES XdkEditionTarget BUILD_USING_BWOI)

#--- Windows SDK
set(SDKVersion 10.0.19041.0)

Choose a reason for hiding this comment

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

According to the GDK documentation, Windows SDK version 10.0.22000+ is required as of October 2023 GDK: https://learn.microsoft.com/en-us/gaming/gdk/_content/gc/getstarted/overviews/sdk-and-tools#windows-software-development-kit-sdk

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a follow up change that will auto detect the latest GDK and Windows SDK versions. I believe its just pending the merge of this change

Choose a reason for hiding this comment

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

I'm more surprised this even builds with what should be the "wrong" version of the Windows SDK. Thanks for the heads up about the follow-up change!

@supervacuus supervacuus force-pushed the feat/mainlining_xbox_support branch from 4699d94 to 10c73c5 Compare December 19, 2024 12:04
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.

Add GDK support to sentry-native
5 participants