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

Deciding how to handle the dependency version matrix #5

Merged
merged 3 commits into from
Mar 14, 2024

Conversation

xStrom
Copy link
Member

@xStrom xStrom commented Feb 28, 2024

As the range of versions a project wants to support increases, so does the complexity of doing so and the tenability of testing them. This RFC puts forth a strategy how to deal with that in a widely applicable manner.

View rendered


Having a one true `Cargo.lock` solves a lot of the contributor and CI issues.
Contributors can easily use the same dependency versions as the CI does.
The CI won't suddenly start producing errors just because a new dependency version was published that causes issues.

Choose a reason for hiding this comment

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

I feel like it would be better if CI did start producing errors in such circumstances. That way we'll know about them and can fix the issue. Most users will be using the latest versions of everything (as noted above), so it's important that this works.

Copy link
Member Author

Choose a reason for hiding this comment

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

We would lose the benefits I outline in the RFC, most notably contributor friction and historic build reproduction. Ultimately I think the CI is a tool meant to help the developers of the project, and so taking trade offs in the developer's favor makes sense.

We still care about these potential breakages. That is why the RFC advocates for regular updates, to not let a single Cargo.lock setup linger for too long.

Also, crucially, the RFC advocates for updating the dependencies just before publishing. So these issues wouldn't be present for users in the immediate time window after release.

Users who already depend on one of our libraries have their own Cargo.lock with a working configuration. If they run cargo update and then their build fails, they can easily revert the Cargo.lock changes.

Thus the only scenario where this would be a real problem is when a user adds one of our libraries as a new dependency after enough time has passed from our release date that one of our sub dependencies is now problematic. But also critically this has to be before one of our regular Cargo.lock updates so that we haven't caught the issue and dealt with it already.

This is about finding a balance, and after careful consideration I belive that commiting Cargo.lock with a regular update cadence is the right trade-off to make. It minimizes the time window where issues could appear for users while greatly improving the contributor experience.

Similar conclusions are being reached in the wider Rust ecosystem and indeed just last year the Cargo team changed their guidance around Cargo.lock to reflect that many in the ecosystem are now committing it for good reason.

On the downsides front, I think that the potential merge conflicts will be a much bigger source of concern for us than users finding broken builds.


### `Cargo.toml` matches `Cargo.lock`

We don't want to claim that our project works with dependency versions that we don't actually test.

Choose a reason for hiding this comment

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

There is experimental cargo support for compiling with minimal library versions. Perhaps we could compile with minimal and latest and assume that everything in between will work. It seems annoying for users if we require a version we don't actually need.

Copy link
Member Author

Choose a reason for hiding this comment

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

As I explain in the section titled Compiling with minimum dependency versions is possible only when stars align those experimental Cargo features don't quite work and so testing minimum versions isn't actually possible. Not in the same workspace anyway.

Can you provide some example scenario where a user would want a SemVer compatible lower version beyond the misguided dependency scenario I outlined in the RFC? Especially because they would have to specifically craft their Cargo.lock with the --precise flag to get such a scenario, which is unlikely to be common.


### MSRV changes are non-breaking

We join the larger Rust community in their choice of declaring MSRV changes as non-breaking.

Choose a reason for hiding this comment

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

Given how frequently we release breaking changes anyway, I think we ought to consider MSRV changes breaking. It seems to me that this would have low cost and be a much better experience for users.

Copy link
Member Author

Choose a reason for hiding this comment

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

Many in the larger Rust ecosystem don't agree with this. As I explained in the RFC it only takes one sub-dependency to bump the MSRV of the whole project. Large Rust projects will inevitably decay to a state where all their published versions have an incorrect rust-version field in the eyes of the current Cargo resolver.

Even if we ignore the sub-dependency issues for the moment and just consider our own code, the situation isn't as clear cut as you suggest.

First, most users won't be running old toolchains. To quote yourself from a sibling comment:

Most people do update almost as soon as a new rust version is release (< 1 week, same day is common)

Second, the experience for the old toolchain users will be indeed better in terms of compilation not breaking after cargo update. Although even that is scheduled to change with Cargo resolver v3. However even today they can either revert their Cargo.lock changes, or use --precise to specifically downgrade the problematic packages. Keep in mind that in today's Rust climate people with old toolchains need to manually craft their Cargo.lock all the time anyway. It's why there is so much work being done by the Cargo team with things like rfc#3537.

However compilation success/failure is only part of the user experience. There's also the ease of gaining knowledge about and access to new features and bug fixes. If MSRV changes are breaking, then simple bugfix patch releases suddenly get promoted to breaking releases. That means they aren't received nor learned about via cargo update.

So the trade-off with having MSRV changes be always breaking is that we improve the compilation experience of a small minority of users still using the affected toolchains, at the cost of them not even learning about our patch releases via cargo update. Plus the bigger cost of most of our users, on qualifying toolchain versions, not receiving the updates either.

While if you flip that and don't consider MSRV changes breaking, well then most users have a smooth experience and receive patches automatically via cargo update. Even the people on too old toolchains will learn of the patches via cargo update, after which they can decide if they want to revert the Cargo.lock changes or update their toolchain.

To me there's no question that considering MSRV not breaking gives our users as a whole more value. Then you add on top of that the fact that our sub-dependencies will be bumping MSRV willy-nilly anyway. The choice crystallises even more in favor of not treating MSRV as breaking.


Our current general CI script is the result of years of experience.
It already has good techniques to avoid some sources of dependency contamination, e.g. feature activations caused by dev dependencies.
It also already pins the stable Rust toolchain version, allowing us to choose when to update it.

Choose a reason for hiding this comment

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

IMO this should be unpinned. Most people do update almost as soon as a new rust version is release (< 1 week, same day is common), and it's important that our libs work with the new version as soon as possible. If clippy is causing issues then the version could be pinned specifically for the clippy check.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is nothing in this strategy that is stopping us from updating the pin the second a new toolchain is released.

Unpinning it would just mean that the errors interfere with unrelated PRs. Someone submiting a typo fix to the docs shouldn't have to start dealing with new toolchain issues.

We ran unpinned toolchains for 4 years in the Linebender repos. Plenty of experience with that. A year ago there was a proposal to start pinning them. I personally was hesitant to do it at first, but after discussing it in more depth in that issue and at office hours we decided to give it a try. Now it's one year later and I can say that it has been a big success. We have had zero cases where the new toolchain failed to compile our code (which might stay hidden until the pin is updated) but we've managed to avoid unrelated warnings and errors adding friction to PRs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will also add that our standard CI already has a docs job with an unpinned nightly toolchain. It won't catch everything, but it does work as a partial canary for certain issues coming from new Rust toolchain features.

@jneem
Copy link

jneem commented Mar 1, 2024

I think some of Nico's concerns can be addressed with automation: have a bot that regularly opens PRs for bumping Cargo.lock and the rust version in CI. This gives you prompt notification of problems (in the form of a PR that fails CI) and makes it easy to keep up-to-date, while maintaining reproducibility of the main branch.

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I think overall this makes sense. I think this could probably do with a more concise summary of what action will actually be taken, such as:

We will start committing Cargo.lock, and update it alongside all dependencies specified in Cargo.toml when we update our stable versions.

We are still missing cron tests for "the latest released version of the crate, but I don't know of anyone actually testing that, so it's probably fine

@xStrom
Copy link
Member Author

xStrom commented Mar 14, 2024

This was discussed in the 2024-03-14 office hours and no blockers were brought up. We recognize that this RFC makes trade-offs in a complicated space and there will be downsides. Even so, the decision is to move forward with its implementation. Further refinements can always come in the future as we learn from experience and the tool ecosystem improves.

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.

4 participants