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(apple): support disabling metal validation #2373

Merged
merged 13 commits into from
Jan 28, 2025

Conversation

Saadnajmi
Copy link
Collaborator

@Saadnajmi Saadnajmi commented Jan 22, 2025

Description

A couple of repositories (react-native-webgpu, @shopfiy/react-native-skia) need to have Metal Validation turned off for their libraries to run (and be tested in CI. Let's add a way to enable/disable that from the manifest.

Note:

  • For whatever reason, when enableGpuValidationMode = "1" is in the xcscheme, that actually means that Metal API Validation is off. To turn it back on, you can't set it to "0", you have to remove the line. 🤷‍♂️

Platforms affected

  • Android
  • iOS
  • macOS
  • visionOS
  • Windows

Test plan

Added and removed the key from the example apps' App.json, re-ran pod install and verified that Metal API Validation would flip.

@Saadnajmi Saadnajmi marked this pull request as draft January 22, 2025 05:20
@Saadnajmi
Copy link
Collaborator Author

@wcandillon FYI

@wcandillon
Copy link

wcandillon commented Jan 22, 2025

this is very cool 😎☺️ Thank you for doing this.

ios/test_app.rb Outdated Show resolved Hide resolved
@Saadnajmi Saadnajmi marked this pull request as ready for review January 26, 2025 06:09
@Saadnajmi Saadnajmi changed the title feat(ios): support disabling metal validation feat(apple): support disabling metal validation Jan 26, 2025
@Saadnajmi
Copy link
Collaborator Author

Saadnajmi commented Jan 26, 2025

Not obvious to me: Not every key in the app.json seems tested. Should I add some sort of test, or leave it as is?

@Saadnajmi
Copy link
Collaborator Author

Oh, another note. Using REXML removes the newlines from the xcscheme so the structure of the file is different, but it's still usable by Xcode. Just thought I'd make sure to mention that.

docs/ios.metalAPIValidation.md Outdated Show resolved Hide resolved
docs/ios.metalAPIValidation.md Show resolved Hide resolved
ios/xcode.rb Outdated Show resolved Hide resolved
@tido64
Copy link
Member

tido64 commented Jan 27, 2025

Not obvious to me: Not every key in the app.json seems tested. Should I add some sort of test, or leave it as is?

I would really appreciate it if you added tests.

Saadnajmi and others added 2 commits January 27, 2025 05:26
Co-authored-by: Tommy Nguyen <[email protected]>
Saadnajmi and others added 2 commits January 27, 2025 06:45
@Saadnajmi
Copy link
Collaborator Author

Not obvious to me: Not every key in the app.json seems tested. Should I add some sort of test, or leave it as is?

I would really appreciate it if you added tests.

I looked around the repo and I'm not sure I am coming up with a good test.. mostly because to practically test you need "pod install" to run, and/or a screenshot of Xcode with the scheme open

Copy link
Member

@tido64 tido64 left a comment

Choose a reason for hiding this comment

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

✅ Verified that enableGPUValidationMode='1' gets written

Thanks for adding this feature ❤️

schema.json Outdated Show resolved Hide resolved
@tido64 tido64 enabled auto-merge (squash) January 28, 2025 09:04
@tido64 tido64 merged commit 5dbcab9 into microsoft:trunk Jan 28, 2025
30 checks passed
@Saadnajmi Saadnajmi deleted the metal-validation branch January 28, 2025 14:00
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.

3 participants