Skip to content

Conversation

@nyonson
Copy link
Contributor

@nyonson nyonson commented Oct 28, 2025

The fact that the stable, nightly, and msrv tasks were essentially the same was making me twitch a bit, so added toolchain validation to ensure they are being called with the intended toolchain.

Added an optional CRATE array parameter to run a task on a subset of crates. This allows for things like MSRV checks on crates with different MSRVs in a workspace. It also allows for quick tests on a crate you are hacking on. I believe this is enough to enable some quick and quiet local test cycles.


case "$required_toolchain" in
nightly)
if ! echo "$current_toolchain" | grep -q nightly; then
Copy link
Member

Choose a reason for hiding this comment

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

In case you like it, claude told me this can be done like this

if [[ "$STRING" == *"world"* ]]; then
    echo "contains world"
fi

Note: Use *pattern* for literal substring matching.

Or

STRING="hello world"
[[ "$STRING" =~ world ]] && echo "contains world"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, no strong feelings either way. I think I'd roll with one of these if the rest of the script was all bash, but it's kinda all jumbled up right now.

Copy link
Member

Choose a reason for hiding this comment

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

No sweat.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 4a49767

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 7e4eb32

@apoelstra
Copy link
Member

utACK b2329b8

@apoelstra
Copy link
Member

Oh, that's the tip of #22 not #23. Lemme ACK and merge there.

@apoelstra
Copy link
Member

In 5688089:

Trailing whitespace. Also, I don't totally buy the "all crates in this repo will have the same MSRV" but I guess we can keep the check until it fails.

@apoelstra
Copy link
Member

At least, we could read the MSRV out of clippy.toml instead of a random Cargo.toml, which I think is what our existing code does. clippy.toml is a top-level file.

@apoelstra
Copy link
Member

Other than that 7e4eb32 looks good to me.

@nyonson
Copy link
Contributor Author

nyonson commented Oct 29, 2025

def44e0: cleaned up trailing whitespace and tweaked the MSRV toolchain check.

I'd prefer to use rust-version from the Cargo.toml since that is the new-ish spot for it to live going forward. Clippy respects it, so the setting is no longer required in clippy.toml. Using cargo metadata is a bit more robust as well since it handles workspace inheritance settings.

I find it kinda annoying that a workspace can have crates with different MSRVs. Bit of a mismatch with our tasks here which are currently designed to run for the whole workspace, not a subset of crates. The script does not change the toolchain on the fly and I think it is probably best to keep it this way.

One easy option is to get the highest MSRV of all the workspace crates and call that the "effective" MSRV of the workspace. But I went with a more robust approach and just check per-crate, if a caller has multiple MSRVs they deal with running different tasks with the necessary toolchain.

@tcharding
Copy link
Member

In 5688089:

Trailing whitespace. Also, I don't totally buy the "all crates in this repo will have the same MSRV" but I guess we can keep the check until it fails.

Yeah I balked at this too, for example I can see the MSRV on the stable crates staying where it is indefinitely while rust-bitcoin moves on.

@nyonson
Copy link
Contributor Author

nyonson commented Oct 30, 2025

dfe6fb7: allow passing multiple crate args

@nyonson
Copy link
Contributor Author

nyonson commented Oct 30, 2025

Yeah I balked at this too, for example I can see the MSRV on the stable crates staying where it is indefinitely while rust-bitcoin moves on.

This seems a little weird to me because I assume crates in a workspace are tightly coupled, so bumping the MSRV of one has a high chance of implicitly raising it for the others, but I see the point with primitives/units. Also cargo doesn't enforce any of this so I probably shouldn't assume it. I think the current version of the script supports the case well enough now, the task can be called with a list of crates so a subset on a different MSRV could be tested together.

@nyonson nyonson changed the title Add toolchain check and crate parameter Add toolchain check and crates parameter Oct 30, 2025
@tcharding
Copy link
Member

I'm fine as it is, we can change it later if we need to.

@nyonson nyonson mentioned this pull request Nov 5, 2025
@nyonson
Copy link
Contributor Author

nyonson commented Nov 10, 2025

Closing in favor of xshell #26

@nyonson nyonson closed this Nov 10, 2025
@apoelstra
Copy link
Member

This seems a little weird to me because I assume crates in a workspace are tightly coupled,

Well, the "coupling" can only go in one direction (Rust doesn't allow circular dependencies.) So we expect the more "fundamental" crates to have a lower MSRV than the ones that depend on them.

@nyonson
Copy link
Contributor Author

nyonson commented Nov 10, 2025

Yea, good point.

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