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 apple tvos support #2169

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

lcruz99
Copy link
Contributor

@lcruz99 lcruz99 commented Oct 10, 2023

What does this PR do

This PR allows using nix lib to target tier 3 *-apple builds:

  • Apple tvOS on aarch64
  • Apple tvOS Simulator on x86_64

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API

@lcruz99 lcruz99 force-pushed the add_apple_tvos_support branch 4 times, most recently from f883991 to e734ffc Compare October 10, 2023 12:30
@lcruz99
Copy link
Contributor Author

lcruz99 commented Oct 10, 2023

tvOS build is failing because parking_lot 0.12.0 doesn't yet support this target, next release will do though.

Will drop CI job commit.

@lcruz99 lcruz99 force-pushed the add_apple_tvos_support branch from e734ffc to 4efe691 Compare October 10, 2023 12:43
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Instead of adding more code to support tvos, how about we remove code? By replacing every instance of:
#[cfg(any(target_os = "macos", target_os = "ios"))]
with
#[cfg(target_vendor = "apple")].
I think that should work.

src/sys/event.rs Outdated
#[cfg(any(target_os = "macos", target_os = "ios", target_os = "freebsd"))]
#[cfg(any(
target_os = "macos",
target_os = "ios",
Copy link
Member

Choose a reason for hiding this comment

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

Should there be a tvos here?

@SteveLauC
Copy link
Member

Instead of adding more code to support tvos, how about we remove code? By replacing every instance of: #[cfg(any(target_os = "macos", target_os = "ios"))] with #[cfg(target_vendor = "apple")]. I think that should work.

Will target_vendor = "apple" contain the watchos? I didn't find any doc on what this target_vendor will be but per the Platform Support doc, watchos is also one of the Apple OSes that Rust supports

Another thing I found while searching is that the Rust team will deprecate the target_vendor attribute in the future, though I believe it is still usable for now

@asomers
Copy link
Member

asomers commented Nov 5, 2023

Another thing I found while searching is that the Rust team will deprecate the target_vendor attribute in the future, though I believe it is still usable for now

That is highly annoying. And it looks like the proposed target_family = apple doesn't exist yet. So deprecating target_vendor is a real step backwards in terms of usability. I wonder, can we create a build.rs script that defines our own cfg variable? Something like #[cfg(apple)] when building for any apple target?

@SteveLauC
Copy link
Member

I wonder, can we create a build.rs script that defines our own cfg variable? Something like #[cfg(apple)] when building for any apple target?

There is a crate cfg_aliases, which seems to be exactly what we want:

// in our `build.rs`

use cfg_aliases::cfg_aliases;

fn main() {
    cfg_aliases! {
        ios: { target_os = "ios" },
        macos: { target_os = "macos" },
        watchos: { target_os = "watchos" },
        tvos: { target_os = "tvos" },
        apple_targets: { any(ios, macos, watchos, tvos) },
    }
}

Then we can use cfg(apple_targets) to conditionally compile for these apple targets

And it seems to be compatiable with the cfg attributes from std, we can mix them in our code:

// For Apple targets and Linux 
#[cfg(any(apple_targets, target_os = "linux")]
fn foo() { }

@asomers
Copy link
Member

asomers commented Nov 6, 2023

Awesome @SteveLauC that looks perfect

@SteveLauC
Copy link
Member

Friendly ping @lcruz99, would you like to do it in this PR:

  1. Add that code snippet to build.rs
  2. Replace #[cfg(any(target_os = "macos", target_os = "ios"))] with #[cfg(apple_targets)]

Please note that this would also make nix support watchos as well


If I do this, then this PR would be replaced by my PR, and I don't want to ruin your first contribution to nix :)

@lcruz99
Copy link
Contributor Author

lcruz99 commented Nov 8, 2023

@SteveLauC will push those changes asap, thanks for the heads up :)

@lcruz99 lcruz99 force-pushed the add_apple_tvos_support branch 2 times, most recently from 17f5c24 to a426093 Compare November 8, 2023 14:36
@lcruz99 lcruz99 force-pushed the add_apple_tvos_support branch from ad35f0a to bbcee88 Compare November 8, 2023 14:43
@lcruz99
Copy link
Contributor Author

lcruz99 commented Nov 8, 2023

@SteveLauC @asomers FYI

Just pushed requested changes.

@SteveLauC SteveLauC added this pull request to the merge queue Nov 9, 2023
Merged via the queue into nix-rust:master with commit 49283c9 Nov 9, 2023
68 checks passed
@SteveLauC
Copy link
Member

Thanks! And congratulations on your first contribution to nix!

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