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

Use cargo's workspace.package and workspace.dependencies #595

Merged
merged 9 commits into from
Dec 16, 2022

Conversation

flub
Copy link
Contributor

@flub flub commented Dec 14, 2022

This switches to use cargo's new workspace.package and
workspace.dependencies features. It reduced duplication in the cargo
files.

A number of dependency versions where unified through this already.

This uses the new (rust 1.64) workspace.package cargo feature to
reduce duplication and copy-paste errors.
@flub flub added the dependencies Pull requests that update a dependency file label Dec 14, 2022
@Arqu
Copy link
Collaborator

Arqu commented Dec 14, 2022

Holly molly, didn't know this was a thing. A+++ for this.
Question, is there a CI check we can integrate to catch these. Seems like it will be hard to maintain long term.

@flub
Copy link
Contributor Author

flub commented Dec 14, 2022

Holly molly, didn't know this was a thing. A+++ for this.

It's pretty new, I think 1.64. So only the merging of quic-rpc bumping MSRV to 1.65 enabled us to use this.

Question, is there a CI check we can integrate to catch these. Seems like it will be hard to maintain long term.

You mean catching a dependency added to a crate's [dendencies] table rather than the workspace's [workspace.dependencies]? Not unless we write some kind of script ourself· I guess this isn't too difficult: parse the Cargo.toml, check the things inside the dependecies table that they all have a workspace: true in them.

Though I'm not sure how much of a problem it will be: if we consistently use them it should be pretty obvious in code review that it's forgotten. So my instinct would be to try it first without enforcement and only later add enforcement if it turns out it is needed.

clap = { version = "4.0.9", features = ["derive"] }
futures = "0.3.21"
clap.workspace = true
futures.workspace = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i realise this example still needs cleaning up. i'll do this once there's approval in principle. this might also race the iroh-embed pr - so one of them will need fixing up anyway. and examples are touched by that pr

@dignifiedquire
Copy link
Contributor

I am concerned about a couple of things here

  • tooling support for this, need at least cargo add, cargo tree, cargo outdated and cargo release fully working
  • enabling unnecessary features: each crate should only enable the features it absolutely needs, as it might be used independently of the other crates in the workspace
  • maintainability: this makes it considerably harder to look at & understand the dependencies of a single crate, especially when working on sth in isolation

@flub
Copy link
Contributor Author

flub commented Dec 14, 2022

I am concerned about a couple of things here

* tooling support for this, need at least `cargo add`, `cargo tree`, `cargo outdated` and `cargo release` fully working
  • doing a cargo add of a package already in the [workspace.dependencies] table results in the right thing: *pkgname.workspace = true is added to the crate where cargo-add is invoked.
  • cargo tree just keeps working.
  • cargo outdated does not: Support workspace inheritance kbknapp/cargo-outdated#325 (to be honest, I didn't even know of this one!). cargo-udeps is already fixed.
  • cargo release is part of rust itself, so I assume this works - it is a bit hard to just try.
* enabling unnecessary features: each crate should only enable the features it absolutely needs, as it might be used independently of the other crates in the workspace

Absolutely, and this is still possible. I think this is mostly a policy thing though and why I wrote down how I decided what feature to put where. To keep minimal perhaps the policy should change that by default as little features as possible are enabled in the [worspace.dependencies] table, only those that we explicitly want everywhere like e.g. anyhow's backtrace.

Happy to make that change if we prefer. I couldn't decide myself here what was easier to work with.

* maintainability: this makes it considerably harder to look at & understand the dependencies of a single crate, especially when working on sth in isolation

Not sure I really agree. You look at that crate's Cargo.toml and see all the dependencies. If we change how we select features to also be primarily in the crate Cargo.toml files then all information is there other than the version number. Which seems practical enough.

@dignifiedquire
Copy link
Contributor

https://github.com/crate-ci/cargo-release is not a part pf cargo

@flub
Copy link
Contributor Author

flub commented Dec 14, 2022

https://github.com/crate-ci/cargo-release is not a part pf cargo

fair enough, anyway - supported since v22 on 21 Oct 2022: https://github.com/crate-ci/cargo-release/releases/tag/v0.22.0

dignifiedquire
dignifiedquire previously approved these changes Dec 15, 2022
Copy link
Contributor

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

haven’t reviewed every line, but lets try it out, if it doesn’t work we can always revert it

dignifiedquire
dignifiedquire previously approved these changes Dec 16, 2022
@flub flub merged commit b409187 into main Dec 16, 2022
@flub flub deleted the flub/workspace-package branch December 16, 2022 13:42
ppodolsky pushed a commit to izihawa/iroh that referenced this pull request Dec 28, 2022
This uses the new (rust 1.64) workspace.package cargo feature to
reduce duplication and copy-paste errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants