Switch to workspace deps - batch 1

[jasl]

Split to separated commits, please focus on default-features

[jasl]

cc @ggwpez

[ggwpez]

Is it reproducible with some command?

[jasl]

Is it reproducible with some command?

Sorry, no, I don't write a script to do this.

My way is:

• Pick one crate I will handle, for example: rand
• Use the keyword rand = to do a full-text search on *.toml; most of these lines are targets that I need to rewrite
• Manual identify patterns, tidy lines into a few patterns
  • Align version first, for example, rand = "0.8.5", rand = 0.8.3, and rand = "0.8" are going to rand = "0.8"
  • Resort keys, for example, rand = { optional = true, version = "0.8.5" }, rand = { default-features = false, version = "0.8.5" }, move the version key to the first
• Plain text replace rand = "0.8.5" to rand = { workspace = true }, rand = { version = "0.8.5" to rand = { workspace = true
• Be careful handling default-features flag

I use JetBrains IDE, it has some useful tools can help me.
The process is boring, but I can do it efficiently.

I may miss some crates, but those moved should be OK. The CI also helps to check feature gates.

[jasl]

To check default-features, There's a simple way

There may have exception when optional = true, but generally this is true

[jasl]

I was thinking of writing a tool, but parsing TOML and doing in-place replacement is difficult, so I gave up.
My way isn't funny, but I refactored over 40 crates in ~3 hours, so feels fine.

[ggwpez]

These things should be automated to avoid human error. I did both of these MRs as preparation to see if the tool reliably works, and it seems to: #2070 & #3366

IIUC what you did manually is:
zepter transpose dependency lift-to-workspace tokio-tungstenite jsonrpsee tokio rand_distr rand_pcg rand_core rand_chacha rand bitflags nix bs58 tracing tracing-log itertools aquamarine prost array-bytes --fix --version-resolver=highest --source-location=remote

It would be good to have a way to reproduce that is reviewable, otherwise i dont want to check the diff every time that we pull up some dependencies. When there is an automated way, then only the automated way needs to be reviewed and not the whole diff.

[ggwpez]

Now imagine we want to do the same thing for all other dependencies, then for the runtimes repo and for all other repos in the Polkadot Ecosystem.
IMHO it makes sense to think in automation to amplify our impact.

[jasl]

These things should be automated to avoid human error. I did both of these MRs as preparation to see if the tool reliably works, and it seems to: #2070 & #3366

IIUC what you did manually is: zepter transpose dependency lift-to-workspace tokio-tungstenite jsonrpsee tokio rand_distr rand_pcg rand_core rand_chacha rand bitflags nix bs58 tracing tracing-log itertools aquamarine prost array-bytes --fix --version-resolver=highest --source-location=remote

It would be good to have a way to reproduce that is reviewable, otherwise i dont want to check the diff every time that we pull up some dependencies. When there is an automated way, then only the automated way needs to be reviewed and not the whole diff.

But I believe you need to do cross-checking even you have a tool?

Besides, if the tool is reliable, why don't you just flush all crates? why need to review? CI can ensure the correctness

[jasl]

Now imagine we want to do the same thing for all other dependencies, then for the runtimes repo and for all other repos in the Polkadot Ecosystem. IMHO it makes sense to think in automation to amplify our impact.

I believe this is a one-time job, once we move to workspace dependencies, contributors should follow the rule.
So I don't think we need to fight this in long-term.

[jasl]

The only diff with your tool is I prefer explicit declare default-features = false, if you don't like it, this can change

[liamaharon]

@jasl @ggwpez you can just use cargo dependency-inheritor --workspace-path "Cargo.toml" -n 1, it automates the process and works for any cargo project. Then we just review it once and we're done.

[jasl]

@jasl @ggwpez you can just use cargo dependency-inheritor --workspace-path "Cargo.toml" -n 1, it automates the process and works for any cargo project. Then we just review it once and we're done.

TBH, this is a boring job, I volunteer to do it, unfortunately, reviewers have to bear the boring, too.

I don't get the point of why it needs to be automatic.
If there's a tool that can help, it can only help me, the volunteer.

But you, the reviewer, have to use another way to cross-check my work,
because the tool may generate incorrect results.

If there's a reliable tool and we use it, the reviewers reproduce my work on their side still make no sense to me.
Use the same tool to get the same result proves nothing (except git add --all bugged or I mix bad things)

For cargo dependency-inheritor --workspace-path "Cargo.toml" -n 1

I tried it, the result is 506 files changed, 7331 insertions(+), 6587 deletions(-)
If you believe this is reviewable, I can push

[liamaharon]

I tried it, the result is 506 files changed, 7331 insertions(+), 6587 deletions(-)
If you believe this is reviewable, I can push

I think a manual skim of all changes (to ensure nothing unexpected) + the CI passing means it's fine, our CI is quite thorough.

Personally I'd rather review one large PR with all the workspace changes and get it over with, instead of reviewing multiple of the exact same type of PR throughout the week. but I'll defer to @ggwpez if he has another pref.

[ggwpez]

Dunno, i already gave my opinion 😆. cc @bkchr

[bkchr]

Yeah let's rip off the band-aid once ;)

If the script @liamaharon mentions works, let's use it.

[ggwpez]

Besides, if the tool is reliable, why don't you just flush all crates? why need to review? CI can ensure the correctness

Yes true. I like to test things one-by-one, so first doing 10 dependencies - seeing if something downstream breaks - then the next batch.

Anyway, the dependency inheritor seems to produce incorrect workspace config:

error: failed to select a version for the requirement `sc-network-types = "*"`
candidate versions found which didn't match: 0.10.0-dev
location searched: /Users/vados/Documents/work/polkadot-sdk/substrate/client/network/types
required by package `sc-service v0.35.0 (/Users/vados/Documents/work/polkadot-sdk/substrate/client/service)`
if you are looking for the prerelease package it needs to be specified explicitly
    sc-network-types = { version = "0.10.0-dev" }

You can use Zepter to at least pull all external deps. It produces valid config, does not double-add default-features = false (like the inheritor does) and the code still compiles.

Both approaches update some deps in the process (you can check first with --version-resolver=unambiguous):
zepter transpose dependency lift-to-workspace "regex:.*" --version-resolver=highest --fix --source-location=remote --ignore-errors
But it does not support internal deps yet (the step where the other tool seems to fail anyway).

[jasl]

does not double-add default-features = false (like the inheritor does) and the code still compiles.

Explicit default-features are my personal preference. it can easily change if you don't like it.

[jasl]

We can discuss the coding style, and I will follow your suggestions.

PS: I duplicated default-features = false because you must jump to the root Cargo.toml to check it, or you don't know if the workspace dependency has the attribute.
IMO, this could lead to potential mistakes.

My point is that when reviewing it, we need to cross-check. That said, reviewers should use another method to ensure its correctness.

So, you'll be able to use tools to check my hand-made work quickly.