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

Add unstable-api feature flag to pallet-revive #6866

Merged
merged 14 commits into from
Dec 13, 2024
Merged

Conversation

davidk-pt
Copy link
Contributor

@davidk-pt davidk-pt commented Dec 13, 2024

Follow up refactor to #6844 (review)

I still need to finish adding #[cfg(feature = "unstable-api")] to the rest of the tests and make sure all tests pass, I want to make sure I'm moving into right direction first

@athei @xermicus

@paritytech-workflow-stopper
Copy link

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

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

The feature should only be added to pallet-revive-uapi. The fixtures just unconditionally enable the feature. This is meant for other user (like ink!) so they need to opt-in to use unstable features. But for our tests we uncondtionally require them.

substrate/frame/revive/Cargo.toml Outdated Show resolved Hide resolved
substrate/frame/revive/fixtures/Cargo.toml Outdated Show resolved Hide resolved
substrate/frame/revive/fixtures/build/_Cargo.toml Outdated Show resolved Hide resolved
@davidk-pt davidk-pt added the T7-smart_contracts This PR/Issue is related to smart contracts. label Dec 13, 2024
@davidk-pt
Copy link
Contributor Author

Got tests and benchmarks passing, feature only in uapi, enabled the feature everywhere it is used

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

Looks much more what I had in mind now. Thanks.

substrate/frame/revive/Cargo.toml Outdated Show resolved Hide resolved
substrate/frame/revive/mock-network/Cargo.toml Outdated Show resolved Hide resolved
substrate/frame/revive/uapi/src/host.rs Outdated Show resolved Hide resolved
@davidk-pt davidk-pt marked this pull request as ready for review December 13, 2024 15:52
@davidk-pt davidk-pt requested a review from athei December 13, 2024 15:52
Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

Thanks just the prdoc missing some bumps.

prdoc/pr_6866.prdoc Show resolved Hide resolved
@athei
Copy link
Member

athei commented Dec 13, 2024

Thanks a lot. Let's get this in quickly. Will be a PITA to rebase.

@athei athei added this pull request to the merge queue Dec 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 13, 2024
@athei athei added this pull request to the merge queue Dec 13, 2024
@athei
Copy link
Member

athei commented Dec 13, 2024

The build-runtimes-polkavm is flaky because it apparently sometimes doesn't finish within the time limit. I will raise the time limit: #6893

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 13, 2024
@athei athei added this pull request to the merge queue Dec 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 13, 2024
@athei athei added this pull request to the merge queue Dec 13, 2024
Merged via the queue into master with commit ec69b61 Dec 13, 2024
182 of 201 checks passed
@athei athei deleted the davidk/revive-unstable-api branch December 13, 2024 23:38
github-merge-queue bot pushed a commit that referenced this pull request Dec 18, 2024
- Fixed failing docs.rs build for `pallet-revive-uapi` by fixing a wring
attribute in the manifest (we were using `default-target` instead of
`targets`)
- Removed the macros defining host functions because the cfg attributes
introduced in #6866 won't work on them
- Added an docs.rs specific attribute so that the `unstable-hostfn`
feature tag will show up on the functions that are guarded behind it.

---------

Co-authored-by: command-bot <>
dudo50 pushed a commit to paraspell-research/polkadot-sdk that referenced this pull request Jan 4, 2025
Follow up refactor to
paritytech#6844 (review)

I still need to finish adding `#[cfg(feature = "unstable-api")]` to the
rest of the tests and make sure all tests pass, I want to make sure I'm
moving into right direction first

@athei @xermicus

---------

Co-authored-by: DavidK <davidk@parity.io>
Co-authored-by: Alexander Theißen <alex.theissen@me.com>
dudo50 pushed a commit to paraspell-research/polkadot-sdk that referenced this pull request Jan 4, 2025
- Fixed failing docs.rs build for `pallet-revive-uapi` by fixing a wring
attribute in the manifest (we were using `default-target` instead of
`targets`)
- Removed the macros defining host functions because the cfg attributes
introduced in paritytech#6866 won't work on them
- Added an docs.rs specific attribute so that the `unstable-hostfn`
feature tag will show up on the functions that are guarded behind it.

---------

Co-authored-by: command-bot <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T7-smart_contracts This PR/Issue is related to smart contracts.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants