zerocopy

zerocopy copied to clipboard

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 https://github.com/google/zerocopy/pull/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.

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

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

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.

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 https://github.com/google/zerocopy/issues/39#issue-1402438366). 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.

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.

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?

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!

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

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

Happy to help out!