Skip to content

ci: introduce rust toolchain #9791

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

Merged
merged 1 commit into from
Mar 24, 2025
Merged

Conversation

tobtoht
Copy link
Collaborator

@tobtoht tobtoht commented Feb 9, 2025

No description provided.

@tobtoht tobtoht force-pushed the ci_rust branch 2 times, most recently from 276e9cc to f55e777 Compare February 16, 2025 07:36
@tobtoht tobtoht marked this pull request as ready for review February 16, 2025 07:37
# We could update ld (a pain), update Ubuntu (requires a large amount of changes), or downgrade Rust
# We can't use Rust 1.70 due to LLVM 16 requiring ld >= 2.40 when building for Windows
run: |
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y

Choose a reason for hiding this comment

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

Installing a script sourced from a serbian TLD without any validation in the depends environment makes me nervous. You may want to force a manual installation with a known checksum, as described here: https://rust-lang.github.io/rustup/installation/other.html#manual-installation.

Choose a reason for hiding this comment

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

To be clear, I think country-level TLDs increase the risk (irrespective of country), but the methodological concern applies to any domain.

Copy link
Collaborator Author

@tobtoht tobtoht Feb 17, 2025

Choose a reason for hiding this comment

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

Rustup installs unsigned binaries that rolled off some CI somewhere. For development builds, I think that's fine. For release builds, that's not okay. The CI jobs here are not release builds and anyone who wishes to run the artifacts should take proper precaution by running them in an isolated environment.

Choose a reason for hiding this comment

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

The CI jobs here are not release builds and anyone who wishes to run the artifacts should take proper precaution by running them in an isolated environment.

I do not object to this policy, and I like the wording; perhaps you can post this exact sentence as a comment at the top of the file, so readers understand the workflow's aim.

@tobtoht tobtoht added this to the FCMP++ HF milestone Feb 27, 2025
Copy link
Contributor

@hinto-janai hinto-janai left a comment

Choose a reason for hiding this comment

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

FYI GitHub CI already has rustup and cargo, only a toolchain version switch may be necessary.

@tobtoht
Copy link
Collaborator Author

tobtoht commented Mar 23, 2025

@hinto-janai The CI runs were containerized in #9708 and #9784. The Ubuntu/Debian base images do not come with Rust/Cargo installed, hence the installation.

@tobtoht tobtoht merged commit 642db87 into monero-project:master Mar 24, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants