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

bridge: only use cpufeatures on iOS #565

Closed
wants to merge 1 commit into from
Closed

bridge: only use cpufeatures on iOS #565

wants to merge 1 commit into from

Conversation

Kladki
Copy link
Contributor

@Kladki Kladki commented Mar 16, 2024

The cpufeatures crate only supports a limited number of architectures, and since a comment explicitly stated that this is for iOS optimization, and since the same is done for jni, I think this is a sensible change.

@Kladki
Copy link
Contributor Author

Kladki commented Mar 16, 2024

I have signed the agreement now.

@jrose-signal
Copy link
Contributor

What's the motivation for making this change? This isn't the only dependency on cpufeatures in the workspace; it's just setting a minimum version before where Apple aarch64 CPUs weren't correctly considered as having unconditional support for…something, I forget what.

@Kladki
Copy link
Contributor Author

Kladki commented Mar 18, 2024

I am the maintainer of the mautrix-signal package for Alpine Linux, which depends on libsignal. We build for many architectures, but some of them, such as riscv64 and ppc64le, are being blocked by the cpufeatures crate, since it doesn't support those architectures.

This isn't the only dependency on cpufeatures in the workspace

This is true, however, the ones that do only use them on architectures that support it, see:

https://lib.rs/crates/sha1
https://lib.rs/crates/aes
https://lib.rs/crates/argon2
https://lib.rs/crates/chacha20
https://lib.rs/crates/curve25519-dalek
https://lib.rs/crates/poly1305
https://lib.rs/crates/polyval
https://lib.rs/crates/sha2

@jrose-signal
Copy link
Contributor

A fair point. Obligatory "Signal the organization doesn't support third-party clients and use with a bridge" and "libsignal the library doesn't support direct use of the entry points in libsignal-ffi (they are even more subject to change than the non-bridge Rust crates and the Swift API built on top of them)".

You didn't quite get the iOS triple right, but we can take care of that.

@Kladki
Copy link
Contributor Author

Kladki commented Mar 18, 2024

Obligatory "Signal the organization doesn't support third-party clients and use with a bridge" and "libsignal the library doesn't support direct use of the entry points in libsignal-ffi (they are even more subject to change than the non-bridge Rust crates and the Swift API built on top of them)".

All good 👍

You didn't quite get the iOS triple right

Really? Because I checked against the rustc book and it looks correct. 🤔 Nvm, I see the correct one in the full platform list.

@jrose-signal
Copy link
Contributor

That's an iOS triple, but not the normal one, which is just aarch64-apple-ios. arm64e is the experimental variant that enables pointer tagging checks, which Apple hasn't finalized the ABI for.

@Kladki
Copy link
Contributor Author

Kladki commented Mar 18, 2024

Thanks for pointing that out, it is now fixed. 👍

@jrose-signal jrose-signal added the awaiting release Will be in the next release of libsignal label Mar 18, 2024
@jrose-signal
Copy link
Contributor

Included in v0.42.0!

@Kladki Kladki deleted the cpufeatures-ios-target branch March 29, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting release Will be in the next release of libsignal
Development

Successfully merging this pull request may close these issues.

2 participants