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

Change logic to disable OpenXR for iOS #781

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

jeyum2
Copy link
Contributor

@jeyum2 jeyum2 commented Jun 26, 2024

Due to target_os = ios not working when cross-platform building,
use CARGO_CFG_TARGET_OS to check the target OS instead.

@Bromeon Bromeon added bug c: tooling CI, automation, tools labels Jun 26, 2024
@jeyum2
Copy link
Contributor Author

jeyum2 commented Jun 26, 2024

Tested on

[Host]
M2 Macbook Pro, macOS 14.2

[Target]
iPhone 14, iOS 17.5.1

@Bromeon
Copy link
Member

Bromeon commented Jun 26, 2024

Thanks a lot!

Maybe someone with macOS can help review this, e.g. @Ughuuu?

Comment on lines 91 to 101
let target_os = std::env::var("CARGO_CFG_TARGET_OS").unwrap_or_else(|_| "unknown".to_string());
if godot_ty.starts_with("OpenXR") {
return true;
if target_os == "ios" {
return true;
}
if target_os == "macos" {
#[cfg(before_api = "4.2")]
return true;
}
}
Copy link
Member

@Bromeon Bromeon Jun 26, 2024

Choose a reason for hiding this comment

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

I think the let could be moved inside if.

Also, you might want to use match against Some("ios"), Some("macos") and None -- then you wouldn't need the "unknown" case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated with match statement

Due to `target_os = ios` not working when cross-platform building,
use `CARGO_CFG_TARGET_OS` to check the target OS instead.
@Ughuuu
Copy link
Contributor

Ughuuu commented Jun 26, 2024

@Bromeon there isn't much code I am familiar with here, looks good to me tho.

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@Bromeon Bromeon added this pull request to the merge queue Jun 26, 2024
Merged via the queue into godot-rust:master with commit 4dc2720 Jun 26, 2024
15 checks passed
@jeyum2 jeyum2 deleted the fix-openxr-ios branch June 27, 2024 00:03
@Bromeon Bromeon added the c: ios iOS export target label Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: ios iOS export target c: tooling CI, automation, tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants