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

chore: update workspace dependencies #736

Merged
merged 4 commits into from
Mar 22, 2023

Conversation

jonaro00
Copy link
Member

Description of change

Update workspace dependencies.

How Has This Been Tested (if applicable)?

cargo check passes, no other tests done (let's see if CI likes it).

@oddgrd
Copy link
Contributor

oddgrd commented Mar 21, 2023

Awesome @jonaro00, thanks for taking this on! So is this from a cargo update? If that works it should confirm we avoided the ahash cyclical dependency bug, because we hit that the last time we ran cargo update. 🥳

For this PR I think we will also need to update the various Cargo.toml's, however, which makes this a larger undertaking. The main reason is that lockfiles will be ignored by dependants on library crates: https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries Would you be interested in doing that as well?

By the way I am updating rust and cargo separately so we can get that released on monday: #738

@jonaro00
Copy link
Member Author

Yes it is cargo update.

Yeah, the lock file currently only affects binaries such as cargo-shuttle (are there others?), and dependencies while developing. When a user depends on the libraries, they will pick the highest available version compatible with Cargo.toml, so there is not really a big need to bump non-major versions in Cargo.tomls since updates to such dependencies are handled by the dependant. Bumping those versions too much could even cause incompatibilities downstream (kind of like the Tokio pin was doing). I can go through and check for major updates in libs and bump minor ones in binaries.

Also, while I was trying the scripts/upgrade-deps.sh, I realized that it was (1) broken and (2) doesn't make sense: The cargo update at the root level makes sense, but going through all examples etc where there are no lock files checked in and running cargo update only affects the developers' (ignored) lock files, while "checking that the sub-crate builds with the latest deps, which is something that I hope CI is already doing. The update script could be modified to only branch, update root, and commit, but that makes it kind of trivial. I don't see a reason to keep it if lock files in examples etc are not checked in.

I also noticed that the new runtime crate has a lot of unnecessary dependencies, such as axum (from common/backend IIRC). This looks like it would only be of interest if the next feature is used. (Trimming deps in library crates is good if possible)

@oddgrd
Copy link
Contributor

oddgrd commented Mar 21, 2023

Yeah, the lock file currently only affects binaries such as cargo-shuttle (are there others?), and dependencies while developing. When a user depends on the libraries, they will pick the highest available version compatible with Cargo.toml, so there is not really a big need to bump non-major versions in Cargo.tomls since updates to such dependencies are handled by the dependant. Bumping those versions too much could even cause incompatibilities downstream (kind of like the Tokio pin was doing). I can go through and check for major updates in libs and bump minor ones in binaries.

I see what you mean, for example if we bump tokio to "1.26" and a users dependency is pinned to a lower version, that could be a problem (are there other scenarios I am missing here?). Although I don't think that's likely to happen often, bumping our dependencies only when we need to would make it even more rare. Bumping all of them could be simpler, though, and it's what we've done in the past. If you prefer do it conservatively (not bump everything), that's great too!

Also, while I was trying the scripts/upgrade-deps.sh, I realized that it was (1) broken and (2) doesn't make sense: The cargo update at the root level makes sense, but going through all examples etc where there are no lock files checked in and running cargo update only affects the developers' (ignored) lock files, while "checking that the sub-crate builds with the latest deps, which is something that I hope CI is already doing. The update script could be modified to only branch, update root, and commit, but that makes it kind of trivial. I don't see a reason to keep it if lock files in examples etc are not checked in.

I don't think anyone has touched that for a while 😂 It actually runs cargo upgrade which upgrades deps in Cargo.toml. So it could be useful with some tweaks, but I don't think I used it myself last time I did this.

I also noticed that the new runtime crate has a lot of unnecessary dependencies, such as axum (from common/backend IIRC). This looks like it would only be of interest if the next feature is used. (Trimming deps in library crates is good if possible)

Yes, good catch! This is a result of us developing a little bit fast-and-loose to finish up this release. There is certainly room for trimming dependencies, probably by quite a bit, which should also make significant improvements to compile times. The last time we did this thoroughly, mainly trimming the common crate deps and feature flagging, we reduced compile times by almost 1/3. So if you see some easy wins here, it would be terrific to have that as well for this PR. If you see a lot of this, it might also make sense to do it in a follow-up PR.

The common crate has also grown increasingly complex and might be due for a refactor as well, but that might be best to do in a separate PR.

Copy link
Contributor

@oddgrd oddgrd left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you so much @jonaro00 for this cleanup and all these bumps, it was overdue. 🙏

@@ -6,44 +6,48 @@ license.workspace = true
publish = false

[dependencies]
acme2 = "0.5.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

Copy link
Contributor

@iulianbarbu iulianbarbu 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 upgrading the deps! 👍

@oddgrd oddgrd merged commit 891f35e into shuttle-hq:main Mar 22, 2023
@jonaro00 jonaro00 deleted the chore/dependencies-upgrades branch March 23, 2023 20:39
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