Workspace inheritance #6089

dani162 · 2022-09-24T18:39:36Z

Objective

Fixes Use workspace inheritance #6064

Solution

Use the new cargo workspace inheritance feature

Cargo.toml

Co-authored-by: Tristan Guichaoua <33934311+tguichaoua@users.noreply.github.com>

…tests

mockersf · 2022-09-24T21:54:23Z

job "check-missing-examples-in-docs" is missing rust update.
You can update it by adding this code block https://github.com/bevyengine/bevy/blob/main/.github/workflows/ci.yml#L31-L34 in this job https://github.com/bevyengine/bevy/blob/main/.github/workflows/ci.yml#L252

dani162 · 2022-09-24T23:11:51Z

What about the other dependencies, like serde, ron etc, should these be included in the workspace as well?

alice-i-cecile · 2022-09-24T23:52:06Z

So, I'd like to specify a single workspace level version to use. Does including them at the workspace level still let us build the constituent crates without these optional dependencies?

EDIT: looks like the answer is no :(

Dependencies from this table cannot be declared as optional

https://doc.rust-lang.org/cargo/reference/workspaces.html#the-dependencies-table

I suspect we should leave at this for now, rather than shipping pointless dependencies to users of the smaller crates.

mockersf · 2022-09-25T00:15:20Z

What about the other dependencies, like serde, ron etc, should these be included in the workspace as well?

Let's try like this for now, and maybe check later for those. not convinced it would improve a lot

This PR also probably remove the need for https://github.com/bevyengine/bevy/blob/main/.github/workflows/release.yml and https://github.com/bevyengine/bevy/blob/main/.github/workflows/post-release.yml which were there to help with the version updates across all crates. @cart do you have an opinion on keeping the auto workflows (that does not support yet workspace inheritance - crate-ci/cargo-release#499)

dani162 · 2022-09-25T00:19:35Z

So, I'd like to specify a single workspace level version to use. Does including them at the workspace level still let us build the constituent crates without these optional dependencies?

EDIT: looks like the answer is no :(

Dependencies from this table cannot be declared as optional

https://doc.rust-lang.org/cargo/reference/workspaces.html#the-dependencies-table

I suspect we should leave at this for now, rather than shipping pointless dependencies to users of the smaller crates.

I just assumed that u can not set the optional at the workspace level, but u can still set it in the specific dependencies sections of the child creates.
But i am not sure about this behaviour, i am relativ new to rust 😄

Edit:
https://rust-lang.github.io/rfcs/2906-cargo-workspace-deduplicate.html#new-dependency-directives
It seems like it is possible if i understand this correctly.

tguichaoua · 2022-09-25T07:19:48Z

So, I'd like to specify a single workspace level version to use. Does including them at the workspace level still let us build the constituent crates without these optional dependencies?
EDIT: looks like the answer is no :(

Dependencies from this table cannot be declared as optional

https://doc.rust-lang.org/cargo/reference/workspaces.html#the-dependencies-table
I suspect we should leave at this for now, rather than shipping pointless dependencies to users of the smaller crates.

I just assumed that u can not set the optional at the workspace level, but u can still set it in the specific dependencies sections of the child creates. But i am not sure about this behaviour, i am relativ new to rust 😄

Edit: https://rust-lang.github.io/rfcs/2906-cargo-workspace-deduplicate.html#new-dependency-directives It seems like it is possible if i understand this correctly.

Yes, this is actually possible, there is an example here

[project]
name = "bar"
version = "0.2.0"

[dependencies]
regex = { workspace = true, features = ["unicode"] }

[build-dependencies]
cc.workspace = true

[dev-dependencies]
rand = { workspace = true, optional = true }

Important point : crate level features is additive with workspace level features and default-feature cannot be used at crate level, it's inherited from workspace.

dani162 · 2022-09-25T11:12:22Z

Important point : crate level features is additive with workspace level features and default-feature cannot be used at crate level, it's inherited from workspace.

So for these one with the default features disabled we have to stick to the old syntax?

So i need to revert the changes where the default features are disabled, right?
For Example

[dependencies]
bevy_dylib = { workspace = true, default-features = false, optional = true }

should use the old syntax

bevy_dylib = { path = "crates/bevy_dylib", version = "0.9.0-dev", default-features = false, optional = true }

tguichaoua · 2022-09-25T11:43:55Z

Important point : crate level features is additive with workspace level features and default-feature cannot be used at crate level, it's inherited from workspace.

So for these one with the default features disabled we have to stick to the old syntax?

So i need to revert the changes where the default features are disabled, right? For Example
[dependencies]
bevy_dylib = { workspace = true, default-features = false, optional = true }
should use the old syntax
bevy_dylib = { path = "crates/bevy_dylib", version = "0.9.0-dev", default-features = false, optional = true }

Since bevy_dylib is only used in the root cargo.toml, I don't think this is necessary to put it in workspace dependencies.

Also, I think we should first establish rules for when using workspace dependencies or not.
For example if the dependency appears more than once using the same features.

dani162 · 2022-09-25T13:04:14Z

Since bevy_dylib is only used in the root cargo.toml, I don't think this is necessary to put it in workspace dependencies.

This was just an example for one dependency with default features, there are more of them, which are used at more places (e. g. bevy_ecs)
Edit:
Actually only these three:

bevy                -> bevy_dylib
bevy                -> bevy_internal
crates/bevy_app     -> bevy_ecs
crates/bevy_dylib   -> bevy_internal

Also, I think we should first establish rules for when using workspace dependencies or not.
For example if the dependency appears more than once using the same features.

So should i just revert the [workspace.dependency] inheritance changes (3c9f8a3) for now, and we only merge the [workspace.package] inheritances?

dani162 · 2022-10-04T14:58:09Z

So should i just revert the [workspace.dependency] inheritance changes (3c9f8a3) for now, and we only merge the [workspace.package] inheritances?

I will just do that and resolve the merge conflicts, that we can close this

This reverts commit 3c9f8a3.

dani162 · 2022-10-04T15:06:29Z

This pr only inculdes the [workspace.package] inheritances

tguichaoua

LGTM

mockersf · 2022-10-04T16:49:36Z

Could you also add the license field to the package level?

This PR would break the workflows release and post-release as cargo-release doesn't understand yet those workspace value. Either this PR is blocked on support, or those workflows should be removed

dani162 · 2022-10-04T17:31:51Z

Any ideas why this action failed before and now it just passed?

alice-i-cecile · 2022-10-04T18:03:59Z

Flaky test :( There's an open issue.

NiklasEi

I think for most crates, we could also inherit the keywords.

Cargo.toml

bjorn3 · 2022-10-09T18:26:32Z

I think for most crates, we could also inherit the keywords.

No, the graphics keyword doesn't apply to most crates.

NiklasEi · 2022-10-10T06:13:14Z

I think for most crates, we could also inherit the keywords.

No, the graphics keyword doesn't apply to most crates.

I am not saying we should inherit all of them as they currently are defined for the parent crate. A minimum of ["bevy"] can be inherited for sure. That's what most crates currently define.

Co-authored-by: bjorn3 <17426603+bjorn3@users.noreply.github.com>

james7132 · 2022-12-04T04:44:36Z

cargo-release now supports workspace inheritance as of 0.22 which landed on 10/21/2022. Is there any other blocker for this PR?

hymm · 2022-12-05T01:52:18Z

Cargo.toml

@@ -23,6 +10,26 @@ members = [
  "errors",
 ]

+[workspace.package]
+version = "0.9.0-dev"


Suggested change

version = "0.9.0-dev"

version = "0.10.0-dev"

I am not sure if we should bump the version in this pull request, in my opinion we should do this in another pr?
If we do, we also need to update all the dependencies, because the version of the dependencies is not inherited at the moment (see conversation above).
I guess their is an GitHub-Action for that?

.github/workflows/ci.yml

This reverts commit 8507b0e. This reverts commit c6c4271.

cart · 2022-12-05T22:10:09Z

I'd like to hear some arguments for using workspace inheritance for "bevy_x crate version numbers", as right now I'm seeing a number of downsides and no upsides:

Currently, this PR doesn't solve the "version number automation problem" because bevy_x dependencies do not use workspace inheritance. If we chose to add workspace inheritance for these crates, we couldn't use it for "optional bevy_x dependencies", which we have a good number of throughout the workspace. Optional dependencies would need to manually specify the Bevy version. This means that we will still need to run our "release" and "post release" workflows.
For a given crate, this change makes it harder to determine its version number. Users trying to find the Bevy version for a specific crate will need to know how this system works (and find the file where the global version is defined). I would consider this an acceptable loss if it could actually consider the number to be the global source of truth. But as outlined in (1), this is not possible.
In the event that we choose to use inheritance for non-optional bevy_x dependencies, this creates refactor weirdness where we go from bevy_animation = { workspace = true } to bevy_animation = { version = "0.9.0", path = "../bevy_animation", optional = true}.

paul-hansen · 2022-12-06T02:29:09Z

I think you can still use workspace dependencies that are optional, you just declare them as optional at the crate level not at the workspace level. (Sorry if I misunderstood and you already knew that)

Example:

./Cargo.toml

[dependencies]
# Root bevy crate optional dep
bevy_dylib = { workspace = true, optional = true}

[workspace.dependencies]
# Can't use optional here, the crates choose that individually
bevy_dylib = { path = "crates/bevy_dylib", version = "0.9.0", default-features = false }
bevy_math = { path = "crates/bevy_math", version = "0.9.0" }

./bevy_reflect/Cargo.toml

[dependencies]
# bevy_x crate with optional dep from workspace
bevy_math = { workspace = true, version = "0.9.0", features = ["serialize"], optional = true }

dani162 · 2022-12-06T18:55:21Z

I would agree that the version number inheritance is not beneficial for us at the moment, since we can't use the dependency inheritance as we would like to.

There is an open issue/pull-request at the cargo repository about the problem, that you can only use the default features for all or for none packages, if the package inherits the workspace package.
rust-lang/cargo#11409
rust-lang/cargo#11329

If the cargo/rust community decides to allow at least to disable default-features on the workspace level and then enable it at the child, we could use the dependency inheritance. I think this could be possible since in the current rfc (https://rust-lang.github.io/rfcs/2906-cargo-workspace-deduplicate.html) it's not allowed to define the default feature in the package level (but there is no error/warning if you define it anyway, it is just not working if you do so), so it shouldn't be a breaking change.

Then at least we could use the following:

[workspace.dependencies]
foo = { version = "0.1.0", default-features = false}

[dependencies]
foo = { workspace = true, default-features = true }

cart · 2022-12-16T02:37:41Z

I think you can still use workspace dependencies that are optional, you just declare them as optional at the crate level not at the workspace level. (Sorry if I misunderstood and you already knew that)

Ah I wasn't aware of this. That does change things. Does that mean that if a "workspace dependency" is not referenced by a crate anywhere in the workspace, it won't actually be built?

paul-hansen · 2022-12-16T03:29:24Z

Does that mean that if a "workspace dependency" is not referenced by a crate anywhere in the workspace, it won't actually be built?

Hmm, that's what I had thought from reading the docs but I just tried it and I was able to use a dependency from the workspace without specifying it in the child library's cargo.toml at all so I guess I was wrong. Seems less useful than I was originally thinking.

alice-i-cecile · 2022-12-16T06:51:43Z

I'm going to close this out then. If there's a strong reason found I'm happy to try again!

dani162 added 2 commits September 24, 2022 18:09

move workspace to top

f7ed1bb

use version, edition, homepage, readme and repo form workspace

c1a9f85

alice-i-cecile requested a review from mockersf September 24, 2022 18:42

alice-i-cecile added C-Dependencies A change to the crates that Bevy depends on C-Code-Quality A section of code that is hard to understand or change labels Sep 24, 2022

tguichaoua suggested changes Sep 24, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

tguichaoua reviewed Sep 24, 2022

View reviewed changes

Cargo.toml Show resolved Hide resolved

dani162 and others added 2 commits September 24, 2022 23:09

Update Cargo.toml

bfd0203

Co-authored-by: Tristan Guichaoua <33934311+tguichaoua@users.noreply.github.com>

undo workspace inheritance for benches, crates/bevy_ecs_compile_fail_…

67afac3

…tests

dani162 added 3 commits September 25, 2022 00:54

replace bevy dependencies with workspace inheritance

3c9f8a3

use newer rust version

8507b0e

use newer rust version

c6c4271

dani162 added 2 commits October 4, 2022 16:59

Revert "replace bevy dependencies with workspace inheritance"

85074c8

This reverts commit 3c9f8a3.

Merge remote-tracking branch 'origin/HEAD' into workspace-inheritance

efd1c40

dani162 marked this pull request as ready for review October 4, 2022 15:05

tguichaoua approved these changes Oct 4, 2022

View reviewed changes

added licence to workspace inheritances

ac90d37

alice-i-cecile added the S-Blocked This cannot move forward until something else changes label Oct 4, 2022

alice-i-cecile mentioned this pull request Oct 4, 2022

Add support for new workspace inheritance feature of cargo nash-io/cargo-release-action#1

Closed

NiklasEi reviewed Oct 9, 2022

View reviewed changes

bjorn3 reviewed Oct 9, 2022

View reviewed changes

Cargo.toml Show resolved Hide resolved

bjorn3 reviewed Oct 9, 2022

View reviewed changes

Cargo.toml Show resolved Hide resolved

bjorn3 reviewed Oct 9, 2022

View reviewed changes

Cargo.toml Show resolved Hide resolved

add line break

0b9c47a

Co-authored-by: bjorn3 <17426603+bjorn3@users.noreply.github.com>

alice-i-cecile removed the S-Blocked This cannot move forward until something else changes label Dec 4, 2022

mockersf mentioned this pull request Dec 5, 2022

[Merged by Bors] - add rust-version for MSRV and CI job to check #6852

Closed

hymm reviewed Dec 5, 2022

View reviewed changes

dani162 added 2 commits December 5, 2022 11:25

Merge branch 'main' into workspace-inheritance

87f833a

added workspace inheritance bevy-android-example

0593593

mockersf reviewed Dec 5, 2022

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

mockersf reviewed Dec 5, 2022

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

dani162 added 2 commits December 5, 2022 12:03

Revert "use newer rust version"

186c63b

This reverts commit 8507b0e. This reverts commit c6c4271.

use stable rust toolchain

003753d

alice-i-cecile closed this Dec 16, 2022

mockersf mentioned this pull request Sep 22, 2023

Use workspace inheritance to abstract some packages metadata #9896

Closed

BD103 mentioned this pull request May 4, 2024

Inherit uncontroversial workspace metadata #13239

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workspace inheritance #6089

Workspace inheritance #6089

dani162 commented Sep 24, 2022

mockersf commented Sep 24, 2022 •

edited

Loading

dani162 commented Sep 24, 2022

alice-i-cecile commented Sep 24, 2022 •

edited

Loading

mockersf commented Sep 25, 2022 •

edited

Loading

dani162 commented Sep 25, 2022 •

edited

Loading

tguichaoua commented Sep 25, 2022

dani162 commented Sep 25, 2022

tguichaoua commented Sep 25, 2022 •

edited

Loading

dani162 commented Sep 25, 2022 •

edited

Loading

dani162 commented Oct 4, 2022

dani162 commented Oct 4, 2022

tguichaoua left a comment

mockersf commented Oct 4, 2022 •

edited

Loading

dani162 commented Oct 4, 2022

alice-i-cecile commented Oct 4, 2022

NiklasEi left a comment

bjorn3 commented Oct 9, 2022

NiklasEi commented Oct 10, 2022 •

edited

Loading

james7132 commented Dec 4, 2022

hymm Dec 5, 2022

dani162 Dec 5, 2022

cart commented Dec 5, 2022

paul-hansen commented Dec 6, 2022

dani162 commented Dec 6, 2022

cart commented Dec 16, 2022

paul-hansen commented Dec 16, 2022

alice-i-cecile commented Dec 16, 2022

Workspace inheritance #6089

Workspace inheritance #6089

Conversation

dani162 commented Sep 24, 2022

Objective

Solution

mockersf commented Sep 24, 2022 • edited Loading

dani162 commented Sep 24, 2022

alice-i-cecile commented Sep 24, 2022 • edited Loading

mockersf commented Sep 25, 2022 • edited Loading

dani162 commented Sep 25, 2022 • edited Loading

tguichaoua commented Sep 25, 2022

dani162 commented Sep 25, 2022

tguichaoua commented Sep 25, 2022 • edited Loading

dani162 commented Sep 25, 2022 • edited Loading

dani162 commented Oct 4, 2022

dani162 commented Oct 4, 2022

tguichaoua left a comment

Choose a reason for hiding this comment

mockersf commented Oct 4, 2022 • edited Loading

dani162 commented Oct 4, 2022

alice-i-cecile commented Oct 4, 2022

NiklasEi left a comment

Choose a reason for hiding this comment

bjorn3 commented Oct 9, 2022

NiklasEi commented Oct 10, 2022 • edited Loading

james7132 commented Dec 4, 2022

hymm Dec 5, 2022

Choose a reason for hiding this comment

dani162 Dec 5, 2022

Choose a reason for hiding this comment

cart commented Dec 5, 2022

paul-hansen commented Dec 6, 2022

Example:

dani162 commented Dec 6, 2022

cart commented Dec 16, 2022

paul-hansen commented Dec 16, 2022

alice-i-cecile commented Dec 16, 2022

mockersf commented Sep 24, 2022 •

edited

Loading

alice-i-cecile commented Sep 24, 2022 •

edited

Loading

mockersf commented Sep 25, 2022 •

edited

Loading

dani162 commented Sep 25, 2022 •

edited

Loading

tguichaoua commented Sep 25, 2022 •

edited

Loading

dani162 commented Sep 25, 2022 •

edited

Loading

mockersf commented Oct 4, 2022 •

edited

Loading

NiklasEi commented Oct 10, 2022 •

edited

Loading