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

Adopt a new dependency updating strategy. #990

Merged
merged 3 commits into from
May 26, 2020

Conversation

xStrom
Copy link
Member

@xStrom xStrom commented May 26, 2020

I wrote much of the rationale for this PR as documentation in CONTRIBUTING.md under a new Preparing for a new release section. I'm not totally sure if CONTRIBUTING.md is the correct place for this kind of stuff, as it is more geared towards existing contributors than new ones. It is meant for contributors though, so perhaps it's just the right place.

This PR deletes Cargo.lock because it causes more harm than good. It's generally not a good practice. It won't actually be used by any developers using druid as a dependency. Its primary use right now is to force the CI (and people working on druid) to use a specific set of dependency versions. Versions that no developer using druid will use. I go more deeply into this in the docs I wrote.

I also updated dependencies in preparation for druid 0.6.0.

PS. Although I don't mention this in the docs, I do have a real world example in the form of proc-macro-hack which was broken, stopped compiling on nightly, and was fixed with a patch release. A bunch of packages like time-rs still claim to work with the older broken version, which I think is a mistake. That cascaded as breaking our builds on nightly too.

@xStrom xStrom added maintenance cleans up code or docs docs concerns documentation S-needs-review waits for review labels May 26, 2020
@xStrom xStrom changed the title Adapt a new dependency update strategy. Adopt a new dependency updating strategy. May 26, 2020
Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

Thanks a lot for writing this down!

I love such write-ups, but I don't think everyone wants to read through the whole section. A TLDR for the main dependencies section, just stating that we only test the newest versions and can't verify the old ones, would probably be the best. And maybe move the details after the 'how to upgrade' section as I could look a bit intimidating.

CONTRIBUTING.md Outdated
Comment on lines 89 to 91
Now we'll use the `cargo upgrade` command to update all the versions in the `Cargo.toml`
files to match the versions specified in `Cargo.lock`. It's crucial that we use the
`--to-lockfile` option, because without it `cargo upgrade` won't respect semver.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now we'll use the cargo upgrade --to-lockfile command...

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 reworded this whole paragraph so that it's more clear that --to-lockfile needs to be used.

@alerque
Copy link

alerque commented May 26, 2020

I agree. This concept goes for more than just Rust, tracking the *.lock file doesn't make sense for libraries if that only applies for building the library and not for the downstream ecosystem. Locking dependencies in the final stage of the food chain makes a lot of sense, but not here.

@xStrom
Copy link
Member Author

xStrom commented May 26, 2020

I'm no fan of TLDR, but I'm willing to compromise so I added a bold introduction sentence that summarizes the text.

I kept the description of why the updating is being done before showing the commands how to do it. To me that is the most logical order of steps.

I do want to address the intimidation factor though, so I added a new intro paragraph explaining that the release preparation stuff is meant primarily for existing contributors and also serves as a checklist. That should make it more clear to newcomers that they don't need to read it.

Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

That should be fine 👍

I've tested some of the examples on Linux and WASM and everything seems to work.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

just blocking to read 😁

Copy link
Member

@cmyr cmyr 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 it's good to be clear about this stuff, and I think it's good to delete Cargo.lock.

On aggressively updating to minor versions: I would be curious to know if there is an accepted practice for this stuff. My inclination is to generally (with, say, serde as a concrete example) keep the version in our cargo.toml at 1.0. The motivation here is that it allows maximum flexibility; if we have serde at 1.22.0 and a client is using another library with serde = 1.17, we are now forcing them to include two versions of serde.

A lot of this comes down to how much we trust semver. as mentioned inline, my inclination is to trust; if an author breaks it we should report upstream and they should yank that release, and I would expect most library authors to do this. If we encounter a specific concern around something like this, we could always repin that specific dependency.

so I'm happy to be convinced here, but I want to make sure we're thinking it through. What I would find really compelling is a link to some discussion from a widely used rust library explaining how they approach this problem, and their rationale; I want to make sure we get this right.

Because the final application chooses the sub-dependency versions and they are most likely
going to be higher than the minimum that is specified in our `Cargo.toml` file,
we need to make sure that druid works properly with these newer versions.
Yes according to [semver] rules they should work, but library authors make mistakes
Copy link
Member

Choose a reason for hiding this comment

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

I am inclined to trust reported semver versions, and also trust maintainers of crates we use (and those should generally be few crates, of a high standard) to yank releases that break semver.

@totsteps
Copy link
Collaborator

totsteps commented May 26, 2020

Thanks for writing this up.

Should we also include, that the grouping order of imports should be:
std / external / crate / super / descendants?

@xStrom
Copy link
Member Author

xStrom commented May 26, 2020

In your example where druid has serde = "1.22.0" and another library has serde = "1.17" will result in only one copy of serde at the latest version that still starts with one - that is to say 1.X.X. That's because both druid's and that other library's specification is a minimum version and allows semver compatible updates. Neither of those specifications demand a specific version beyond the minimum.

As for reporting if someone breaks semver - I absolutely agree. That's one of the main goals of this strategy change. Because right now with the current Cargo.lock approach we'll never know if something newer breaks things.


For a widely used example, there's Rust itself.

There's rust#57435 where a question is asked:

Is there any particular reason why we're not bumping the version in the various Cargo.tomls?

.. and then answered by a Rust team member:

I see that as an optional but probably recommended step, I don't think we're trying to avoid that at all.

Then there's rust#57443 where there is discussion and movement towards automating this. This is the thread from where I found cargo upgrade --to-lockfile.


So ultimately the changes boil down to two points:

  1. Removing Cargo.lock. If we stick with Cargo.lock then our testing won't match the dependencies that any developer is using. We'll be in our own little echo chamber. We won't be able to report any newer versions breaking something accidentally, because we'll never know. The solution is to remove Cargo.lock and this is explictly recommend.

  2. Now that we've removed Cargo.lock we're only testing the newest versions. The Cargo.toml file can contain a minimum specification of a dependency that has some bug that would actually break druid - but was never discovered back when we were using that version because the druid feature didn't exist yet. That older version might not even have a whole method that druid depends on, because that method was added with a minor version upgrade, which is legal semver.
    The goal of increasing the minimum versions is to make our technical claims match our actual methodology. We're not testing the older versions and we have no real clue if the older versions still work. So we shouldn't be claiming that they are guaranteed to work in our Cargo.toml files.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Thanks for the links, that's helpful, I think this makes sense.

@xStrom xStrom removed the S-needs-review waits for review label May 26, 2020
@xStrom xStrom merged commit 780fa88 into linebender:master May 26, 2020
@xStrom xStrom deleted the semver-lib branch May 26, 2020 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs concerns documentation maintenance cleans up code or docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants