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

build(wasm_samples): pre-compiled wasm_samples #5123

Merged
merged 7 commits into from
Oct 7, 2024

Conversation

0x009922
Copy link
Contributor

@0x009922 0x009922 commented Oct 4, 2024

Context

Split from #5087

There are numerous invocations of iroha_wasm_builder library interface from within tests/benches/examples.

Problems:

  • Critical: interferes with -C instrument-coverage rustc flag (refactor!: black-box integration tests #5087 (comment)), which is set when we run tests with coverage.
  • Inconsistent test runs: they are slow at first (while building WASMs for the first time), then fast. Reduces ability to set stricter time policies (which we could with Nextest).
  • Code duplication

Solution

  • No running of iroha_wasm_builder from tests. It makes possible to run all tests with coverage in a single CI run (that was achieved in refactor!: black-box integration tests #5087 (comment))
  • One script to rule them all build all wasm_samples, both locally and CI
  • Unified API: iroha_test_samples::load_sample_wasm("default_executor") -> WasmSmartContract
    • Has a descriptive error message.
  • Build all WASMs in one CI run

Migration Guide (optional)

For developers: ./scripts/build_wasm_samples.sh is a single command for you to prepare all WASMs and defaults/executor.wasm. You need to run it before running tests that depend on WASM samples.


Review notes (optional)

None

Checklist

  • I've read CONTRIBUTING.md.
  • (optional) I've written unit tests for the code changes.
  • All review comments have been resolved.
  • All CI checks pass.

@0x009922 0x009922 added CI Refactor Improvement to overall code quality labels Oct 4, 2024
@0x009922 0x009922 self-assigned this Oct 4, 2024
@mversic
Copy link
Contributor

mversic commented Oct 4, 2024

I notice you didn't remove the wasm build from the build scripts such as wasm_samples/multisig_register/build.rs?

@0x009922
Copy link
Contributor Author

0x009922 commented Oct 4, 2024

I notice you didn't remove the wasm build from the build scripts such as wasm_samples/multisig_register/build.rs?

Yes, I left it as-is. It just works, and I don't know how to be with it: shall multisig_register not include another WASM inside, but get it from somewhere else? E.g. it could be supplied as a payload with ExecuteTrigger ISI.

Co-authored-by: Marin Veršić <[email protected]>
Signed-off-by: 0x009922 <[email protected]>
@mversic
Copy link
Contributor

mversic commented Oct 4, 2024

I notice you didn't remove the wasm build from the build scripts such as wasm_samples/multisig_register/build.rs?

Yes, I left it as-is. It just works, and I don't know how to be with it: shall multisig_register not include another WASM inside, but get it from somewhere else? E.g. it could be supplied as a payload with ExecuteTrigger ISI.

yeah, it's ok. Building multisig_register will first build multisig.wasm

mversic
mversic previously approved these changes Oct 5, 2024
@s8sato s8sato self-assigned this Oct 7, 2024
@0x009922 0x009922 enabled auto-merge (squash) October 7, 2024 07:01
@0x009922 0x009922 merged commit 289d972 into hyperledger:main Oct 7, 2024
14 of 15 checks passed
@0x009922 0x009922 deleted the feat/pre-compiled-wasms branch October 7, 2024 07:25
@alexstroke1 alexstroke1 self-assigned this Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Refactor Improvement to overall code quality
Projects
Status: Testing & Verification
Development

Successfully merging this pull request may close these issues.

5 participants