Add check_msrv job to ci workflow by memark · Pull Request #70 · google/zerocopy

memark · 2022-10-16T21:39:40Z

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 · 2022-10-16T22:09:47Z

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

AntoniosBarotsis · 2022-10-17T15:51:16Z

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 · 2022-10-17T18:27:22Z

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 · 2022-10-17T18:45:41Z

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 · 2022-10-18T06:16:03Z

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 · 2022-10-18T18:12:08Z

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 · 2022-10-18T18:20:02Z

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 · 2022-10-18T19:35:57Z

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

memark · 2022-10-19T06:27:30Z

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!

.github/workflows/ci.yml

joshlf

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

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

memark · 2022-10-19T21:48:16Z

Happy to help out!

Validate that the MSRV that we document in our public-facing crate documentation is the same one that we test in CI. Closes #39

joshlf requested changes Oct 18, 2022

View reviewed changes

memark added 2 commits October 19, 2022 07:53

add check_msrv job to ci workflow

35093e0

simplify check_msrv

c6a8029

joshlf requested changes Oct 19, 2022

View reviewed changes

address PR comments

04e634d

joshlf added a commit that referenced this pull request Oct 19, 2022

[DO NOT MERGE] Validate that #70 catches MSRV mismatch

4e27089

joshlf mentioned this pull request Oct 19, 2022

[DO NOT MERGE] Validate that #70 catches MSRV mismatch #79

Closed

joshlf approved these changes Oct 19, 2022

View reviewed changes

joshlf merged commit 9f79f0c into google:main Oct 19, 2022

joshlf pushed a commit that referenced this pull request Aug 3, 2023

[CI] Validate that MSRVs match (#70)

0b6c4e1

Validate that the MSRV that we document in our public-facing crate documentation is the same one that we test in CI. Closes #39

Conversation

memark commented Oct 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshlf commented Oct 16, 2022

Uh oh!

AntoniosBarotsis commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

memark commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshlf commented Oct 17, 2022

Uh oh!

memark commented Oct 18, 2022

Uh oh!

joshlf left a comment

Choose a reason for hiding this comment

Uh oh!

memark commented Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshlf commented Oct 18, 2022

Uh oh!

joshlf commented Oct 18, 2022

Uh oh!

memark commented Oct 19, 2022

Uh oh!

joshlf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joshlf left a comment

Choose a reason for hiding this comment

Uh oh!

memark commented Oct 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

memark commented Oct 16, 2022 •

edited

Loading

AntoniosBarotsis commented Oct 17, 2022 •

edited

Loading

memark commented Oct 17, 2022 •

edited

Loading

memark commented Oct 18, 2022 •

edited

Loading