Run tests for MSRV, on macOS versions, and fix toolchain

[micolous]

#611 added the MSRV to CI, but doesn't run any tests, or run it against the same set of macOS versions as regular tests.

This puts the MSRV in as a build matrix entry for the normal builds, so it'll run the same way for all.

This also switches to dtolnay/rust-toolchain, because actions-rs/toolchain is unmaintained and doesn't actually work properly anymore (it'll still build with Rust stable!).

CI run: https://github.com/micolous/core-foundation-rs/actions/runs/5514991574

[jrmuizel]

I don't think it's necessary to run on all the combinations of macOS and toolchains. Running MSRV on just one version of macOS should be fine.

[micolous]

I've done that under the assumption that there are two configuration parameters to test: the Rust version (stable and MSRV) and macOS version (11, 12, 13). It seems logical to test the Cartesian product of those two parameters, given that there are a fairly small number of configurations, the tests execute reasonably quickly, and they can be executed in parallel.

Digging into the PR history, there have been some macOS-version-specific test failures (eg: #600) and many changes to fix issues that came up on new Rust versions.

[micolous]

I don't know what can push this forward, and I suspect the bots need a poke.

However, lets wait for #615, because that PR touches nearly every file in this repository, whereas this only touches one. :)

[micolous]

Should be good to merge now.

[djc]

I recommend only running check --lib --all-features with the MSRV. There's no need to run tests (or involve dev-dependencies) in the MSRV job.

[waywardmonkeys]

Should probably change actions/checkout@v2 to actions/checkout@v3 at the same time!

[micolous]

I recommend only running check --lib --all-features with the MSRV. There's no need to run tests (or involve dev-dependencies) in the MSRV job.

@djc The whole point of having an MSRV is you're warranting the software works with a particular Rust compiler. Omitting unit tests/targets on the MSRV would (in effect) make it not as well supported as Rust stable.

While it means there's more work to do in CI (by not skipping steps, and because the MSRV compiler seems to be slower than stable), the targets are run in parallel, so the performance hit is less severe (as long as the jobs aren't queued in a backlog).

I'm not advocating for every Rust compiler between MSRV and stable to be tested – while that would be even more thorough, it comes with a lot more overhead (especially with macOS runners, which tend to be slow), and risks queuing. Testing two Rust versions is a reasonable enough approximation, and is the strategy that most Rust libraries go for.

There are performance optimisations which could be done to this which don't reduce test coverage (eg: using sccache), but I'll leave this as an exercise for the maintainers. 😄

Should probably change actions/checkout@v2 to actions/checkout@v3 at the same time!

Done, it looks like that's also causing warnings in the CI log.

To reiterate a point I made in the description: right now, the msrv job doesn't actually work as described or intended: it still builds against Rust stable. This issue has come up in another Rust project I'm working on, and I'm trying to prevent that from surprising you as well 😄

[djc]

@djc The whole point of having an MSRV is you're warranting the software works with a particular Rust compiler. Omitting unit tests/targets on the MSRV would (in effect) make it not as well supported as Rust stable.

My point is that it's extremely unlikely to have the compiler version affect functionality as long as the library can compile against the MSRV. That is, the MSRV is about language/syntax support and about the contents of core/alloc/std. It is very unlikely that changes in the language and those libraries will break your tests.

My issue is not CI resources but the MSRV constraints of dev-dependencies brought in by running tests/compiling example targets etc. If you use dev-dependencies in your MSRV job in CI you end up bumping your library's MSRV when your dev-dependencies require it, not when your actual library requires it, which is a bad trade-off.

However, this might be moot because right now the crates in this repo don't appear to pull in any dev-dependencies at all.

[micolous]

I understand now, and that's a fair point as well.

I feel like that's something that can be considered when it comes along, where options including abandoning testing on the MSRV, bumping the MSRV, and changing the test/example to meet the existing MSRV would all be on the table.

You could also have a test which depends on newer versions of the compiler - and you'd never see the issue when it's never built with cfg(test).

[waywardmonkeys]

Could we get just the non-controversial parts of this in place as they would be an immediate improvement?

[micolous]

To make sure my understanding is correct, I believe the only outstanding concern here was that testing on MSRV might fail if dev-dependencies change their MSRV.

Given there are no dev-dependencies declared here, and testing on MSRV is one of this PR's objectives, I think this issue should be addressed when and if it arrives.

My interpretation of @djc's last comment was that it was waiting for me to acknowledge the concern, and take action if I felt it was warranted. So:

• if my interpretation is accurate (ie: @djc is happy with that), then I think we can say that all issues are settled and move forward with merging this as-is.

• if not, then I'll drop this PR in favour of a simpler PR which switches all jobs to actions/checkout@v3, and the msrv job to dtolnay/rust-toolchain.

I have a preference for this PR's approach, but at the end of the day, I acknowledge this isn't my call to make, and this issue doesn't block anything that I want to do, so I have no desire to fuel controversies. 😄

[djc]

Agreed we can move forward with merging this (not that I technically have any authority in this repo).