Skip to content

chore: update rust toolchain, migrate to 2024 edition #3535

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

Conversation

tediou5
Copy link
Contributor

@tediou5 tediou5 commented May 14, 2025

let_chains is already stable.
duration_constructors_lite was forked from duration_constructors and contains only the non-controversial parts(rust-lang/rust#120301).

Most of the changes are simply moves triggered by the updated fmt rules.

The only difference comes from the updated lifetime-capture rules in RPIT(https://doc.rust-lang.org/edition-guide/rust-2024/rpit-lifetime-capture.html):

  1. ) -> Option<impl Future<Output = Option<Piece>> + use<FarmIndex>> {
  2. https://github.com/tediou5/subspace/blob/c65631e55bd8076f0b75ad790f657ab79dff8a89/crates/subspace-farmer/src/single_disk_farm/farming.rs#L131-L143

Code contributor checklist:

Copy link
Contributor

@vedhavyas vedhavyas left a comment

Choose a reason for hiding this comment

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

Would be ideal if you could derive the edition from the workspace Cargo.toml.

Rest looks good

@@ -3,14 +3,13 @@
array_windows,
assert_matches,
btree_extract_if,
duration_constructors,
Copy link
Contributor

Choose a reason for hiding this comment

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

is this just a rename or anything changed that we should be aware of ?

Copy link
Member

Choose a reason for hiding this comment

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

For the record, the lite version includes from_mins and from_hours, we use from_mins only.
(Days, weeks, and larger time units have issues with daylight saving time, civil time, and scientific time rust-lang/rust#140881)

teor2345

This comment was marked as resolved.

@tediou5 tediou5 force-pushed the chore/update-rust-toolchain-and-edition branch from c65631e to 511d7ca Compare May 16, 2025 10:52
@tediou5 tediou5 requested review from teor2345 and vedhavyas May 16, 2025 10:53
@tediou5

This comment was marked as resolved.

@tediou5 tediou5 force-pushed the chore/update-rust-toolchain-and-edition branch 2 times, most recently from f8fb5f0 to b9e8ad0 Compare May 21, 2025 03:22
@tediou5

This comment was marked as resolved.

@vedhavyas

This comment was marked as resolved.

@tediou5

This comment was marked as resolved.

Copy link
Member

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I've had a look at all the manual commits, and they look ok. I'll be reviewing the other ones by running the commands in the commit message, and checking the command outputs.

You could simplify/combine both your cargo fix commands using:
cargo clippy --locked --all-targets --all-features --fix --all

@@ -806,8 +806,7 @@ where
);

return Err(Error::StringError(format!(
"Request limit ({}) exceed the server limit: {} ",
limit, MAX_SEGMENT_HEADERS_PER_REQUEST
"Request limit ({limit}) exceed the server limit: {MAX_SEGMENT_HEADERS_PER_REQUEST} "
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need to be fixed in this PR, but there's an odd extra space at the end of this log message. A search and replace could fix them all at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I found more than one. After merging this pull request, I will submit a separate pull request to fix them.

@tediou5

This comment was marked as resolved.

@tediou5 tediou5 force-pushed the chore/update-rust-toolchain-and-edition branch from b9e8ad0 to ad37d83 Compare June 1, 2025 14:36
@tediou5

This comment was marked as resolved.

@tediou5 tediou5 requested a review from teor2345 June 1, 2025 14:44
teor2345
teor2345 previously approved these changes Jun 2, 2025
Copy link
Member

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks for this change, I'll try and get it in now to avoid further conflicts.

How I Reviewed

For other reviewers/audit, I've left search and replace commands on each commit.

This command gets you the initial commit to base these automatic changes on:

git checkout -b update-rust-edition-auto 76d0a35f5d993ae031f2962575077b83decb8756^

Here is my branch with all those commits, which matches this PR exactly, except for an extra whitespace fix to make some of my tooling stop warning:
https://github.com/autonomys/subspace/tree/update-rust-edition-auto

Future Work

There are some missed feature clippy fixes, which can be done in another PR:

cargo clippy --locked --all-targets --features std,shim,test-ethereum,kzg,binary,serde,parallel,cluster,numa,default-library,testing,with-tracing --fix --all
cargo clippy --locked --all-targets --features no_std --fix --all

It might also be a good idea to update the rustfmt.toml edition to 2024.

Cargo.toml Outdated
Comment on lines 1 to 3
[workspace.package]
edition = "2021"

Copy link
Member

Choose a reason for hiding this comment

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

This commit contains this manual change, and a search and replace:

fastmod --accept-all --fixed-strings 'edition = "2021"' 'edition.workspace = true'  --extensions rs

Comment on lines -4 to +5
std::env::set_var("WASM_BUILD_TYPE", "release");
// SAFETY: no concurrent writing or reading here, build scripts are single threaded.
unsafe { std::env::set_var("WASM_BUILD_TYPE", "release") };
Copy link
Member

Choose a reason for hiding this comment

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

Search and replace:

fastmod --accept-all --fixed-strings 'std::env::set_var("WASM_BUILD_TYPE", "release");' '// SAFETY: no concurrent writing or reading here, build scripts are single threaded.
        unsafe { std::env::set_var("WASM_BUILD_TYPE", "release") };'

#![feature(let_chains, try_blocks, duration_constructors)]
#![feature(let_chains, try_blocks, duration_constructors_lite)]
Copy link
Member

Choose a reason for hiding this comment

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

Search and replace:

fastmod --accept-all --fixed-strings 'duration_constructors' 'duration_constructors_lite'

@@ -3,14 +3,13 @@
array_windows,
assert_matches,
btree_extract_if,
duration_constructors,
Copy link
Member

Choose a reason for hiding this comment

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

For the record, the lite version includes from_mins and from_hours, we use from_mins only.
(Days, weeks, and larger time units have issues with daylight saving time, civil time, and scientific time rust-lang/rust#140881)

@@ -61,7 +61,7 @@ where
pub fn read_piece(
&self,
piece_index: PieceIndex,
) -> Option<impl Future<Output = Option<Piece>> + 'static> {
) -> Option<impl Future<Output = Option<Piece>> + use<FarmIndex>> {
Copy link
Member

Choose a reason for hiding this comment

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

Automatically performed (but the 'static needs to be manually removed):

cargo fix --broken-code

extern "C" {
unsafe extern "C" {
Copy link
Member

Choose a reason for hiding this comment

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

Possibly fixed/suggested by (I don't have Linux locally):

cargo fix --broken-code --features cuda

extern "C" {
unsafe extern "C" {
Copy link
Member

Choose a reason for hiding this comment

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

Possibly fixed/suggested by (I don't have Linux locally):

cargo fix --broken-code --features cuda

Comment on lines 2213 to 2217
if pre_dispatch {
if let Some(future_slot) = T::BlockSlot::future_slot(current_block_number) {
if pre_dispatch
&& let Some(future_slot) = T::BlockSlot::future_slot(current_block_number) {
ensure!(slot_number <= *future_slot, BundleError::SlotInTheFuture)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Command can be simplified to:

cargo clippy --locked --all-targets --features runtime-benchmarks,cuda --fix --all

I couldn't automatically compile/fix cuda or x86_64/aes because I don't have the OS/hardware, so I reviewed them manually instead.

self.handle_remove_listeners(&[address.clone()]);
self.handle_remove_listeners(std::slice::from_ref(address));
Copy link
Member

Choose a reason for hiding this comment

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

Recommended by cargo clippy, clone() does not have side effects here.

Nitpick for future changes: usually we'd use std::slice; at the top of the file and slice::from_ref here.

#![feature(array_windows, let_chains, variant_count)]
#![feature(array_windows, variant_count)]
Copy link
Member

Choose a reason for hiding this comment

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

This can be automated using:

fastmod --accept-all --fixed-strings '#![feature(let_chains)]' ''
fastmod --accept-all --fixed-strings 'let_chains,' ''
cargo fmt --all

Copy link
Member

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I fixed a clippy warning hidden behind a feature that wasn't automatically fixed in this PR.

@teor2345 teor2345 enabled auto-merge June 2, 2025 04:03
@teor2345 teor2345 added this pull request to the merge queue Jun 2, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 2, 2025
@teor2345 teor2345 added this pull request to the merge queue Jun 2, 2025
@teor2345
Copy link
Member

teor2345 commented Jun 2, 2025

These tests took more than 90 minutes on macOS, they usually take 10-30 minutes, probably some race condition:

   SLOW [>4980.000s] domain-client-operator tests::test_xdm_channel_allowlist_removed_after_xdm_resp_relaying
   SLOW [>4980.000s] domain-client-operator tests::test_xdm_false_invalid_fraud_proof

I restarted the whole set of merge checks (it's not possible to restart just one check in the merge queue).

Merged via the queue into autonomys:main with commit 20ce4d9 Jun 2, 2025
10 checks passed
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