Skip to content

Conversation

thunderbiscuit
Copy link
Member

@thunderbiscuit thunderbiscuit commented Sep 24, 2025

This addresses #314. It also incorporates the changes in #299.

The rust-toolchain.toml file is used to help contributors build, format, and clippy check the codebase using the same version of the Rust compiler. It can easily be overriden, but if no other arguments are supplied to commands like cargo run, cargo fmt, and cargo clippy, they will pick the version defined in this file.

In order to ensure the CI runs and the default versions devs build the library with (the one defined in rust-toolchain.toml) stay in sync, a few changes were made to the cont_integration.yaml workflow:

  1. I removed the "prepare" job, which parsed the rust-version file in order to use it throughout the CI jobs.
  2. I split the Build & Test job (and the 3-way matrix of (a) OS, (b) compilation features, (c) Rust version) into 2 different jobs. This is IMO easier for the eye to parse when looking at the workflow, and fully separate the testing of the MSRV and Stable into two separate jobs instead of a matrix of jobs with some extra of if/else stuff (for running the pinning script for example). The 2 jobs are now called Build & Test MSRV and Build and Test Stable. Note that this makes it easier to see and understand the workflow in the GitHub UI.
  3. I picked a new "rust toolchain" action, setup-rust-toolchain. It's the one recommended to me by both ChatGPT and Claude, and has a lot of stars and users (albeit a bit less than the dtolnay action, which its readme cites as its main inspiration). The reason for this is 2-fold: (a) it comes with caching out of the box, and (b) it respects the toolchain.toml file by default. See their docs on this exact feature. This makes our workflows easy to work with: in the first job, we explicitly set the toolchain version to our MSRV, in the second we let the action download the toolchain defined in rust-toolchain.toml.

I also bumped the checkout action to its latest v5 version (last commit).

close #314
close #299

Checklists

All Submissions:

Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

cACK 60d2601

I wonder if we should add the targets meant for cross-compilation too.

@thunderbiscuit
Copy link
Member Author

I wonder if we should add the targets meant for cross-compilation too.

If I understand correctly, this would install all those targets for all devs cloning and building the repo, which doesn't seem optimal to me. Mind you they won't make cargo attempt to build for all those targets, only that they are downloaded/available.

But as is, the file is more of a convenience for contributors and users, only ensuring the library will build, format, and lint consistently if no other arguments superceding the compiler version are set. This was an issue for me as my default toolchain is 1.85 (what we use to build the bindings), and compilation always fails because of an error about our use of an unstable library feature, the unsigned_is_multiple_of. Adding this file makes it so my default toolchain system-wide can stay 1.85, but whenever I come into the bdk_wallet repo it builds nice and clean, using 1.89 without requiring anything else from me.

@thunderbiscuit thunderbiscuit force-pushed the feature/rust-toolchain branch 4 times, most recently from 0cfd2a5 to 55a805b Compare September 29, 2025 20:08
@thunderbiscuit thunderbiscuit force-pushed the feature/rust-toolchain branch 2 times, most recently from 45aa94f to 58cd1d1 Compare September 30, 2025 17:00
@thunderbiscuit
Copy link
Member Author

thunderbiscuit commented Sep 30, 2025

The code coverage run fails but that's not related to this PR.

I updated the description above, but re-posting here for visibility:

In order to ensure the CI runs and the default versions devs build the library with (the one defined in rust-toolchain.toml) stay in sync, a few changes were made to the cont_integration.yaml workflow:

  1. I removed the "prepare" job, which parsed the rust-version file in order to use it throughout the CI jobs.
  2. I split the Build & Test job (and the 3-way matrix of (a) OS, (b) compilation features, (c) Rust version) into 2 different jobs. This is IMO easier for the eye to parse when looking at the workflow, and fully separates the testing of the MSRV and Stable into two separate jobs instead of a matrix of jobs with some extra of if/else stuff (for running the pinning script for example). The 2 jobs are now called Build & Test MSRV and Build and Test Stable. Note that this makes it easier to see and understand the workflow in the GitHub UI.
  3. I picked a new "rust toolchain" action, setup-rust-toolchain. It's the one recommended to me by both ChatGPT and Claude, and has a lot of stars and users (albeit a bit less than the dtolnay action, which its readme cites as its main inspiration). The reason for this is 2-fold: (a) it comes with caching out of the box, and (b) it respects the toolchain.toml file by default. See their docs on this exact feature. This makes our workflows easy to work with: in the first job, we explicitly set the toolchain version to our MSRV, in the second we let the action download the toolchain defined in rust-toolchain.toml.

I also bumped the checkout action to its latest v5 version (last commit).

@thunderbiscuit thunderbiscuit force-pushed the feature/rust-toolchain branch from 7dedc67 to d93b87b Compare October 1, 2025 00:25
@oleonardolima oleonardolima self-requested a review October 1, 2025 04:17
@thunderbiscuit thunderbiscuit force-pushed the feature/rust-toolchain branch from 53351b1 to c6af44b Compare October 3, 2025 15:33
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

utACK 6f140ed

@ValuedMammal ValuedMammal moved this from In Progress to Needs Review in BDK Wallet Oct 7, 2025
@ValuedMammal ValuedMammal added this to the Wallet 3.0.0 milestone Oct 7, 2025
@ValuedMammal
Copy link
Collaborator

ACK 6f140ed

@ValuedMammal ValuedMammal merged commit 321c8a0 into bitcoindevkit:master Oct 7, 2025
15 checks passed
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Wallet Oct 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Consider adding a rust-toolchain.toml file

3 participants