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

Improve features dev-ex #1831

Merged
merged 15 commits into from
Oct 24, 2023
Merged

Improve features dev-ex #1831

merged 15 commits into from
Oct 24, 2023

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Oct 9, 2023

Adds a config file that allows to run zepter without any arguments in the workspace to address all issues.
A secondary workflow for the CI is provided as zepter run check. Both the formatting and linting are now in one check for efficiancy.

The latest version also detects some more things that featalign was already showing.

Error message in the CI now looks like this:

...
crate 'test-parachains' (/Users/vados/Documents/work/polkadot-sdk/polkadot/parachain/test-parachains/Cargo.toml)
  feature 'std'
    must propagate to:
      parity-scale-codec
Found 55 issues (run with --fix to fix).
Error: Command 'lint propagate-feature' failed with exit code 1

Polkadot-SDK uses the Zepter CLI to detect abnormalities in the feature configuration.
It looks like one more more checks failed; please check the console output. You can try to automatically address them by running `zepter`.
Otherwise please ask directly in the Merge Request, GitHub Discussions or on Matrix Chat, thank you.

For more information, see:
  - https://github.com/paritytech/polkadot-sdk/issues/1831
  - https://github.com/ggwpez/zepter

TODO:

  •  Check that CI fails correctly

ggwpez added 4 commits October 9, 2023 21:48
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez added the R0-silent Changes should not be mentioned in any release notes label Oct 9, 2023
@ggwpez ggwpez marked this pull request as ready for review October 9, 2023 20:06
@ggwpez ggwpez requested review from a team October 9, 2023 20:06
@ggwpez ggwpez requested a review from a team October 9, 2023 20:06
@ggwpez ggwpez requested review from koute and a team as code owners October 9, 2023 20:06
@paritytech-ci paritytech-ci requested review from a team October 9, 2023 20:08
@bkontur
Copy link
Contributor

bkontur commented Oct 9, 2023

Maybe unrelated question, but should dev-dependencies be added to std / try-runtime / runtime-benchmarks features?

E.g. here https://github.com/paritytech/polkadot-sdk/pull/1801/files#diff-57e966c3ef58bb810065f964f9d2d0c5ef141de756082abd9b0f877de98f78c3R25-L34
pallet-balances = { path = "../balances" } and it was unnecessary added to std as pallet-balances/std.
But on the other hand, pallet-balances has to be added to try-runtime as pallet-balances/try-runtime, is that ok?

@ggwpez
Copy link
Member Author

ggwpez commented Oct 10, 2023

Maybe unrelated question, but should dev-dependencies be added to std / try-runtime / runtime-benchmarks features?

I am not really sure how to handle dev and build dependencies. Normally we dont disable the default features for dev dependencies, so we dont need to add them to std. But for runtime-benchmarks and try-runtime it would make sense.

@paritytech-ci paritytech-ci requested a review from a team October 12, 2023 23:35
Copy link
Contributor

@liamaharon liamaharon left a comment

Choose a reason for hiding this comment

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

🎉

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Seems like a better spot than the .cargo folder.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Otherwise it wont find the config file.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez enabled auto-merge (squash) October 15, 2023 17:29
@ggwpez
Copy link
Member Author

ggwpez commented Oct 16, 2023

IIUC #1870 moved some deps upstream. Now the problem is that the introduced dependency sp-bls12-381 pulls in sp-crypto-ec-utils from the SDK, which in term wants sp-std v11.
Since we did not backport the version bumps, the repo has version 8 and now pulls that stuff in twice - resulting in a build error here in the CI.

@bkchr
Copy link
Member

bkchr commented Oct 16, 2023

Does cargo unify deps which have a path and a version? So, would that be really fixed by merging the version bumps back to master?

@ggwpez ggwpez mentioned this pull request Oct 16, 2023
bkchr pushed a commit that referenced this pull request Oct 16, 2023
Changes:
- Add missing crate to the workspace
- Remove versions from local dependency links

Maybe it is finally worth it to add this scrip to the CI to find these
things earlier:
[check-deps.py](https://github.com/ggwpez/substrate-scripts/blob/master/import-runtime-repos/check-deps.py).

@paritytech/ci what would be the best location for that check?  
It takes only a second to run, so maybe we can squeeze it into one of
the existing checks?
Otherwise creating a new GH workflow feels a bit wasteful... maybe i can
group it with #1831

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Copy link
Member Author

ggwpez commented Oct 17, 2023

Does cargo unify deps which have a path and a version? So, would that be really fixed by merging the version bumps back to master?

It does not seem to quite fix it, since we use one GH dependency here which then pulls in duplicate versions again.
I locally removed that dep and it worked, so hopefully it would be fixed if the bandersnatch also uses all the latest versions and gets published.

tdimitrov pushed a commit that referenced this pull request Oct 23, 2023
Changes:
- Add missing crate to the workspace
- Remove versions from local dependency links

Maybe it is finally worth it to add this scrip to the CI to find these
things earlier:
[check-deps.py](https://github.com/ggwpez/substrate-scripts/blob/master/import-runtime-repos/check-deps.py).

@paritytech/ci what would be the best location for that check?  
It takes only a second to run, so maybe we can squeeze it into one of
the existing checks?
Otherwise creating a new GH workflow feels a bit wasteful... maybe i can
group it with #1831

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez merged commit 4a44356 into master Oct 24, 2023
10 checks passed
@ggwpez ggwpez deleted the oty-zepter-devex branch October 24, 2023 15:59
@ggwpez ggwpez mentioned this pull request Oct 24, 2023
ordian added a commit that referenced this pull request Oct 26, 2023
* master:
  Removed TODO from test-case for hard-coded delivery fee estimation (#2042)
  Expose collection attributes from `Inspect` trait (#1914)
  `polkadot-parachain-primitives` should not depend on `frame-support`. (#1897)
  [testnet] Align testnet system parachain runtimes using `RelayTreasuryLocation` and `SystemParachains` in the same way (#2023)
  Sort the benchmarks before listing them (#2026)
  publish pallet-root-testing (#2017)
  Contracts: Add benchmarks to include files (#2022)
  Small optimisation to `--profile dev` wasm builds (#1851)
  basic-authorship: Improve time recording and logging (#2010)
  Application Crypto and BEEFY Support for paired (ECDSA,BLS) crypto (#1815)
  [ci] Run check-rust-feature-propagation in pr and master (#2012)
  Improve features dev-ex (#1831)
  Remove obsolete comment. (#2008)
ordian added a commit that referenced this pull request Oct 26, 2023
* tsv-disabling:
  Removed TODO from test-case for hard-coded delivery fee estimation (#2042)
  Expose collection attributes from `Inspect` trait (#1914)
  `polkadot-parachain-primitives` should not depend on `frame-support`. (#1897)
  [testnet] Align testnet system parachain runtimes using `RelayTreasuryLocation` and `SystemParachains` in the same way (#2023)
  Sort the benchmarks before listing them (#2026)
  publish pallet-root-testing (#2017)
  Contracts: Add benchmarks to include files (#2022)
  Small optimisation to `--profile dev` wasm builds (#1851)
  basic-authorship: Improve time recording and logging (#2010)
  Application Crypto and BEEFY Support for paired (ECDSA,BLS) crypto (#1815)
  [ci] Run check-rust-feature-propagation in pr and master (#2012)
  Improve features dev-ex (#1831)
  Remove obsolete comment. (#2008)
ordian added a commit that referenced this pull request Oct 26, 2023
* tsv-disabling: (36 commits)
  Removed TODO from test-case for hard-coded delivery fee estimation (#2042)
  Expose collection attributes from `Inspect` trait (#1914)
  `polkadot-parachain-primitives` should not depend on `frame-support`. (#1897)
  [testnet] Align testnet system parachain runtimes using `RelayTreasuryLocation` and `SystemParachains` in the same way (#2023)
  Sort the benchmarks before listing them (#2026)
  publish pallet-root-testing (#2017)
  Contracts: Add benchmarks to include files (#2022)
  Small optimisation to `--profile dev` wasm builds (#1851)
  basic-authorship: Improve time recording and logging (#2010)
  Application Crypto and BEEFY Support for paired (ECDSA,BLS) crypto (#1815)
  [ci] Run check-rust-feature-propagation in pr and master (#2012)
  Improve features dev-ex (#1831)
  Remove obsolete comment. (#2008)
  Refactor candidates test in paras_inherent (#2004)
  PVF: Add worker check during tests and benches (#1771)
  Bump actions/setup-node from 3.8.1 to 4.0.0 (#1997)
  polkadot: enable tikv-jemallocator/unprefixed_malloc_on_supported_platforms (#2002)
  Make `IdentityInfo` generic in `pallet-identity` (#1661)
  Ensure correct variant count in `Runtime[Hold/Freeze]Reason` (#1900)
  `CheckWeight`: Add more logging (#1996)
  ...
s0me0ne-unkn0wn pushed a commit that referenced this pull request Oct 29, 2023
Adds a config file that allows to run `zepter` without any arguments in
the workspace to address all issues.
A secondary workflow for the CI is provided as `zepter run check`. Both
the formatting and linting are now in one check for efficiancy.

The latest version also detects some more things that `featalign` was
already showing.

Error message [in the
CI](https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3916205)
now looks like this:
```pre
...
crate 'test-parachains' (/Users/vados/Documents/work/polkadot-sdk/polkadot/parachain/test-parachains/Cargo.toml)
  feature 'std'
    must propagate to:
      parity-scale-codec
Found 55 issues (run with --fix to fix).
Error: Command 'lint propagate-feature' failed with exit code 1

Polkadot-SDK uses the Zepter CLI to detect abnormalities in the feature configuration.
It looks like one more more checks failed; please check the console output. You can try to automatically address them by running `zepter`.
Otherwise please ask directly in the Merge Request, GitHub Discussions or on Matrix Chat, thank you.

For more information, see:
  - #1831
  - https://github.com/ggwpez/zepter
```

TODO:
- [x] Check that CI fails correctly

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
franciscoaguirre pushed a commit to paritytech/xcm that referenced this pull request Jan 25, 2024
Adds a config file that allows to run `zepter` without any arguments in
the workspace to address all issues.
A secondary workflow for the CI is provided as `zepter run check`. Both
the formatting and linting are now in one check for efficiancy.

The latest version also detects some more things that `featalign` was
already showing.

Error message [in the
CI](https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3916205)
now looks like this:
```pre
...
crate 'test-parachains' (/Users/vados/Documents/work/polkadot-sdk/polkadot/parachain/test-parachains/Cargo.toml)
  feature 'std'
    must propagate to:
      parity-scale-codec
Found 55 issues (run with --fix to fix).
Error: Command 'lint propagate-feature' failed with exit code 1

Polkadot-SDK uses the Zepter CLI to detect abnormalities in the feature configuration.
It looks like one more more checks failed; please check the console output. You can try to automatically address them by running `zepter`.
Otherwise please ask directly in the Merge Request, GitHub Discussions or on Matrix Chat, thank you.

For more information, see:
  - paritytech/polkadot-sdk#1831
  - https://github.com/ggwpez/zepter
```

TODO:
- [x] Check that CI fails correctly

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
Changes:
- Add missing crate to the workspace
- Remove versions from local dependency links

Maybe it is finally worth it to add this scrip to the CI to find these
things earlier:
[check-deps.py](https://github.com/ggwpez/substrate-scripts/blob/master/import-runtime-repos/check-deps.py).

@paritytech/ci what would be the best location for that check?  
It takes only a second to run, so maybe we can squeeze it into one of
the existing checks?
Otherwise creating a new GH workflow feels a bit wasteful... maybe i can
group it with paritytech#1831

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
Adds a config file that allows to run `zepter` without any arguments in
the workspace to address all issues.
A secondary workflow for the CI is provided as `zepter run check`. Both
the formatting and linting are now in one check for efficiancy.

The latest version also detects some more things that `featalign` was
already showing.

Error message [in the
CI](https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3916205)
now looks like this:
```pre
...
crate 'test-parachains' (/Users/vados/Documents/work/polkadot-sdk/polkadot/parachain/test-parachains/Cargo.toml)
  feature 'std'
    must propagate to:
      parity-scale-codec
Found 55 issues (run with --fix to fix).
Error: Command 'lint propagate-feature' failed with exit code 1

Polkadot-SDK uses the Zepter CLI to detect abnormalities in the feature configuration.
It looks like one more more checks failed; please check the console output. You can try to automatically address them by running `zepter`.
Otherwise please ask directly in the Merge Request, GitHub Discussions or on Matrix Chat, thank you.

For more information, see:
  - paritytech#1831
  - https://github.com/ggwpez/zepter
```

TODO:
- [x] Check that CI fails correctly

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
bkchr pushed a commit that referenced this pull request Apr 10, 2024
* use GitLab env vars to get git commit

* compile_error to test it

* Revert "compile_error to test it"

This reverts commit 67d4782298d3cdfbe1a28231042bba6444316e8a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants