Fix the CI

[YuhanLiin]

Right now the CI is broken for two reasons I know of. Firstly, the newest version of thiserror(1.0.26) breaks Clippy on older versions. This can be sidestepped by locking thiserror to an older version, but I don't see the value of running Clippy on older versions of Rust in the first place. We should only run code quality for stable Rust and maybe beta. We are also pinning our version of nightly in CI. This was done because using the latest nightly caused occasional breakages in the past. Unfortunately our current nightly version is too old for some of our dependencies. We may be able to fix this by reverting back to using latest nightly, but I don't see the point in running nightly in CI since Linfa is used in stable Rust.

[bytesnake]

Should we then run the complete pipeline one MVC, stable and beta and only run cargo check for the latest nightly version?

[YuhanLiin]

IMO code quality and coverage checks should be run for stable only since those checks don't actually matter for the purposes of testing whether our code works on alternative toolchains. We don't care about whether our code passes Clippy on MSRV, we only care that it works. I also don't know why we test/check our code on nightly and beta, since we don't provide guarantees that it works on those platforms. I think we should run cargo test and cargo check on stable and MSRV and run the rest of the pipeline in just stable.

[xd009642]

A standard for most library authors is to run on all three and just add something to nightly so it's allowed to fail and doesn't stop CI. It's always best to have some inkling that linfa's stopped working on nightly as other people working on ML and related areas might be using nightly for fancier const generics or other optimisations.

[YuhanLiin]

You have a point about running nightly/beta checks and tests and allowing those runs to fail. I still don't see the point of running non-stable toolchains for code quality and coverage jobs, since those don't have any impact on whether Linfa "works" on a particular toolchain.

[xd009642]

Oh yeah agree with that, although with tarpaulin in future nightly will provide more accurate coverage reports because of using some unstable flags :eyes:. So once that's done there'll be a benefit

[bytesnake]

IMO code quality and coverage checks should be run for stable only since those checks don't actually matter for the purposes of testing whether our code works on alternative toolchains. We don't care about whether our code passes Clippy on MSRV, we only care that it works. I also don't know why we test/check our code on nightly and beta, since we don't provide guarantees that it works on those platforms. I think we should run cargo test and cargo check on stable and MSRV and run the rest of the pipeline in just stable.

mh I would like to be at least a bit conservative here and run the code quality checks for channels MSRV and stable in order to find possible regression, I do care whether the linting procedures work correctly. We also should keep a closer eye on problems for the nightly channel but I'm getting from your tone that frequent breaks happen there. I think that @xd009642 idea is best to still run a simple compilation test but not block allowance for PR on that. But yeah I agree wholeheartedly that the CI is atm overparametrized ..

[YuhanLiin]

If we run code quality for older versions then we need to lock thiserror to 1.0.25 since the newest version breaks older versions of Clippy.

[bytesnake]

merged #145 and updated the protection rules, let's keep this open until we are sure that the CI is not throwing false positives anymore. The pipeline actually failed after merging to master because an endpoint for the rustc stable release didn't respond but we can't do much about this

[YuhanLiin]

Note that if the nightly step fails in a PR it will look like a normal pipeline failure, but unlike a normal failure you will be allowed to merge the PR.

[YuhanLiin]

I believe it's safe to close this issue now, since CI's been well-behaved recently.