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

Pin rust version with toolchain file #7547

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

kl
Copy link
Contributor

@kl kl commented Jan 29, 2025

As part of enabling reproducible builds we need to build a given commit with a pinned Rust version.

This commit adds a minimal rust-toolchain.toml file that specifies this version (this Rust version is enforced by rustup when invoking a cargo command).

In the container we extract the version that is specified in rust-toolchain.toml to ensure that the container is pre-built with the correct Rust version (so that when buildling in the container we do not need to download the right Rust version).

This PR sets the Rust version to 1.83.0.


This change is Reviewable

Copy link

linear bot commented Jan 29, 2025

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 5 files at r1.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @kl)


building/build-and-publish-container-image.sh line 38 at r1 (raw file):

full_container_name="$REGISTRY_HOST/$REGISTRY_ORG/$container_name"

RUST_VERSION=$(rg -o "channel = \"(.*)\"" -r '$1' "$REPO_DIR/rust-toolchain.toml")

We need to do the same in android/fdroid-build/init.sh

Code quote:

RUST_VERSION=$(rg -o "channel = \"(.*)\"" -r '$1' "$REPO_DIR/rust-toolchain.toml")

@kl kl requested review from faern and raksooo as code owners January 30, 2025 14:56
@kl kl force-pushed the pin-rust-version-and-allow-automatic-install-of-right-des-554 branch from ac3fdeb to 188506a Compare January 30, 2025 15:24
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 8 files reviewed, all discussions resolved


building/build-and-publish-container-image.sh line 38 at r1 (raw file):

Previously, albin-mullvad wrote…

We need to do the same in android/fdroid-build/init.sh

NVM, that should be handled by the combination of the rust-toolchain.toml file and the init script adding targets during the build process.

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 5 files at r1, 1 of 4 files at r2.
Reviewable status: 4 of 8 files reviewed, all discussions resolved

Copy link
Member

@faern faern left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 8 files reviewed, 1 unresolved discussion (waiting on @kl)


a discussion (no related file):
We probably need to update a few more CI workflows. Search for rustup default in the .github/ directory. All those CI jobs set up a default toolchain and expect all subsequent Rust invocation to use that toolchain. But rust-toolchain.toml will override it. This has to be solved in a different way.

We do want to continue running CI against beta and nightly. So those CI jobs need have their overrides fixed to work. Maybe they can just locally delete rust-toolchain.toml before building anything?

@kl kl force-pushed the pin-rust-version-and-allow-automatic-install-of-right-des-554 branch from 188506a to a1db840 Compare January 30, 2025 15:39
@kl kl force-pushed the pin-rust-version-and-allow-automatic-install-of-right-des-554 branch from 8dcc83f to 31d3cff Compare January 30, 2025 16:32
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