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

Add visionos support #6611

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Add visionos support #6611

wants to merge 1 commit into from

Conversation

guusw
Copy link

@guusw guusw commented Nov 25, 2024

Add build support for VisionOS by adding the required feature checks. Note that vision is almost identical to iOS, except that the rust team decided to add a unique os feature "visionos"

Alternatives could be to check something like #[all(target_vendor = "apple", not(target_os = "macos"))]
to simplify all mobile devices, excluding desktop macos, let me know.

NOTE

A potential Issue it that the objc library does not have support for visionos and the library owned no longer seems to maintain the crate, we use a patched version in our project https://github.com/shards-lang/rust-objc/tree/shards-0.2.5
(based on this PR SSheldon/rust-objc#123)

I'm not sure what the preferred solution here would be:

  • Using a patched crate vendored under gfx-rs org git url?
  • Using a vendored copy of objc?

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@guusw guusw requested review from a team and crowlKats as code owners November 25, 2024 07:22
@nical
Copy link
Contributor

nical commented Nov 25, 2024

A few cosmetic adjustments are needed to pass CI.

Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

The question of cargo fmt notwithstanding (@guusw: I wasn't able to push to this PR; did you check the checkbox to let maintainers edit this PR?), this sounds like a difficult PR to merge because of its dependencies. The OP's suggestions:

I'm not sure what the preferred solution here would be:

  • Using a patched crate vendored under gfx-rs org git url?
  • Using a vendored copy of objc?

…don't work for publishing to Crates.io (viz., cargo publish invocations will be blocked by validation that dependencies are also Crates.io crates). Unless we can get upstream to publish a new version with the PR that this is based on, I think we might have to rebase this on top of #5641, or use the alternative also in the OP:

Alternatives could be to check something like #[all(target_vendor = "apple", not(target_os = "macos"))]
to simplify all mobile devices, excluding desktop macos, let me know.

@nical
Copy link
Contributor

nical commented Nov 25, 2024

I think we might have to rebase this on top of #5641

It seems to me that getting the cfg part to work and using a version of obj-c that supports the VisionOS are orthogonal, so I don't expect a rebase to be needed before merging this.

That said, looks like using target_vendor = "apple" might be cleaner than enumerating all of the apple devices to decide when to use metal. In most places we don't appear to differentiate between desktop and mobile.

@teoxoy
Copy link
Member

teoxoy commented Nov 25, 2024

It's worth noting that not all apple OSes support Metal.

Copy link
Collaborator

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

Deno has no opinion on this change and is fine with the cargo.toml addition

@nical
Copy link
Contributor

nical commented Nov 27, 2024

After some discussions on the matrix chat we concluded that we want to list the targets the way this PR does it. So once the CI errors are fixed, this should be good to land.

@jimblandy
Copy link
Member

@guusw Could you look into the CI failures? These are MacOS failures so they do seem relevant.

Copy link
Contributor

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Up to you of course, but I'd recommend #[cfg(target_vendor = "apple")] for all Metal stuff as that also includes tvOS (and any future OS Apple might release), #[cfg(all(target_vendor = "apple", not(target_os = "macos")))] for the places where you need to single out UIKit-specific stuff (or just #[cfg(not(target_os = "macos"))] if you're already within the context of the Metal backend), and #[cfg(target_os = "ios")] for truly iOS-specific stuff.

And sure sure, #[cfg(target_vendor = ...)] might be deprecated one day, but not before a suitable alternative such as #[cfg(target_family = "darwin")] is introduced.

If you really want, you could add something like the following to explicitly say that things won't compile on watchOS:

#[cfg(target_os = "watch")]
compile_error!("Metal, and by extension WGPU, does not work on watchOS");

@teoxoy
Copy link
Member

teoxoy commented Dec 17, 2024

I don't know what happens if you try to compile wgpu for watchos (it will probably work buy fail at runtime) but given that we already have cfg aliases I don't see why we shouldn't list the ones we explicitly want to support there.

@cwfitzgerald
Copy link
Member

With apple's propensity to make new operating systems, I am inclined to just fully bucket it - just so don't have to maintain the list

@cwfitzgerald cwfitzgerald assigned cwfitzgerald and unassigned nical Dec 18, 2024
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.

8 participants