Add check_msrv job to ci workflow

[memark]

Add check_msrv job to ci workflow. Checking the MSRV specified in all places mentioned in #39:

• Doc comment in src/lib.rs
• .github/workflows/ci.yml
• tests/trybuild.rs (added in Make Unalign::into_inner const #60, which hasn't merged as of this writing)
• zerocopy-derive/tests/trybuild.rs

When running locally this check works against #60 as well.

Resolves #39.

[joshlf]

Thanks, @memark! I'll give this a review once the CI tests are passing.

[AntoniosBarotsis]

Hello!

I tried messing around with this for a bit cause I saw you were getting an error with no error message.

I see you used '.jobs.build_test.strategy.matrix.channel[0]' somewhere, I couldn't figure out how to get matrix variables from a different job but one (albeit slightly messy) workaround would be to define a top-level env variable with the first channel and then use that in both the matrix and in the MSRV step.

I tried doing that here but I am still getting some errors (this time with error messages!) and here are the logs. Not sure if this is of any help but I hope so :)

Note
I didn't use that said variable anywhere in the matrix yet, I just declared it to see that it works

[memark]

Thanks, that's an interesting approach! @AntoniosBarotsis

Something I struggled/hesitated a bit about is where the "true" MSRV really lives? (If there is one?)
Right now I'm pretty much only comparing all versions I can extract. Could this variable be it? (Is the workflow the place where we'd want to store it?)

I did find out why there where no error messages btw, it was that pesky "set -e" that I blindly copied from the other jobs... After one afternoon of trials in my own repo I finally figured that one out.

[joshlf]

Something I struggled/hesitated a bit about is where the "true" MSRV really lives? (If there is one?)
Right now I'm pretty much only comparing all versions I can extract. Could this variable be it? (Is the workflow the place where we'd want to store it?)

I originally tried to have a source of truth in an MSRV.txt file (see #39 (comment)). I even tried to have a source of truth just in the scope of the ci.yml file, and discovered that that would also be really difficult to do ergonomically. That's why I figured that "the same MSRV in all of the files" might be the best we could do without a lot of over-engineering.

[memark]

That's why I figured that "the same MSRV in all of the files" might be the best we could do without a lot of over-engineering.

Understood. I've gone with this approach as well now. I think the workflow should pass now, please allow it to run.

[joshlf]

Hey, sorry about the churn on this one, but I just had a realization:

• Looking for all MSRVs is pretty brittle because it relies on the exact format of a number of different files; e.g., I could use a different expression in the trybuild.rs files, and all of a sudden the greps we're running on those files would just output nothing, which would cause this script to continue to succeed
• It would probably be fine to not look at the trybuild.rs files - if we get the MSRV there wrong, all that will happen is that at worst the tests will fail, which we'll notice and fix
• We'd like to do set -e to catch any errors in the script, but we can't so long as tests/trybuild.rs doesn't exist yet
• The thing we really need to check for - the thing that would be bad if we missed - is if we're testing for a different MSRV in ci.yml than the one we have advertised in the doc comment in src/lib.rs

With all that in mind, how would you feel about making the following changes?

• Doing set -e at the top
• Only checking that the MSRV in src/lib.rs matches the one in ci.yml

[memark]

Sure, I'll make those changes.

I will add though that it would have saved me a lot of time and energy if I knew that all that was required was comparing two version numbers (out of the six present in the locations listed in the issue).

As for set -e I think it makes more sense in a script that does side-effectful actions, and less in a query-based script like this. It also has the effect that if something fails (e.g. grep) all you get is "-1" without much explanation.

Since the src/lib.rs comment is not yet available in main, and we want set -e, should I then write a version of the script that doesn't work until #60 is merged? Or just wait?

[joshlf]

Sure, I'll make those changes.

Thanks!

I will add though that it would have saved me a lot of time and energy if I knew that all that was required was comparing two version numbers (out of the six present in the locations listed in the issue).

Yeah, sorry about that 🙁 Didn't realize that this was the right approach until you'd already put up the code.

As for set -e I think it makes more sense in a script that does side-effectful actions, and less in a query-based script like this. It also has the effect that if something fails (e.g. grep) all you get is "-1" without much explanation.

In this particular case, we expect grep to never return a non-zero code, right? It should always find at least one matching line, so if it doesn't, we should see that error and the test should fail.

Since the src/lib.rs comment is not yet available in main, and we want set -e, should I then write a version of the script that doesn't work until #60 is merged? Or just wait?

It's up to you. I think you could also just include an MSRV section in src/lib.rs in this PR - if it merges before #60, then cool. If it doesn't, then we can rebase. But you could also just wait until #60 merges. Up to you!

[joshlf]

OK, #60 has been superseded by #76, which has been merged. This should be unblocked now.

[memark]

Just pushed a simplified version of the msrv check. Ready for review.

[joshlf]

Awesome, thanks for this, @memark! Just a few nit-pick comments, but this looks great!

[joshlf]

Confirmed in #79 that this test catches an MSRV mismatch:

Thanks again for all your work on this, @memark!

[memark]

Happy to help out!