Update rust ci version to be explicitly numbered

[lord]

No description provided.

[xStrom]

What's the rationale for this? We've been doing quite fine with using the latest stable version.

Edit: Aha I see that your branch is named clippy-fix. So the way we fix clippy errors is by changing the code based on the clippy suggestions. In rare cases we add a clippy ignore attribute if we disagree with a specific lint.

[lord]

As you can see from the now failing build of this PR, the main branch is failing CI because clippy lints are very not stable between Rust versions. I think it's a frustrating contributor experience to first have to fix unrelated clippy lints before getting your change in, and so this is a proposal to switch to upgrading the Rust version used in CI manually.

[lord]

I think the cost of having a slightly older version of Rust running in CI is very low, and the cost of having the build randomly break once a month is somewhat higher. Although it sounds like maybe you prefer auto-updating to the latest stable?

[jneem]

I'm in favor of pinning a version for CI. (We can also run latest stable, but not have it be required for merging.) With the current level of activity on this repo, a significant fraction of PRs get blocked on clippy complaining about issues not related to that PR.

[xStrom]

What you're saying has some merit, but unfortunately it doesn't fully resolve the issue. It just shifts it.

If the CI is pinned to an older version, then all contributors also need to be using an older Rust version on their own machines. I think the Rust culture is heavily biased towards people doing rustup update very frequently. So contributors with newer Rust versions would either not run clippy at all (which we don't want) or have their clippy fail and we would be less aware because the CI would still be green.

The issue of CI failing is still real though. The solution I've been moving towards is solving clippy issues with the beta toolchain. This very same issue for example was resolved during beta for Druid in druid#2363.

[xStrom]

There is another issue with pinning the CI. It doesn't actually introduce a brand new issue, but it greatly increases its scope.

Imagine a PR that is waiting for review for some time. As long as the modified files don't conflict with changes in main, this PR branch doesn't need to be rebased on main. That means the CI for this PR is always running 1.68.

Meanwhile 1.69 has come out and main has an updated CI config.

Because there is no conflict with the PR branch, all changes to the PR continue to run with clippy 1.68. Once merged, suddenly main will be broken.

If the CI config has stable, then any fresh trigger of CI will actually use 1.69 and catch the new errors before merge. Though yes there is still a chance of main breakage if the PR is sitting without changes for a long time and then merged without rerunning the CI (e.g. due to a new commit).

[jneem]

If the CI is pinned to an older version, then all contributors also need to be using an older Rust version on their own machines.

Could you elaborate on this? I don't see why it's a problem: if I'm using a newer rustc than CI, I might get a few clippy warnings and then I have the option to fix them or not. And if the clippy warnings are in code that I'm not touching, I can fix and PR them later instead of having to fix them first.

The other thing that happened to me fairly regularly in druid-shell is that I'll miss clippy warnings because they're on a different platform or feature-gated out. I used to have a git hook to run clippy on all targets, but I got rid of it because it took lots of time and disk space...

[xStrom]

I might get a few clippy warnings

It's not uncommon to have 10 warnings, sometimes even 30. A real warning that is part of your new code is among them. I don't think it's trivial to see which are which. You have to read through the output every time or use some sort of IDE that does squigglies.

I used to have a git hook

That's also an important point. These hooks don't differentiate between your code and old code, they just treat all warnings the same.

I'm not saying that it's impossible, but I am saying it would be a brand new annoyance to deal with.

---

Ultimately my perspective here is that main should pass clippy without errors on the latest stable version. Pinning just hides the symptoms. This doesn't mean that main can be left to rot, like happened here.

I would advocate for a more systematic approach across repos. A train schedule based on the Rust release schedule. Ensuring that main passes beta clippy e.g. 1 week before that beta is promoted to stable. A proactive approach based on policy, not a reactive approach based on last minute breakage discovery.

[lord]

How about a compromise here? A proactive, train-like upgrade schedule based on Rust releases sounds like a good idea, and I'd be in favor. But it would be great if, were we to slip and take an extra couple of weeks to do the fix, we don't break the main CI build for everybody. Perhaps we could keep the upgrade schedule as you propose, but rather than upgrading and releasing the week before the Rust release, we upgrade and release within a couple days after the Rust version bumps. The version of Rust would be explicitly specified in CI, and the scheduled release would bump this version number as well as fix any associated clippy lints.

[lord]

Because there is no conflict with the PR branch, all changes to the PR continue to run with clippy 1.68. Once merged, suddenly main will be broken.

I think this is a valid concern. Perhaps we should use commit queues or bors or something like that?

[xStrom]

How about a compromise here?

Perhaps we should use commit queues

Maybe. I'll allocate some time for thinking this stuff through more thoroughly on Monday, including looking at merge queues etc. Then I can give a more informed reply.

[lord]

Sounds good, thanks for considering all this!

[richard-uk1]

This is the kind of thing that might be worth bringing up at the office hours.

[xStrom]

I've been looking into the merge queues and initial impressions are promising. I haven't found as much time to think about this as I'd like, but hopefully that will happen soon. It does look like we might be able to change policies in a way that will alleviate the issues of pinning a version.

Unfortunately we ran into technical difficulties during office hours today, so couldn't talk about it in any depth. Will try again next week to bring up this topic for discussion and meanwhile I'll continue gathering my thoughts.

[xStrom]

We talked about this at the office hours. Unfortunately instead of simple convergence there were even more differences in takes added to the list. That said, we still managed to decide to test run a variant of the compromise we talked here. If it works out, the policy can be applied to other linebender repos as well.

So to be more explicit, what we'll test run here is:

• Instead of stable we'll specify an explicit version. However let's do MAJOR.MINOR without PATCH so that bugfixes still come automatically.
• We'll use merge queues in an attempt to mitigate potential main breakage due to PR branches having stale CI configs.
• We'll have a more formal routine of upgrading the pinned version number along with fixing clippy to avoid main failing with the latest stable Rust.

So to align this PR here with the new plan, please do the following:

• Change the pinned version from 1.67.0 to 1.68
• Restore the beta testing (this may end up being removed later, but that's a separate question, and also beta clippy failures don't block PRs)

[lord]

Sounds good to me! How does this look?

[xStrom]

This is good for merging, thanks!