Skip to content

[pallet-revive] fixture as dev dep#7844

Merged
pgherveou merged 19 commits intomasterfrom
pg/fixture-as-dev
Mar 13, 2025
Merged

[pallet-revive] fixture as dev dep#7844
pgherveou merged 19 commits intomasterfrom
pg/fixture-as-dev

Conversation

@pgherveou
Copy link
Copy Markdown
Contributor

@pgherveou pgherveou commented Mar 7, 2025

Update pallet-revive-fixtures so that it can build without looking up dependencies from the workspace

@pgherveou
Copy link
Copy Markdown
Contributor Author

/cmd prdoc --audience runtime_dev --bump patch

@pgherveou pgherveou added T7-smart_contracts This PR/Issue is related to smart contracts. R0-no-crate-publish-required The change does not require any crates to be re-published. labels Mar 7, 2025
Copy link
Copy Markdown
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

I see it twice in pallet-revive; once in normal and once in dev-dependencies.
Can it maybe also just be dev in that case?

@pgherveou
Copy link
Copy Markdown
Contributor Author

pgherveou commented Mar 7, 2025

I see it twice in pallet-revive; once in normal and once in dev-dependencies. Can it maybe also just be dev in that case?

👀

@pgherveou
Copy link
Copy Markdown
Contributor Author

I see it twice in pallet-revive; once in normal and once in dev-dependencies. Can it maybe also just be dev in that case?

@athei we need it as a dep in pallet-revive? Is that for benchmark?

@athei
Copy link
Copy Markdown
Member

athei commented Mar 10, 2025

I see it twice in pallet-revive; once in normal and once in dev-dependencies. Can it maybe also just be dev in that case?

@athei we need it as a dep in pallet-revive? Is that for benchmark?

Yeah the dummy and noop benchmark fixtures are being used in benchmarks. If you want to get rid of them you could re-write them in assembly. However, the assembler doesn't support import tables. But you could use a hardcoded syscall number as I did for the code size benchmark.

@pgherveou pgherveou requested review from a team as code owners March 10, 2025 11:34
@pgherveou
Copy link
Copy Markdown
Contributor Author

/cmd prdoc --audience runtime_dev --bump patch

@pgherveou
Copy link
Copy Markdown
Contributor Author

/cmd prdoc --audience runtime_dev --bump patch

@github-actions
Copy link
Copy Markdown
Contributor

Command "prdoc --audience runtime_dev --bump patch" has failed ❌! See logs here

Comment on lines +91 to +96
if uapi_pkg.source.is_none() {
uapi_dep.insert(
"path".to_string(),
toml::Value::String(
fixtures_dir.join("../uapi").canonicalize()?.to_str().unwrap().to_string(),
),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How is this supposed to work? Is it set depending on whether we are in a workspace or not?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah, would need to test it out, to make sure it works with the release deployment pipeline

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Aye if this works as expected its a good solution.

@pgherveou pgherveou enabled auto-merge March 11, 2025 11:59
@pgherveou pgherveou added this pull request to the merge queue Mar 13, 2025
@pgherveou pgherveou removed this pull request from the merge queue due to a manual request Mar 13, 2025
@pgherveou pgherveou enabled auto-merge March 13, 2025 12:11
@paritytech-workflow-stopper
Copy link
Copy Markdown

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/13834260283
Failed job name: check-try-runtime

@pgherveou pgherveou added this pull request to the merge queue Mar 13, 2025
auto-merge was automatically disabled March 13, 2025 14:31

Pull Request is not mergeable

Merged via the queue into master with commit 8265c65 Mar 13, 2025
237 of 243 checks passed
@pgherveou pgherveou deleted the pg/fixture-as-dev branch March 13, 2025 14:44
github-merge-queue bot pushed a commit that referenced this pull request Mar 14, 2025
Fix pallet-revive-uapi resolution when building pallet-revive-fixtures
contracts
follow up from #7844

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: xermicus <[email protected]>
EgorPopelyaev pushed a commit that referenced this pull request Mar 17, 2025
backport revive PRs:
- #7928 [pallet-revive] Fix pallet-revive-fixtures build.rs 
- #7879 [pallet-revive] Support blocktag in eth_getLogs RPC
- #7848 [pallet-revive] Add support for eip1898 block notation
- #7844 [pallet-revive] fixture as dev dep
- #7827 [revive-rpc] allow using legacy data field
- #7810 [pallet-revive] precompiles 2->9

- **bump asset-hub-westend spec version**

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
ordian added a commit that referenced this pull request Mar 17, 2025
* origin: (325 commits)
  Add an extra_constant to pallet-treasury (#7918)
  Bump the ci_dependencies group across 1 directory with 4 updates (#7855)
  remove compromised action (#7934)
  Fixing token-economics dead link (#5302)
  [pallet-revive] Fix pallet-revive-fixtures build.rs (#7928)
  cumulus: fix pov exporter format (#7923)
  sp-api: Support `mut` in `impl_runtime_apis!` (#7924)
  Remove clones from block seal function (#7917)
  [pallet-revive] precompiles 2->9 (#7810)
  Use non-native token to benchmark xcm on asset hub (#7893)
  [CI] bump timeout wait for build in zombienet workflows. (#7871)
  taplo: split long array line to multiline array (#7905)
  [pallet-revive] fixture as dev dep (#7844)
  notifications/libp2p: Punish notification protocol misbehavior on outbound substreams (#7781)
  [Release|CI/CD] Update version of the cache action in the Publish docker ci (#7892)
  Remove `pallet::getter` usage from bridges/modules (#7120)
  [pallet-revive] Support blocktag in eth_getLogs RPC (#7879)
  Improve error message in benchmark macro (#7873)
  staking: add `manual_slash` extrinsic (#7805)
  Remove execute_with_origin implementation in the XCM executor (#7889)
  ...
EgorPopelyaev pushed a commit that referenced this pull request Mar 26, 2025
# Description

When building `polkadot-sdk-parachain-template` workspace (`cargo build
--workspace --all-features`), based on `stable2412` dependencies
(paritytech/polkadot-sdk-parachain-template#26),
it fails with an error like below.
<details>
  <summary><b>error message</b></summary>
error[E0080]: evaluation of constant value failed
-->
/home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/sc-network-0.47.0/src/protocol/message.rs:79:40
   |
79 |     #[derive(Debug, PartialEq, Eq, Clone, Encode, Decode)]
| ^^^^^^ the evaluated program panicked at 'Found variants that have
duplicate indexes. Both `Consensus` and `RemoteCallResponse` have the
index `6`. Use different indexes for each variant.',
/home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/sc-network-0.47.0/src/protocol/message.rs:79:43
   |
= note: this error originates in the macro `$crate::panic::panic_2021`
which comes from the expansion of the macro `::core::panic` (in Nightly
builds, run with -Z macro-backtrace for more info)

error[E0080]: evaluation of constant value failed
-->
/home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/sc-network-0.47.0/src/protocol/message.rs:79:48
   |
79 |     #[derive(Debug, PartialEq, Eq, Clone, Encode, Decode)]
| ^^^^^^ the evaluated program panicked at 'Found variants that have
duplicate indexes. Both `Consensus` and `RemoteCallResponse` have the
index `6`. Use different indexes for each variant.',
/home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/sc-network-0.47.0/src/protocol/message.rs:79:51
   |
= note: this error originates in the macro `$crate::panic::panic_2021`
which comes from the expansion of the macro `::core::panic` (in Nightly
builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0080`.
error: could not compile `sc-network` (lib) due to 2 previous errors
</details>

The root cause is `sc-network 0.47.0` (which builds fine when >=0.48.0),
a dependency of `pallet-revive-eth-rpc 0.2.0`, which is a dependency of
`polkadot-sdk 0.12.1`, used for various things in
`polkadot-sdk-parachain-template` repo. While investigating how to fix
the issue on `stable2412`, I discovered a few things:

1. `pallet-revive 0.3.1` can not build with `runtime-benchmarks` based
on latest `stable2412`, and it is also a dependency of
`pallet-revive-eth-rpc 0.2.0`, and pulled by `polkadot-sdk 0.12.1` for
its `runtime-benchmarks` feature.

2. Based on some chats started here:
#7844 (comment),
we identified that `pallet-revive/pallet-revive-eth-rpc` were set with
`publish = false` for a good reason and it shouldn't have changed here:
31b5923.
We also identified as a potential fix what this PR does, meaning
removing the `pallet-revive/pallet-revive-eth-rpc` dependency of
`polkadot-sdk`. I removed the other dependencies as well since I feel it
does not make sense to still keep them once `pallet-revive` is out.

3. Releasing correct crates for `pallet-revive*` and use them in
`polkadot-sdk` isn't yet necessary IMO, since some things still need to
be polished, and given their builds fail when building certain features,
they are not usable by users fully, so maybe not worth keeping partial
working functionality around just for the sake of having the crates in
`polkadot-sdk` - please challenge this if you think differently.
Ideally, we will make these pallets available for developers to use via
`polkadot-sdk` when all builds correctly with all features.

## Integration

Runtime/Node devs will not be able to reference `pallet-revive*` pallets
anymore via `polkadot-sdk`, so this is a breaking change, but if their
usage is close to zero by polkadot devs we can tune it down and consider
it a minor change for `polkadot-sdk`.

## Review Notes

1. If we think this requires a major bump for `polkadot-sdk`.. then we
can't look at the PR as a fix for `stable2412` and associated published
`polkadot-sdk` version. If we consider `pallet-revive` logic not being
necessarily used by polkadot devs so not very breaking from usage
perspective, then we can go with a minor bump and finally have a working
`polkadot-sdk` for `stable2412.

2. We also considered fixing `pallet-revive` & `pallet-revive-eth-rpc`
on `stable2412`, and publish correct versions on `crates.io`, and then
follow with a `polkadot-sdk` publish that depends on them, and in the
end `polkadot-sdk-parachain-template` workspace would build successfully
based on `stable2412` crates, but this is more work which could've been
done by now if it was necessary - e.g. in a scenario where actual
developers were using them (instead of setting the crates with `publish
= false`), so I think it makes sense to remove them for now from
`polkadot-sdk`.

3. ~IMO this PR should be backported to stable2503 too. Releasing
`polkadot-sdk` with `pallet-revive*` dependencies can be done with the
June release if things are more stable by then.~ We actually want to fix
`pallet-revive*` crates for stable2503, so must be kept in master and
2503 branch.

---------

Signed-off-by: Iulian Barbu <[email protected]>
Co-authored-by: Alexander Theißen <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
alindima pushed a commit that referenced this pull request Apr 8, 2025
Update pallet-revive-fixtures so that it can build without looking up
dependencies from the workspace

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
alindima pushed a commit that referenced this pull request Apr 8, 2025
Fix pallet-revive-uapi resolution when building pallet-revive-fixtures
contracts
follow up from #7844

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: xermicus <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

R0-no-crate-publish-required The change does not require any crates to be re-published. T7-smart_contracts This PR/Issue is related to smart contracts.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants