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

Unify dependency aliases #4633

Merged
merged 23 commits into from
Jun 5, 2024
Merged

Unify dependency aliases #4633

merged 23 commits into from
Jun 5, 2024

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented May 29, 2024

Inherited workspace dependencies cannot be renamed by the crate using them (see 1, 2).
Since we want to use inherited workspace dependencies everywhere, we first need to unify all aliases that we use for a dependency throughout the workspace.
The umbrella crate is currently excluded from this procedure, since it should be able to export the crates by their original name without much hassle.

For example: one crate may alias parity-scale-codec to codec, while another crate does not alias it at all. After this change, all crates have to use codec as name. The problematic combinations were:

  • conflicting aliases: most crates aliases as A but some use B.
  • missing alias: most of the crates alias a dep but some dont.
  • superfluous alias: most crates dont alias a dep but some do.

The script that i used first determines whether most crates opted to alias a dependency or not. From that info it decides whether to use an alias or not. If it decided to use an alias, the most common one is used everywhere.

To reproduce, i used this python script in combination with this error output from Zepter.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez requested review from athei, andresilva and a team as code owners May 29, 2024 14:58
@paritytech-review-bot paritytech-review-bot bot requested a review from a team May 29, 2024 14:58
@ggwpez ggwpez marked this pull request as draft May 29, 2024 15:01
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez added R0-silent Changes should not be mentioned in any release notes and removed R0-silent Changes should not be mentioned in any release notes labels May 29, 2024
@@ -35,7 +35,7 @@ use sp_runtime::{
};

/// The log target to be used by client code.
pub const CLIENT_LOG_TARGET: &str = "grandpa";
pub const CLIENT_LOG_TARGET: &str = "finality_grandpa";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub const CLIENT_LOG_TARGET: &str = "finality_grandpa";
pub const CLIENT_LOG_TARGET: &str = "grandpa";

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yea it did a string replace. I also replaced the string with quotes, since there are feature gates that we call the same as the dependency.
But i can run it again and print all the cases where it replaced an exact quoted string match.

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked the diff with git diff -U0 $(git merge-base HEAD^ origin/master) -- ':!**/*.toml' | egrep '"(\w|_|-)+"' and it seems like only a few cases where the string replace messed up.

Check with:
git diff -U0 $(git merge-base HEAD^ origin/master) -- ':!**/*.toml' | egrep '"(\w|_|-)+"'

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Bastian Köcher <git@kchr.de>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez marked this pull request as ready for review May 31, 2024 16:55
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez added the T8-polkadot This PR/Issue is related to/affects the Polkadot network. label Jun 3, 2024
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
polkadot/node/subsystem-bench/Cargo.toml Outdated Show resolved Hide resolved
prdoc/pr_4633.prdoc Outdated Show resolved Hide resolved
ggwpez and others added 2 commits June 4, 2024 23:59
Co-authored-by: Bastian Köcher <git@kchr.de>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

Cool script 🚀

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez enabled auto-merge June 5, 2024 12:15
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6405457

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez added this pull request to the merge queue Jun 5, 2024
Merged via the queue into master with commit d2fd536 Jun 5, 2024
156 checks passed
@ggwpez ggwpez deleted the oty-uniform-dep-alias branch June 5, 2024 15:07
ordian added a commit that referenced this pull request Jun 13, 2024
* master: (29 commits)
  Append overlay optimization. (#1223)
  finalization: Skip tree route calculation if no forks present (#4721)
  Remove unncessary call remove_from_peers_set (#4742)
  add pov-recovery unit tests and support for elastic scaling (#4733)
  approval-voting: Add no shows debug information (#4726)
  Revamp the Readme of the parachain template (#4713)
  Update README.md to move the PSVM link under a "Tooling" section under the "Releases" section (#4734)
  frame/proc-macro: Refactor code for better readability (#4712)
  Contracts:  update wasmi to 0.32 (#3679)
  Backport style changes from P<>K bridge to R<>W bridge (#4732)
  New reference doc for Custom RPC V2 (#4654)
  Frame Pallets: Clean a lot of test setups (#4642)
  Fix occupied core handling (#4691)
  statement-distribution: Fix false warning (#4727)
  Update the README to include a link to the Polkadot SDK Version Manager (#4718)
  Cleanup PVF artifact by cache limit and stale time (#4662)
  Update link to a latest polkadot release (#4711)
  [CI] Delete cargo-deny config (#4677)
  fix build on MacOS: bump secp256k1 and secp256k1-sys to patched versions (#4709)
  Unify dependency aliases (#4633)
  ...
github-merge-queue bot pushed a commit that referenced this pull request Jun 24, 2024
After preparing in #4633,
we can lift also all internal dependencies up to the workspace.

This does not actually change anything, but uses `workspace = true` for
all dependencies. You can check it with:
```bash
git checkout -q $(git merge-base oty-lift-all-deps origin/master)
cargo tree -e features > master.out

git checkout -q oty-lift-all-deps
cargo tree -e features > new.out
diff master.out new.out
```

It did not yet lift 100% of dependencies, some inside of `target.*` or
some that had conflicting aliases introduced recently. But i will do
these together in a follow-up with CI checks.

Can be reproduced with [zepter](https://github.com/ggwpez/zepter/):
`zepter transpose d lift-to-workspace "regex:.*" --version-resolver
highest --skip-package "polkadot-sdk" --ignore-errors --fix`.

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
Inherited workspace dependencies cannot be renamed by the crate using
them (see [1](rust-lang/cargo#12546),
[2](https://stackoverflow.com/questions/76792343/can-inherited-dependencies-in-rust-be-aliased-in-the-cargo-toml-file)).
Since we want to use inherited workspace dependencies everywhere, we
first need to unify all aliases that we use for a dependency throughout
the workspace.
The umbrella crate is currently excluded from this procedure, since it
should be able to export the crates by their original name without much
hassle.

For example: one crate may alias `parity-scale-codec` to `codec`, while
another crate does not alias it at all. After this change, all crates
have to use `codec` as name. The problematic combinations were:
- conflicting aliases: most crates aliases as `A` but some use `B`.
- missing alias: most of the crates alias a dep but some dont.
- superfluous alias: most crates dont alias a dep but some do.

The script that i used first determines whether most crates opted to
alias a dependency or not. From that info it decides whether to use an
alias or not. If it decided to use an alias, the most common one is used
everywhere.

To reproduce, i used
[this](https://github.com/ggwpez/substrate-scripts/blob/master/uniform-crate-alias.py)
python script in combination with
[this](https://github.com/ggwpez/zepter/blob/38ad10585fe98a5a86c1d2369738bc763a77057b/renames.json)
error output from Zepter.

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Bastian Köcher <git@kchr.de>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
After preparing in paritytech#4633,
we can lift also all internal dependencies up to the workspace.

This does not actually change anything, but uses `workspace = true` for
all dependencies. You can check it with:
```bash
git checkout -q $(git merge-base oty-lift-all-deps origin/master)
cargo tree -e features > master.out

git checkout -q oty-lift-all-deps
cargo tree -e features > new.out
diff master.out new.out
```

It did not yet lift 100% of dependencies, some inside of `target.*` or
some that had conflicting aliases introduced recently. But i will do
these together in a follow-up with CI checks.

Can be reproduced with [zepter](https://github.com/ggwpez/zepter/):
`zepter transpose d lift-to-workspace "regex:.*" --version-resolver
highest --skip-package "polkadot-sdk" --ignore-errors --fix`.

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
github-merge-queue bot pushed a commit that referenced this pull request Aug 29, 2024
closes #849

## Context

For the background on this and the long-term fix, see
#849 (comment).

## Changes

* The weigh files are renamed from `runtime_(parachains|common).*` to
`polkadot_runtime_(parachains|common).*`. The reason for it is the
renaming introduced in #4633. The new weight command and files are
generated now include `polkadot_` prefix.
* The WeightInfo for `paras_inherent` now includes `enter_empty` which
calculates the cost of processing an empty parachains inherent. This
cost is subtracted dynamically when calculating other weights (so the
other weights remain the same)

## Benefits

See
#849 (comment),
but TL;DR is that we are not blocked on weights for scaling the number
of validators and cores further.

Resolved questions:
- [x] why new benchmarks for westend are doing fewer db IOPS?
Is it due polkadot-sdk update (db IOPS diff)?
or the bench setup is no longer valid?

https://github.com/polkadot-fellows/runtimes/blob/7723274a2c5cbb10213379271094d5180716ca7d/relay/polkadot/src/weights/runtime_parachains_paras_inherent.rs#L131-L196
Answer: see background section of #5270 

TODOs:
- [x] Rerun benchmarks for Rococo and Westend
- [x] PRDoc

---------

Co-authored-by: command-bot <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T8-polkadot This PR/Issue is related to/affects the Polkadot network.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants