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

Improve ci and fix clippy #290

Merged
merged 7 commits into from
Sep 20, 2023
Merged

Conversation

waywardmonkeys
Copy link
Contributor

No description provided.

* `actions/checkout` is updated from `v1` to the current `v4`.
* `actions-rs/toolchain` is replaced by `dtolnay/rust-toolchain` as
  the `actions-rs` actions haven't been maintained in a long time.
The parameter takes `IntoIterator`, so we don't have to call
`into_iter` at the call site.
Due to changes in the nightly compiler, using a recent nightly
requires proc-macro2 1.0.60 or later:

dtolnay/proc-macro2#356
@waywardmonkeys
Copy link
Contributor Author

@zkat Fixing the MSRV builds may require updating the MSRV to 1.63 or later. Please advise as to how you'd like to proceed.

@zkat
Copy link
Owner

zkat commented Sep 18, 2023

@waywardmonkeys bumping the MSRV is a semver-major so it should be a separate PR. I'm trying to debate "this is already broken, so an MSRV bump should just be a semver-patch, rather than a semver-major". I really hate having to do semver-major changes for tiny stuff like this.

@waywardmonkeys
Copy link
Contributor Author

@zkat I agree! I can argue either side of it here, so I didn't know what to do. We can at least get most things fixed and I'll be happy to do what's needed in a follow up PR.

rustix is causing the problem and that's used by is_terminal ... so looking at that, is_terminal bumped MSRV in 0.4.8 from 1.48 to 1.63 where it is now (0.4.9).

We could modify the build for the MSRV to use is_terminal 0.4.7 perhaps?

@zkat
Copy link
Owner

zkat commented Sep 18, 2023

ugh. See, it feels incredibly rude to do an MSRV bump in a patch release.

I'm ok with locking our is_terminal version to 0.4.7 and just create an issue for removing the lock when we're ready to do a semver-major for miette.

@Benjamin-L
Copy link
Contributor

You might want to consider committing a lockfile that has version 0.4.7 of is-terminal, without bumping the version in Cargo.toml. My understanding is that a crate is considered to be compatible with a particular MSRV as long as it is possible to select dependency versions that are compatible with the MSRV and the constraints in Cargo.toml. Cargo usually won't find this set automatically. Hopefully some day we'll have a MSRV-aware solver in cargo and this whole problem will go away :)

@waywardmonkeys
Copy link
Contributor Author

@Benjamin-L Interesting ... maybe a possible follow up?

@Benjamin-L
Copy link
Contributor

I think pinning the dependency in Cargo.toml is a breaking change. For example, the following now fails to resolve:

[package]
name = "miette-290-test"
version = "0.1.0"
edition = "2021"

[dependencies]
is-terminal = "0.4.8"

[dependencies.miette]
git = "https://github.com/waywardmonkeys/miette.git"
branch = "improve-ci-and-fix-clippy"
features = [ "fancy" ]

The semver docs don't say anything about this situation, however the cargo dependency docs discourage upper bounds on library crates for this reason.

@waywardmonkeys
Copy link
Contributor Author

It is after midnight here, so tomorrow, I will look at fixing this in the CI scripts, instead of pinning the version like I did here.

is-terminal 0.4.8 updated its MSRV to 1.63, so we can't use it
with our MSRV of 1.56. Force usage of the older version which has
an older MSRV.
@waywardmonkeys
Copy link
Contributor Author

@zkat @Benjamin-L I pushed a different way to resolve it. What do you think?

@zkat
Copy link
Owner

zkat commented Sep 19, 2023

I'm fine with this.

@zkat zkat merged commit cc81382 into zkat:main Sep 20, 2023
11 checks passed
@waywardmonkeys waywardmonkeys deleted the improve-ci-and-fix-clippy branch September 24, 2023 02:30
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.

None yet

3 participants