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

refactor: Deduplicate dependency versions #5691

Merged
merged 2 commits into from
Jun 17, 2024
Merged

Conversation

Hocuri
Copy link
Collaborator

@Hocuri Hocuri commented Jun 13, 2024

Deduplicate dependency versions by specifying them only once in Cargo.toml for the whole workspace under [workspace.dependencies].

Where possible, also deduplicate the dependencies' features (if different crates use different set of features, of course these can't be deduplicated).

@Hocuri Hocuri requested review from link2xt and iequidoo June 14, 2024 20:59
@iequidoo
Copy link
Collaborator

iequidoo commented Jun 15, 2024

Where possible, also deduplicate the dependencies' features (if different crates use different set of features, of course these can't be deduplicated).

They are actually deduplicated this way (https://doc.rust-lang.org/cargo/reference/resolver.html#features):

When building multiple packages in a workspace (such as with --workspace or multiple -p flags), the features of the dependencies of all of those packages are unified. If you have a circumstance where you want to avoid that unification for different workspace members, you will need to build them via separate cargo invocations.

So, each dependency is built only once with all requested features. Not a problem if we don't have any dependencies with mutually exclusive features.

EDIT: But i think it's better to use Feature resolver version 2.
EDIT: We have edition = "2021" in Cargo.toml, so we're already using the Feature resolver v2 actually.

@Hocuri
Copy link
Collaborator Author

Hocuri commented Jun 15, 2024

They are actually deduplicated this way (https://doc.rust-lang.org/cargo/reference/resolver.html#features):

I know, what I meant is: deltachat-ffi needs yerpc only with features ["anyhow_expose"]. deltachat-jsonrpc needs it with ["anyhow_expose", "openrpc"]. So, I didn't put any of the features into [workspace.dependencies].

This is purely a choice what we like better; I could as well have put the anyhow_expose into [workspace.dependencies], since all crates that use yerpc enable this feature. Or I could have put no feature flags at all into [workspace.dependencies], not deduplicating them. I chose the described middle ground.

If you have a circumstance where you want to avoid that unification for different workspace members, you will need to build them via separate cargo invocations.

That's what we do in this case, we use one cargo invocation to build deltachat-ffi and another one to build deltachat-jsonrpc. This is not changed by this PR.


Not sure I explained everything in an understandable way, but also, it's not that important, I think it's fine to ignore that I ever mentioned features :)

@iequidoo
Copy link
Collaborator

I chose the described middle ground.

I'd prefer not to deduplicate any features at all, otherwise if a new package not requiring the given feature appears, you should "duplicate the features back". Or deduplicate a feature only if there are some strong reasons to use it in all packages. I think this way it's just easier to maintain dependencies' features.

Copy link
Collaborator

@iequidoo iequidoo left a comment

Choose a reason for hiding this comment

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

Wrt the dependencies' features, it doesn't look critical anyway, but i'd wait for @link2xt's approval before merging

@Hocuri Hocuri changed the title chore: Deduplicate dependency versions refactor: Deduplicate dependency versions Jun 16, 2024
@Hocuri Hocuri enabled auto-merge (squash) June 16, 2024 21:24
@Hocuri Hocuri force-pushed the hoc/deduplicate-dependencies branch from cf1fa8a to 675132f Compare June 17, 2024 07:37
@Hocuri Hocuri merged commit a5d14b3 into main Jun 17, 2024
37 checks passed
@Hocuri Hocuri deleted the hoc/deduplicate-dependencies branch June 17, 2024 07:51
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.

None yet

3 participants