Pin toolchain in actions

[smklein]

Fixes #138

[davepacheco]

I'm loath to have CI do something different than what someone would do if they just cloned and ran the tests locally. Is it possible to put this into rust-toolchain instead? I'm not sure what effect that has consumers of library crates.

I think ideal behavior would be:

• For people working on Dropshot (so, builds from a local clone as well as CI), we have a rust-toolchain file (and Cargo.lock, for that matter). This way, builds are reproducible and things never break because dependencies change. When these get rev'd, it's always in a controlled way (i.e., a PR that runs checks).
• We have some automation to update rust-toolchain and Cargo.lock so that we still stay up-to-date.
• Consumers can pick whatever toolchain and dependency versions they want (by virtue of their own rust-toolchain and Cargo.toml/Cargo.lock files) but they'll be able to clearly see what we've tested.

I'm not sure this works that way for library crates though.

[smklein]

I updated the PR to do the following:

• Include a Cargo.lock file, so we can at least control things explicitly
• Specify a rust-toolchain.toml file which should match the github action
• Update the test outputs (since Cargo.lock was rev'd)

Aside from the automation bit, I think this accomplishes what you described?

[ahl]

I agree with what Dave's saying. We (I?) knew that these "fail" tests would be potentially be fragile. Most of the time changes have been innocuous such as the compiler underlining slightly different code fragments or whitespace differences. The point of these test is to make sure we continue to produce useful error messages. In at least one case, changes in the output produced a bug in rustc rust-lang/rust#76360 (which admittedly, has languished).

My concern with the approach in this PR is that we'll be stuck on 1.55 indefinitely and won't see regressions in the quality of our error output. Without automating a path to successive toolchains (sort of like dependabot) I'm not sure this is a clear improvement.

[smklein]

I agree with what Dave's saying. We (I?) knew that these "fail" tests would be potentially be fragile. Most of the time changes have been innocuous such as the compiler underlining slightly different code fragments or whitespace differences. The point of these test is to make sure we continue to produce useful error messages. In at least one case, changes in the output produced a bug in rustc rust-lang/rust#76360 (which admittedly, has languished).

If testing the compiler output is a valuable part of the Dropshot Experience™ - and I think it is! - then those tests have value. Definitely want us to be able to track the compiler output across versions, as an example.

However, I don't think pinning toolchains prevents that - it just gives us more explicit control over when this happens. Without using a pinned toolchain, we have no control over when the dropshot commits could start failing. This is a pain, for obvious reasons, but also is a bummer for new devs! See #136 - it sucks to make a small PR and see that the main branch is broken.

My concern with the approach in this PR is that we'll be stuck on 1.55 indefinitely and won't see regressions in the quality of our error output. Without automating a path to successive toolchains (sort of like dependabot) I'm not sure this is a clear improvement.

I dunno, I didn't view this as an "either/or" - we can rev the toolchain whenever we want, we can add automation, and without this, we cannot predict when main will be broken (due to the "fail" tests having such unstable dependencies).

[smklein]

I found renovatebot/renovate#11488 - not done yet, but would give us this feature

[ahl]

I had a longer comment drafted, but here's the argument (of my own) that won me over:

$ git clone https://github.com/oxidecomputer/dropshot
$ cargo test
<fail>

That's unspeakably embarrassing, and that's where we are today. And as much as I do want to be exposed to error message changes, I don't want a casual purveyor of dropshot to justifiably categorize it as junk.

Please proceed!

[davepacheco]

I updated the PR to do the following:

* Include a `Cargo.lock` file, so we can at least control things explicitly

* Specify a `rust-toolchain.toml` file which should match the github action

* Update the test outputs (since `Cargo.lock` was rev'd)

Aside from the automation bit, I think this accomplishes what you described?

I think that mostly does what I said, but like I said, I don't know if this does the right thing for consumers. Did you test that?

[davepacheco]

By the way: I had done the Cargo.lock part in #113, a PR that was approved the day I went on parental leave and then I lost track of it. Sorry about that! I'll get that one landed, too.

Edit: I even wrote there:

Note that this file is not used in consumers' builds, though.

I wish I'd cited a source.

[zephraph]

As an aside that renovate PR is merged and I have a lot of renovate experience. I'd be happy to help out.

[davepacheco]

@zephraph would you propose just adopting renovate for rust-toolchain, or replacing the dependabot configs as well? I don't partifcularly like dependabot and it's not quite doing the right thing. I suspect Renovate would be better. But I also suspect it will take an annoying amount of time to get a new thing right and I'm not sure it's worth the investment right now. What do you think?

[zephraph]

I'm a little biased here.

Dependabot really doesn't give you enough granularity of control. It's often too noisy and definitely lags behind renovate in feature development.

I think it's easy enough to start with the tool chain and then potentially move on to using renovate entirely. Like I said, I've got a lot of experience with renovate. I built out Artsy's config: https://github.com/artsy/renovate-config

[zephraph]

I made some updates to this PR. In hindsight, I should've just PR'd the PR, but it slipped my mind until writing this. Sorry about that.

---

Renovate is enabled as of #235. I borrowed some configuration in the issue @smklein referenced above to enable automatic updates to the toolchain file. You can see that config here: https://github.com/oxidecomputer/renovate-config/blob/main/rust/toolchain.json. It's applied as apart of our default renovate configuration, so right now any repo that has a toolchain configuration file and has renovate enabled will get this update.

I also removed the latest stable version from the github action file. action-rs will use a toolchain config file if it's present so it's easier to centralize the versioning.

Oh, and I updated the version to 1.57.0

[zephraph]

It looks like it's failing b/c action-rs doesn't support the .toml extension for the rust-toolchain file yet. There's an issue for it here: actions-rs/toolchain#208. I'll just remove the .toml extension for now and we can re-add it once action-rs is updated.

[zephraph]

I put in an upstream PR to fix the lack of .toml support: actions-rs/toolchain#209

[zephraph]

After delving a little deeper into actions-rs/toolchain I discovered it's not just the lack of extension, but it doesn't use toml at all. It uses the legacy version of the rust-toolchain which is just the toolchain name and nothing else. I updated this PR to temporarily use my fork until the PR can be merged. If we don't get any real movement on the PR it might be advisable to fork the action into our org.

[davepacheco]

It seems like a lot of this file was reformatted and that makes it a little harder to view the diffs.

[zephraph]

Ah, yeah, sorry about that. I'll revert those changes.

[zephraph]

Ah, one note: You can hide whitespace changes in diffs

I usually have that enabled which is probably why I didn't noticed the formatting diffs.

(I'll still revert them though)

[davepacheco]

Thanks!