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

Update pqcrypto-internals build.rs to support iOS SDK 18.1 #72

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

gridbox
Copy link
Contributor

@gridbox gridbox commented Jan 2, 2025

I was still experiencing sha3 error even with version 0.2.7. I noticed the OP in #68 was using iOS SDK 17.2. I have since updated to Xcode 16 which is at iOS SDK 18.1 and the build flag did not fix it. However, through some searching I found that updating the build flag to -march=armv8-a+sha3 seems to work.

Unfortunately, I'm not using the library directly but it is an indirect dependency of another dependency that I am using to build a Flutter mobile app backed by a custom Rust library. I'm not sure if my fix is universal or is just appropriate for my specific use case.

LukasPukenis added a commit to NordSecurity/libtelio that referenced this pull request Jan 9, 2025
Darwin runners fail after the update of this crate
which is a transient dependency. Using `[patch]` doesn't
update properly so this is a manual attempt.

There's outstanding PR which claims to fix the issue:
rustpq/pqcrypto#72

The error in question from compilation:
```
cargo:warning=<instantiation>:79:5: error: instruction requires: sha3
```

Signed-off-by: Lukas Pukenis <lukas.pukenis@nordsec.com>
LukasPukenis added a commit to NordSecurity/libtelio that referenced this pull request Jan 9, 2025
Darwin runners fail after the update of this crate
which is a transient dependency. Using `[patch]` doesn't
update properly so this is a manual attempt.

There's outstanding PR which claims to fix the issue:
rustpq/pqcrypto#72

The error in question from compilation:
```
cargo:warning=<instantiation>:79:5: error: instruction requires: sha3
```

Signed-off-by: Lukas Pukenis <lukas.pukenis@nordsec.com>
@thomwiggers thomwiggers enabled auto-merge (squash) January 19, 2025 15:18
auto-merge was automatically disabled January 24, 2025 13:47

Head branch was pushed to by a user without write access

@SamuelScheit
Copy link

SamuelScheit commented Jan 28, 2025

@gridbox Thank you very much for this fix, this worked for me and fixed the compiling issue when trying to build for aarch64-apple-ios

Copy link
Member

@thomwiggers thomwiggers left a comment

Choose a reason for hiding this comment

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

I'm going to re-do this in a quick PR of my own, but thanks for the suggestion and sorry for taking a while to get to testing this.

pqcrypto-internals/build.rs Outdated Show resolved Hide resolved
@thomwiggers thomwiggers enabled auto-merge (squash) January 28, 2025 14:10
@thomwiggers thomwiggers merged commit b399374 into rustpq:main Jan 28, 2025
3 checks passed
@charlag
Copy link
Contributor

charlag commented Feb 3, 2025

Can we get a release with this fix at some point please? We had to pin the version to a commit again

@charlag
Copy link
Contributor

charlag commented Feb 3, 2025

Also it seems like it breaks the build for Android aarch64

@thomwiggers
Copy link
Member

If you could contribute a test case that covers cross-compiling for Android, that would be great. I don't have a lot of time to work on this project.

@thomwiggers
Copy link
Member

The tricky thing about what we're trying to build in pqcrypto-internals, is that we're trying to compile some ARMv8 code that relies on extensions (namely SHA3) that are not necessarily present on all ARMv8 cores — runtime feature detection is supposed to catch this but clearly some compilers are being picky.

@charlag
Copy link
Contributor

charlag commented Feb 3, 2025

It's totally understandable, thank you for your effort and the quick reply.

How would you like to see the test case? A Github workflow or something else?

@thomwiggers
Copy link
Member

This should likely just be integrated into the Github workflow, I don't think we could specify build flags otherwise.

@vaf-hub
Copy link

vaf-hub commented Feb 3, 2025

Here's a quick fix that fixes the problem for android: #74

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.

5 participants