Lift dependencies to the workspace (Part 1) #2070

ggwpez · 2023-10-27T19:21:01Z

Changes (partial #994):

Set log to 0.4.20 everywhere
Lift log to the workspace

Starting with a simpler one after seeing #2065 from @jsdw.
This sets the default-features to false in the root and then overwrites that in each create to its original value. This is necessary since otherwise the default features are additive and its impossible to disable them in the crate again once they are enabled in the workspace.

I am using a tool to do this, so its mostly a test to see that it works as expected.

bkchr

We should not disable the default features in the workspace. IMO this "feels" weird.

And BTW, the wasm-builder currently not supports workspace deps.

bkchr · 2023-10-28T07:59:56Z

Cargo.toml

@@ -4,6 +4,9 @@ edition = "2021"
 repository = "https://github.com/paritytech/polkadot-sdk.git"
 license = "GPL-3.0-only"

+[workspace.dependencies]
+log = { version = "^0.4.20", default-features = false }


Suggested change

log = { version = "^0.4.20", default-features = false }

log = "0.4.20"

I saw that if the default-features are not disabled in the workspace, then they cannot be disabled lateron in the crates, since features are additive.
So AFAIK it is best practice to disable them at the workspace and then re-enable at the crate level - if desired. It is explained here rust-lang/cargo#11329

bkchr · 2023-10-28T08:00:33Z

cumulus/client/consensus/common/Cargo.toml

@@ -10,7 +10,7 @@ async-trait = "0.1.73"
 codec = { package = "parity-scale-codec", version = "3.0.0", features = [ "derive" ] }
 dyn-clone = "1.0.12"
 futures = "0.3.28"
-log = "0.4.20"
+log = { workspace = true, default-features = true }


Suggested change

log = { workspace = true, default-features = true }

log = { workspace = true }

This and all the other places you have done this "feel weird"

Yea I think the default-features = true should be implicit in this case, so we should be able to omit it.

👍

I think it makes sense to do default-features = false in the workspace Cargo.toml on a case-by-case basis if we actually don't want to always enable all of the features for a given dependency, but this probably shouldn't be done by default (at least not blindly), and if you're adding a default-features = true all over the place then it's most likely the wrong thing to do. (If a crate needs a specific feature it should explicitly say which one, instead of doing default-features = true.)

And in case of the log it looks like it doesn't even have any default features.

And in case of the log it looks like it doesn't even have any default features.

Hm. Then we would need to check on every version update whether some default features were added? I guess that could work since the WASM build should fail when it tries to pull in std.

and if you're adding a default-features = true all over the place then it's most likely the wrong thing to do.

Yes I agree if this is the case in 100% of the occurences (and under the assumption that this would not change in the future). But for a dependency where we sometimes want default-features and sometimes not, then we need to set it to false in the workspace i think.

If a crate needs a specific feature it should explicitly say which one, instead of doing default-features = true

If this is the goal, then i dont think that it is automatable.

What a fun ❤️

I ran into a similar thing when I tried this; you need to specify default-features = false in the worksapce in order to be able to use default-features on that dependency in the crate's Cargo.toml, else you get a warning like:

warning: ..polkadot-sdk/polkadot/xcm/Cargo.toml: `default-features` is ignored for scale-info, since `default-features` was not specified for `workspace.dependencies.scale-info`, this could become a hard error in the future

Yes, this is what @ggwpez linked above.

So can we go ahead with this:

default-features = false in the root workspace for all crates that have a default feature

nothing in the crates if the default-features should be true

default-features = false in each crate like before when they should be disabled.

This should make the diff here a bit shorter.

If that works, lets do this.

ggwpez · 2023-10-28T10:27:45Z

And BTW, the wasm-builder currently not supports workspace deps.

What does this mean? That we cannot use it for the no-std runtime dependencies?
Then i will continue with other deps after this PoC is done.

bkchr · 2023-10-28T17:05:24Z

And BTW, the wasm-builder currently not supports workspace deps.

What does this mean? That we cannot use it for the no-std runtime dependencies? Then i will continue with other deps after this PoC is done.

Anything crate that is using wasm builder will not work right now with a workspace dependency. We just need to add support for this.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez · 2024-02-02T13:25:48Z

Anything crate that is using wasm builder will not work right now with a workspace dependency. We just need to add support for this.

How to do this? I see that all CI checks are green. It should catch that or not @bkchr?

bkchr · 2024-02-05T11:24:37Z

Anything crate that is using wasm builder will not work right now with a workspace dependency. We just need to add support for this.

How to do this? I see that all CI checks are green. It should catch that or not @bkchr?

Thinking again about this, my comment wasn't that correct. As we include the real runtime crate as dependency, it is fetching its deps from its original workspace. So yeah, just continue to merge this pr!

substrate/client/consensus/common/Cargo.toml

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez · 2024-02-09T11:46:30Z

@koute i guess you are quite busy, but i settled on this idiomatic pattern now:

put default-features = false in the workspace
re-enable them per-crate with default-features = true if they were previously enabled and leave them disabled otherwise.

I think this pattern is applicable to every dependency, so we dont have to think much and just use some tool to replace it.

koute

Looks mostly good to me, although in this specific case of log all of the default-features are pretty much useless since the crate doesn't actually have any default features in the first place, so it's just noise that will confuse people.

So I would suggest that we just use a bare log = { workspace = true } everywhere.

ggwpez · 2024-02-12T11:14:53Z

Looks mostly good to me, although in this specific case of log all of the default-features are pretty much useless since the crate doesn't actually have any default features in the first place, so it's just noise that will confuse people.

So I would suggest that we just use a bare log = { workspace = true } everywhere.

Yes this could be done for now. But if we ever update log and it gets default features, then we would need to change it again.
And at that point we already lost the information in which crates we would like to enable them, since we removed the annotation.

koute · 2024-02-12T11:51:24Z

Yes this could be done for now. But if we ever update log and it gets default features, then we would need to change it again. And at that point we already lost the information in which crates we would like to enable them, since we removed the annotation.

I don't see a problem with this, considering any changes to the default features will have to be semver-compatible anyway.

And the "lost the information in which crates we would like to enable them" is, well, complete nonsense considering we're talking about features which don't exist yet. (: How do you know we want to enable a feature if we don't know what that feature is?

Just because a given crate might enable some kind of a feature by default has nothing to do with whether we want to enable or disable it. And there is also no rule that in the client we should enable all of the features by default and in the runtime disable them. A given feature's value lies in what exactly that feature does, and not in whether it's enabled by default or not.

ggwpez · 2024-02-12T12:24:19Z

I don't see a problem with this, considering any changes to the default features will have to be semver-compatible anyway.

Oh right. So it should not make a difference anyway until there is a major bump 🙃

And the "lost the information in which crates we would like to enable them" is, well, complete nonsense considering we're talking about features which don't exist yet. (: How do you know we want to enable a feature if we don't know what that feature is?
Just because a given crate might enable some kind of a feature by default has nothing to do with whether we want to enable or disable it. And there is also no rule that in the client we should enable all of the features by default and in the runtime disable them. A given feature's value lies in what exactly that feature does, and not in whether it's enabled by default or not.

Yea true. I only thought about our convention of treating default-features like std.
For the sake of this MR i think its still fine to do, since it only transforms the existing code into a more readable format, without making assumptions about whether that code is correct or not (garbage in = garbage out).
Something that i plan to repeat for more dependencies.

ggwpez · 2024-02-16T17:53:23Z

I changed the default logic now to not write default-features = false if they are enabled everywhere.
This resulted in a more readable diff here: #3366

@jsdw

Changes (partial paritytech#994): - Set log to `0.4.20` everywhere - Lift `log` to the workspace Starting with a simpler one after seeing paritytech#2065 from @jsdw. This sets the `default-features` to `false` in the root and then overwrites that in each create to its original value. This is necessary since otherwise the `default` features are additive and its impossible to disable them in the crate again once they are enabled in the workspace. I am using a tool to do this, so its mostly a test to see that it works as expected. --------- Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez added the R0-silent Changes should not be mentioned in any release notes label Oct 27, 2023

ggwpez marked this pull request as ready for review October 27, 2023 21:27

ggwpez requested review from a team October 27, 2023 21:27

ggwpez requested review from athei, acatangiu, andresilva, sorpaas, jsidorenko and cheme as code owners October 27, 2023 21:27

ggwpez requested a review from a team October 27, 2023 21:27

ggwpez requested review from koute and a team as code owners October 27, 2023 21:27

paritytech-ci requested a review from a team October 27, 2023 21:28

bkchr reviewed Oct 28, 2023

View reviewed changes

ggwpez self-assigned this Oct 28, 2023

ggwpez mentioned this pull request Nov 6, 2023

serde_json: bumped to 1.0.108 #2168

Merged

ggwpez requested review from a team as code owners January 13, 2024 15:21

ggwpez added 2 commits January 13, 2024 16:23

Uniformly use log 0.4.20

79e081a

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Lift log dependency to workspace

962c19a

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez force-pushed the oty-lift-deps branch from 89f9cd0 to 962c19a Compare January 13, 2024 15:24

bkchr reviewed Feb 5, 2024

View reviewed changes

substrate/client/consensus/common/Cargo.toml Outdated Show resolved Hide resolved

ggwpez added 3 commits February 6, 2024 14:08

Reset to master

78a8f1e

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Merge remote-tracking branch 'origin/master' into oty-lift-deps

726c2f7

Lift log to the workspace

cf12657

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez requested a review from a team as a code owner February 6, 2024 15:37

Fmt toml

da8391e

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez requested a review from bkchr February 7, 2024 13:39

bkchr approved these changes Feb 12, 2024

View reviewed changes

koute approved these changes Feb 12, 2024

View reviewed changes

svyatonik approved these changes Feb 12, 2024

View reviewed changes

ggwpez added this pull request to the merge queue Feb 12, 2024

Merged via the queue into master with commit e80c247 Feb 12, 2024
130 checks passed

ggwpez deleted the oty-lift-deps branch February 12, 2024 11:57

ggwpez mentioned this pull request Apr 2, 2024

Lift all dependencies to the workspace #3949

Closed

ggwpez mentioned this pull request May 6, 2024

Switch to workspace deps - batch 1 #4391

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lift dependencies to the workspace (Part 1) #2070

Lift dependencies to the workspace (Part 1) #2070

ggwpez commented Oct 27, 2023 •

edited

Loading

bkchr left a comment

bkchr Oct 28, 2023

ggwpez Oct 28, 2023

bkchr Oct 28, 2023

ggwpez Oct 28, 2023

koute Oct 28, 2023 •

edited

Loading

koute Oct 28, 2023

ggwpez Oct 28, 2023

bkchr Oct 30, 2023

jsdw Oct 30, 2023 •

edited

Loading

bkchr Oct 30, 2023

ggwpez Nov 6, 2023 •

edited

Loading

bkchr Nov 9, 2023

ggwpez commented Oct 28, 2023 •

edited

Loading

bkchr commented Oct 28, 2023

ggwpez commented Feb 2, 2024

bkchr commented Feb 5, 2024

ggwpez commented Feb 9, 2024

koute left a comment

ggwpez commented Feb 12, 2024

koute commented Feb 12, 2024 •

edited

Loading

ggwpez commented Feb 12, 2024 •

edited

Loading

ggwpez commented Feb 16, 2024

	log = { version = "^0.4.20", default-features = false }
	log = "0.4.20"

	log = { workspace = true, default-features = true }
	log = { workspace = true }

Lift dependencies to the workspace (Part 1) #2070

Lift dependencies to the workspace (Part 1) #2070

Conversation

ggwpez commented Oct 27, 2023 • edited Loading

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koute Oct 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggwpez Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggwpez commented Oct 28, 2023 • edited Loading

bkchr commented Oct 28, 2023

ggwpez commented Feb 2, 2024

bkchr commented Feb 5, 2024

ggwpez commented Feb 9, 2024

koute left a comment

Choose a reason for hiding this comment

ggwpez commented Feb 12, 2024

koute commented Feb 12, 2024 • edited Loading

ggwpez commented Feb 12, 2024 • edited Loading

ggwpez commented Feb 16, 2024

ggwpez commented Oct 27, 2023 •

edited

Loading

koute Oct 28, 2023 •

edited

Loading

jsdw Oct 30, 2023 •

edited

Loading

ggwpez Nov 6, 2023 •

edited

Loading

ggwpez commented Oct 28, 2023 •

edited

Loading

koute commented Feb 12, 2024 •

edited

Loading

ggwpez commented Feb 12, 2024 •

edited

Loading