Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade rust toolchain and use new lockfile format #2620

Merged
merged 3 commits into from
Oct 9, 2020
Merged

Conversation

jurre
Copy link
Member

@jurre jurre commented Oct 9, 2020

Previously we were installing whatever was the latest version
implicitly, and this would upgrade our rust toolchain whenever our
docker caching happened to get busted before rust was installed.

We now explicitly specify a version to install, and I've also upgraded
this to the latest version.

In rust-lang/cargo#7070 the lockfile format was
updated, instead of a top-level [metadata] key that holds the
checksums for dependencies, these checksums are now specified in-line
with the dependency.

Since we shell out to cargo for most things, and in our tests only
explicitly check for the presence of the sha values, this change is
fairly minimal.

I've opted to remove the checksums in most of our fixtures, as they are
not strictly needed in our tests. I could have left them in there and
things would still pass (cargo will silently upgrade them to the new
format), however it seemed noisy.

@jurre jurre requested a review from a team as a code owner October 9, 2020 11:44
@jurre
Copy link
Member Author

jurre commented Oct 9, 2020

Alternatively we could also pin to 1.37.0 that we were (probably? I can check though) using previously, but upgrading seems better to me?

Previously we were installing whatever was the latest version
implicitly, and this would upgrade our rust toolchain whenever our
docker caching happened to get busted before rust was installed.

We now explicitly specify a version to install, and I've also upgraded
this to the latest version.

There is a slight change in the lockfile format, fixed in a follow-up
PR, as to make it easier to pin to an earlier version if desired.
In rust-lang/cargo#7070 the lockfile format was
updated, instead of a top-level `[metadata]` key that holds the
checksums for dependencies, these checksums are now specified in-line
with the dependency.

Since we shell out to `cargo` for most things, and in our tests only
explicitly check for the presence of the `sha` values, this change is
fairly minimal.

I've opted to remove the checksums in most of our fixtures, as they are
not strictly needed in our tests. I could have left them in there and
things would still pass (`cargo` will silently upgrade them to the new
format), however it seemed noisy.
Copy link
Contributor

@thepwagner thepwagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 as you can't argue with CI failing in main but 🤞 not this branch.

Can you walk me through the implications for someone on rust 1.37 / 1.47 today?
This PR seems to support the new style and remove support for the old; does that force users to upgrade? How will we handle old style lockfiles now?

@jurre
Copy link
Member Author

jurre commented Oct 9, 2020

Can you walk me through the implications for someone on rust 1.37 / 1.47 today?
This PR seems to support the new style and remove support for the old; does that force users to upgrade? How will we handle old style lockfiles now?

Old style lockfiles will be upgraded to the new version by cargo (according to the changelog and I also verified this by running on an old lockfile).

The new format was introduced in 1.38.0 (released on 2019-09-26) (according to that same changelog), so I expect most people will already be on that version.

What's new in this version is automatically upgrading the lockfile, but I expect that few projects will be on an incompatible version? I don't know of a way to verify that though 🤔

@feelepxyz
Copy link
Contributor

Have we already been running the new version and upgrading users lockfiles without too much issue feedback?

Inclined to just go for the new version so we don't need to do the whole dance to install multiple version and switch at runtime.

@thepwagner
Copy link
Contributor

@jurre awesome, thanks! A regression test that upgraded an old style fixture to new would have been warm/fuzzy, but your description is 💯 - the old style has been out for so long I don't think it's worth maintaining.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants