Skip to content

feat(workspace): enable sign-ext everywhere#2522

Merged
clearloop merged 6 commits intomasterfrom
cl/issue-2496
Apr 11, 2023
Merged

feat(workspace): enable sign-ext everywhere#2522
clearloop merged 6 commits intomasterfrom
cl/issue-2496

Conversation

@clearloop
Copy link
Copy Markdown
Contributor

@clearloop clearloop commented Apr 7, 2023 •
edited
Loading

Resolves #2496

  • patches all sign-ext related pacakges
  • re-enable sign-ext in wasm-opt of wasm-builder
  • workaround weights for sign-ext instructions

Patches

only annotated the #[feature = "sign-ext"] in these branches' rust files, kept features.sign-ext for preventing inner complation errors since some of the packages insde may still require this feature

@gear-tech/dev

clearloop
clearloop commented Apr 7, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@clearloop clearloop left a comment

Choose a reason for hiding this comment

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

The CI will confirm if everything works since this PR has enabled the sign-ext feature of wasm-opt which means all of the wasm binaries are built with sign-ext in CI now

clearloop

This comment was marked as duplicate.

Sign in to view

@clearloop clearloop mentioned this pull request Apr 7, 2023
Closed
@clearloop clearloop marked this pull request as ready for review April 7, 2023 17:59
@clearloop clearloop requested review from NikVolf, gshep and shamilsan April 7, 2023 17:59
@shamilsan
Copy link
Copy Markdown
Contributor

shamilsan commented Apr 7, 2023

Our wasm-instrument was patched comparing to the original one. @gshep Need to check whether it is correct now.

@clearloop
Copy link
Copy Markdown
Contributor Author

clearloop commented Apr 7, 2023

Our wasm-instrument was patched comparing to the original one. @gshep Need to check whether it is correct now.

my patch checked out from our previous patch, see the diff here

https://github.com/gear-tech/wasm-instrument/compare/gear-stable...gear-tech:wasm-instrument:v0.2.1-sign-ext?expand=1

shamilsan
shamilsan approved these changes Apr 9, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@shamilsan shamilsan left a comment

Choose a reason for hiding this comment

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

👍

gshep
gshep approved these changes Apr 10, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@gshep gshep left a comment

Choose a reason for hiding this comment

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

LGTM!

However I'm curious why not just enable sign_ext feature of both wasm-instrument crates instead of patching them?

@clearloop
Copy link
Copy Markdown
Contributor Author

clearloop commented Apr 10, 2023 •
edited
Loading

LGTM!

However I'm curious why not just enable sign_ext feature of both wasm-instrument crates instead of patching them?

sc-executor-common from substrate is using wasm-instrument without sign-ext, so...for preventing the potential errors, just replaced all, and since they all are patches, easy to switch back

UPDATED:

for our version, just adding "sign_ext" works!

@breathx
Copy link
Copy Markdown
Member

breathx commented Apr 11, 2023

@clearloop please, unpin nightly, rerun CI and merge asap if everything is ok

@clearloop clearloop mentioned this pull request Apr 11, 2023
Closed
@clearloop
Copy link
Copy Markdown
Contributor Author

clearloop commented Apr 11, 2023 •
edited
Loading

LGTM!

However I'm curious why not just enable sign_ext feature of both wasm-instrument crates instead of patching them?

followed

@clearloop please, unpin nightly, rerun CI and merge asap if everything is ok

#2530 (comment)

@clearloop clearloop requested a review from breathx April 11, 2023 14:49
@clearloop clearloop merged commit 7696300 into master Apr 11, 2023
@clearloop clearloop deleted the cl/issue-2496 branch April 11, 2023 14:56
@shamilsan shamilsan added the B1-releasenotes The feature deserves to be added to the Release Notes label Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NikVolf NikVolf NikVolf approved these changes

@gshep gshep gshep approved these changes

@breathx breathx Awaiting requested review from breathx

+1 more reviewer

@shamilsan shamilsan shamilsan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

B1-releasenotes The feature deserves to be added to the Release Notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Can't upload a program with new sign extention instructions

5 participants

@clearloop @shamilsan @breathx @NikVolf @gshep