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

Explicitly set RUSTUP_TOOLCHAIN to 1.64.0 #2440

Merged
merged 1 commit into from
Dec 30, 2022

Conversation

link2xt
Copy link
Contributor

@link2xt link2xt commented Dec 25, 2022

Core is supposed to support all Rust versions above MSRV, there is no need to use the same toolchain for the core across all platforms.

@link2xt
Copy link
Contributor Author

link2xt commented Dec 25, 2022

This will avoid problems like deltachat/deltachat-core-rust#3856 in the future. Android build can stay with 1.64.0 as long as this toolchain works, there is no need to use the same version for core CI, iOS, Android and Desktop builds. We can then keep updating Rust toolchain that we use for development and even remove the rust-toolchain file from the core repository so developers can update at their own pace.

@github-actions
Copy link

To test the changes in this pull request, install this apk:
📦 app-preview.apk

@link2xt link2xt force-pushed the link2xt/deprecate-rust-toolchain branch from c9e1d67 to 83b840c Compare December 25, 2022 23:22
@github-actions
Copy link

To test the changes in this pull request, install this apk:
📦 app-preview.apk

Copy link
Member

@r10s r10s left a comment

Choose a reason for hiding this comment

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

probably makes sense. in all these stacks, ndk, jni and whatnot a slightly older rust toolchain is probably not adding much of issues.

@@ -108,12 +108,14 @@ if test -z "$(find "$ANDROID_NDK_ROOT" -name libgcc.a -print -quit)"; then
echo 'INPUT(-lunwind)' >"$TMPLIB/libgcc.a"
fi

RUSTUP_TOOLCHAIN=1.64.0
Copy link
Member

Choose a reason for hiding this comment

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

maybe add a comment that when modifying this line, one should also modify install-toolchains.sh. and maybe also say why we are not using cat rust-toolchain - at least i tend to forget about that completely as it is touched only very rarely - and at least some other devs are not that deep in rust :)

if test -z $1 || test $1 = armeabi-v7a; then
echo "-- cross compiling to armv7-linux-androideabi (arm) --"
export CFLAGS=-D__ANDROID_API__=16
TARGET_CC=armv7a-linux-androideabi16-clang \
TARGET_AR=llvm-ar \
cargo +`cat rust-toolchain` rustc $RELEASEFLAG --target armv7-linux-androideabi -p deltachat_ffi -- -L "$TMPLIB"
cargo "+$RUSTUP_TOOLCHAIN" rustc $RELEASEFLAG --target armv7-linux-androideabi -p deltachat_ffi -- -L "$TMPLIB"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm +/-0 on this PR, because the downside is that this will mean that we need to bump the Rust version in even more places every few months.

But maybe we can create one file rust-toolchain in the dc-android repo that contains the name of the currently used Rust version? This way, we'd only need to update it in one place.

E.g.:

Suggested change
cargo "+$RUSTUP_TOOLCHAIN" rustc $RELEASEFLAG --target armv7-linux-androideabi -p deltachat_ffi -- -L "$TMPLIB"
cargo +`cat ../../rust-toolchain` rustc $RELEASEFLAG --target armv7-linux-androideabi -p deltachat_ffi -- -L "$TMPLIB"

@link2xt link2xt force-pushed the link2xt/deprecate-rust-toolchain branch 2 times, most recently from c85ddb6 to b126538 Compare December 30, 2022 00:43
@link2xt
Copy link
Contributor Author

link2xt commented Dec 30, 2022

@r10s @Hocuri I addressed the comments, now there is a single scripts/rust-toolchain file.

Core is supposed to support all Rust versions above
MSRV, there is no need to use the same toolchain for
the core across all platforms.
@link2xt link2xt force-pushed the link2xt/deprecate-rust-toolchain branch from b126538 to 78aada7 Compare December 30, 2022 16:38
@github-actions
Copy link

To test the changes in this pull request, install this apk:
📦 app-preview.apk

@link2xt link2xt merged commit 78aada7 into master Dec 30, 2022
@link2xt link2xt deleted the link2xt/deprecate-rust-toolchain branch December 30, 2022 17:13
@link2xt link2xt mentioned this pull request Mar 11, 2023
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.

4 participants