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

Move all internal dependancies into the workspace. #17486

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

AlephCubed
Copy link
Contributor

@AlephCubed AlephCubed commented Jan 21, 2025

This moves all internal crate dependancies into the root Cargo.toml, which can be referenced using workspace = true.

Fixes #16652.

Default features

Because of the way workspace dependancies work, default-features are now opt-in (see this warning for details).

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-Cross-Cutting Impacts the entire engine X-Contentious There are nontrivial implications that should be thought through S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 22, 2025
bevy_derive = { workspace = true, default-features = true }
bevy_log = { workspace = true, default-features = true }
bevy_math = { workspace = true, default-features = true }
bevy_reflect = { workspace = true, default-features = true, features = [
Copy link
Member

Choose a reason for hiding this comment

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

Won't this enable these features for all of the crates that rely on bevy_reflect across the workspace? My understanding was that only a single version of the crate is selected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to this, it seems like that is already the case for the current method:

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.

This post goes over some of the pitfalls of dependancy unification, but that appears to apply to workspaces in general, not just workspace level dependancies.

But I could totally be wrong here. It's kinda confusing.

Copy link
Contributor

@BenjaminBrienen BenjaminBrienen Jan 22, 2025

Choose a reason for hiding this comment

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

I think the main concern is when a project has a direct dependency on a crate like bevy_ecs, they shouldn't have extra features due to the workspace structure.

Copy link
Contributor Author

@AlephCubed AlephCubed Jan 22, 2025

Choose a reason for hiding this comment

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

I don't think that will be effected. Only members built at the same time should have their dependencies unified.

Just beware that if you build multiple workspace members at the same time, the features will be unified so that if one member sets default-features = true (which is the default if not explicitly set), the default-features will be enabled for all members using that dependency.

But upon rereading this, I think I was wrong about default-features being opt out when disabled on a workspace level.
I will see about fixing that tomorrow.

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 just tested running cargo tree -p bevy_ecs -e features and bevy_reflect has no features enabled, same as in main.
So if you compile an individual sub-crate, only its required features should be enabled.

@BenjaminBrienen BenjaminBrienen added the D-Straightforward Simple bug fixes and API improvements, docs, test and examples label Jan 22, 2025
@mockersf
Copy link
Member

blocked on dependabot/dependabot-core#7896

@mockersf mockersf added the S-Blocked This cannot move forward until something else changes label Jan 22, 2025
@alice-i-cecile alice-i-cecile removed the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Cross-Cutting Impacts the entire engine C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Blocked This cannot move forward until something else changes X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Cargo workspace features for internal dependency versions
4 participants