Restructure `frame_support` macro related exports by juangirini · Pull Request #14745 · paritytech/substrate

juangirini · 2023-08-10T10:30:33Z

Partially for paritytech/polkadot-sdk#172

juangirini · 2023-08-14T14:36:34Z

bot fmt

command-bot · 2023-08-14T14:36:38Z

@juangirini https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3379775 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 30-ab51b0e6-b330-4de7-a7f0-3557e9051f13 to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2023-08-14T14:38:08Z

@juangirini Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3379775 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3379775/artifacts/download.

…o-related-exports

juangirini · 2023-08-16T14:06:41Z

bot fmt

command-bot · 2023-08-16T14:06:45Z

@juangirini https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3401090 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 37-5bad9448-a260-4c19-941b-b85f3335db38 to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2023-08-16T14:09:05Z

@juangirini Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3401090 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3401090/artifacts/download.

ggwpez · 2023-08-16T15:55:12Z

This is going to lead to quite a bit of downstream breaking changes. I seem to recall @kianenigma was suggesting that we hide a lot of common imports under one crate ( #14137 ) - this seems like going in the opposite direction? If we're going to do both then it would be kinder for downstream if we landed both PRs at the same time as downstream would probably then have less work to update their codebases.

@gilescope it should not break downstream - unless downstream used internal re-exports, which is a crime anyway…
We now just make the fact that they are internal, more explicit.

liamaharon

Looks fine. Although I am a bit surprised to see this __private syntax. Doesn't feel very Rust-like. Why do we need to use it so much in macros?

liamaharon · 2023-08-17T04:48:53Z

+ let __benchmarked_call_encoded = $crate::frame_support::__private::codec::Encode::encode(
 &__call
 );
 }: {
 let __call_decoded = <
 Call<T $(, $instance )?>
- as $crate::frame_support::codec::Decode
+ as $crate::frame_support::__private::codec::Decode


this feels..wrong.
curious why we need to do it this way, and can't just pull Encode and Decode from their codec crate.

To make it correct, you would need to re-export codec from frame-benchmarking.

yes, benchmarking re-exports will follow in the parent issue https://github.com/paritytech/substrate/issues/14213

Because its a macro that emits code. The emitted code cannot make the assumption that codec is in the Cargo.toml of the crate that it is being used by.
We therefore need to ship a macro will all exports that it needs.

liamaharon · 2023-08-17T04:54:14Z

+ #scrate::__private::codec::Encode,
+ #scrate::__private::codec::Decode,
+ #scrate::__private::scale_info::TypeInfo,
+ #scrate::__private::RuntimeDebug,


Is there a better way? 😄

I know it looks weird, but it seems like the "less bad" solution to make it obvious that those re-exports are not supposed to be used externally

This actually isn't that bad, since macro-generated code is not part of "surface syntax". I think most people do expect code expanded this way to be sort of internal after all -- if you want to convince yourself more, try looking at the __is_call_part_defined_{num} macros that gets automatically generated and exported while we process #[pallet::call].

bkchr · 2023-08-17T07:45:21Z

+pub use codec::{
+	Codec, Decode, Encode, EncodeAsRef, EncodeLike, HasCompact, Input, MaxEncodedLen, Output,
+};
+pub use scale_info::TypeInfo;


Most of these exports here are should also be private, but yeah do this later.

bkchr · 2023-08-17T07:47:07Z

+ let __benchmarked_call_encoded = $crate::frame_support::__private::codec::Encode::encode(
 &__call
 );
 }: {
 let __call_decoded = <
 Call<T $(, $instance )?>
- as $crate::frame_support::codec::Decode
+ as $crate::frame_support::__private::codec::Decode


To make it correct, you would need to re-export codec from frame-benchmarking.

Co-authored-by: Bastian Köcher <git@kchr.de>

juangirini · 2023-08-17T10:15:35Z

bot rebase

…o-related-exports

paritytech-processbot · 2023-08-17T10:15:41Z

Rebased

juangirini · 2023-08-21T10:28:42Z

bot rebase

…o-related-exports

paritytech-processbot · 2023-08-21T10:28:48Z

Rebased

juangirini · 2023-08-22T07:44:46Z

bot rebase

…o-related-exports

paritytech-processbot · 2023-08-22T07:44:51Z

Rebased

juangirini · 2023-08-23T11:43:19Z

bot merge

paritytech-processbot · 2023-08-23T11:43:26Z

Waiting for commit status.

juangirini added 15 commits August 10, 2023 09:08

make reexports private
1dfc9b4

make reexports private 2
dccfaeb

make reexports private for runtime-benchmarking
9706353

make reexports private for try-runtime
7d6c124

fix for try-runtime
87fd6de

make reexports private for tests
1ffc743

rebase and fix conflicts
222b45d

fmt
e795557

make reexports private for tests
4c9ef13

make reexports private for experimental
4d0b806

fix beefy
e32c82f

fix ui test
4386de3

fix ui test
54657c0

fix benches
3566961

Merge branch 'master' into jg/restructure-macro-related-exports
08de067

".git/.scripts/commands/fmt/fmt.sh"
c419d4a

juangirini added 4 commits August 15, 2023 11:52

fix contracts use
b931ecb

wip
f4e5f55

wip
4ecbfff

Merge remote-tracking branch 'origin/master' into jg/restructure-macr…
b7eaa34
…o-related-exports

juangirini mentioned this pull request Aug 15, 2023
Companion: restructure macro related exports paritytech/cumulus#3015
Merged

juangirini changed the title ~~[WIP] Restructure macro related exports~~Restructure macro related exportsAug 15, 2023

juangirini mentioned this pull request Aug 15, 2023
Companion: restructure macro related exports paritytech/polkadot#7626
Merged

juangirini marked this pull request as ready for review August 15, 2023 16:20

juangirini requested review from a teamAugust 15, 2023 16:20

juangirini requested a review from athei as a code owner August 15, 2023 16:20

fix CI checks
3816fc4

juangirini requested a review from bkchr August 16, 2023 11:05

fix support tests
32f74a6

".git/.scripts/commands/fmt/fmt.sh"
7d7d73b

liamaharon approved these changes Aug 17, 2023
View reviewed changes

bkchr approved these changes Aug 17, 2023
View reviewed changes

juangiriniand others added 3 commits August 17, 2023 10:03

Update frame/support/src/lib.rs
86ddd81
Co-authored-by: Bastian Köcher <git@kchr.de>

import codec directly
5d00dfd

fmt
a6a4e3d

Merge remote-tracking branch 'origin/master' into jg/restructure-macr…
4df379c
…o-related-exports

fix node-cli tests
0fcc322

Merge remote-tracking branch 'origin/master' into jg/restructure-macr…
daf3dcb
…o-related-exports

Merge remote-tracking branch 'origin/master' into jg/restructure-macr…
e94e54b
…o-related-exports

Merge branch 'master' into jg/restructure-macro-related-exports
20a95f2

paritytech-processbotBot merged commit df383f2 into masterAug 23, 2023

paritytech-processbotBot deleted the jg/restructure-macro-related-exports branch August 23, 2023 12:30

jasl mentioned this pull request Oct 9, 2023
Upgrade to Polkadot v1.1.0 Phala-Network/phala-blockchain#1403
Merged

crystalin mentioned this pull request Oct 20, 2023
Update substrate/polkadot/cumulus from v0.9.43 to v1.1.0 moonbeam-foundation/moonbeam#2535
Closed

Uh oh!

Conversation

juangirini commented Aug 10, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juangirini commented Aug 14, 2023

Uh oh!

command-botBot commented Aug 14, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

command-botBot commented Aug 14, 2023

Uh oh!

juangirini commented Aug 16, 2023

Uh oh!

command-botBot commented Aug 16, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

command-botBot commented Aug 16, 2023

Uh oh!

ggwpez commented Aug 16, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liamaharon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juangirini commented Aug 17, 2023

Uh oh!

paritytech-processbotBot commented Aug 17, 2023

Uh oh!

juangirini commented Aug 21, 2023

Uh oh!

paritytech-processbotBot commented Aug 21, 2023

Uh oh!

juangirini commented Aug 22, 2023

Uh oh!

paritytech-processbotBot commented Aug 22, 2023

Uh oh!

juangirini commented Aug 23, 2023

Uh oh!

paritytech-processbotBot commented Aug 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

juangirini commented Aug 10, 2023•
edited
Loading

command-botBot commented Aug 14, 2023•
edited
Loading

command-botBot commented Aug 16, 2023•
edited
Loading

ggwpez commented Aug 16, 2023•
edited
Loading