-
Notifications
You must be signed in to change notification settings - Fork 7
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
Create workaround flow and release script #10
Conversation
These should be removed when the x86_64 issue is solved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some clarifications.
- name: Install Rust | ||
uses: dtolnay/rust-toolchain@nightly | ||
with: | ||
toolchain: 1.67.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could use 1.68 here, but I'm not sure and testing it is quite time consuming.
|
||
- name: Install cargo-ndk | ||
continue-on-error: true | ||
run: cargo install cargo-ndk --version 2.11.0 --force # needed to use a previous version of cargo-ndk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we might be able to use versions up to 2.12.2 if I'm not mistaken, but again, testing is time consuming and this version seems to work fine.
@@ -22,7 +22,7 @@ android { | |||
|
|||
minSdk ConfigurationData.minSdk | |||
targetSdk ConfigurationData.targetSdk | |||
versionName ConfigurationData.versionNameCrypto | |||
versionName ConfigurationData.versionNameSdk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there was a copy&paste typo issue here.
# Call the build script in debug mode first to workaround a bug in cargo-ndk | ||
execute_build_script(current_dir, sdk_path, args.module, False) | ||
# Then use the proper release mode one | ||
execute_build_script(current_dir, sdk_path, args.module, True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The release script clones the SDK repo, so I had to add the 'build debug, then release' workaround here and duplicate the script.
When releasing v0.1.7 we found there was an issue with x86_64 binaries which instantly crashed the app on startup. While this seems to be a Rust compiler issue, I also opened an issue in the cargo-ndk repo in case we can find a better workaround, as the team in Mozilla did here.
For now it seems like we'd have to roll back to a previous version of the NDK and cargo-ndk (and because of this, a previous rustc version too if I'm not mistaken). This old cargo-ndk version also has the issue that building only the release version will fail, so we need to build the library twice, first with the dev profile and then the release one.
Also, I'd have to test it, but I think we might not need to use the
macos-latest
runner for these, and that should improve build times a fair bit.