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

feat: Add --release flag to wasm_builder #5209

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

aoyako
Copy link
Contributor

@aoyako aoyako commented Oct 30, 2024

Context

Resolves #5202

Problem

The current version of iroha_wasm_builder has a --release flag set by default for its output.
The input --optimize flag can be set that will apply -O -Os.
Still, it cannot produce a build in debug mode.

Solution

Changes in iroha_wasm_builder

This PR introduces a single flag --release that:

  • When set, apply --release -O -Os to the build.
  • When not set, produce a debug build.

Changes in build_wasm.sh

Now, build_wasm.sh produces a debug build by default. Flag --release can be added to build in optimized mode.

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.

Signed-off-by: Lohachov Mykhailo <lohachov@soramitsu.co.jp>
Signed-off-by: Lohachov Mykhailo <lohachov@soramitsu.co.jp>
@aoyako aoyako changed the title Feat: Add --release flag to wasm_builder feat: Add --release flag to wasm_builder Oct 30, 2024
Signed-off-by: Lohachov Mykhailo <lohachov@soramitsu.co.jp>
@aoyako aoyako marked this pull request as draft October 30, 2024 15:46
Signed-off-by: Lohachov Mykhailo <lohachov@soramitsu.co.jp>
Signed-off-by: Lohachov Mykhailo <lohachov@soramitsu.co.jp>
@aoyako aoyako marked this pull request as ready for review October 30, 2024 16:28
Comment on lines -60 to -62
*)
echo "error: arg must be 'libs', 'samples', or empty to build both"
exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's reject invalid args so that we don't type e.g. --releas lib and get unintended results

Copy link
Contributor Author

@aoyako aoyako Nov 7, 2024

Choose a reason for hiding this comment

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

Changed to --profile={deploy, profiling} and --target={samples, libs}

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding whether named or position-based, either is acceptable but I personally prefer the shorter one

Comment on lines -194 to +206
"--release",
if self.release { "--release" } else { "" },
Copy link
Contributor

Choose a reason for hiding this comment

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

I think some CI workflows should keep --release build, at least iroha2-release.yml and maybe iroha2-custom-image.yml @BAStos525 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • it should definitely be release for iroha2-release.yml
  • for the custom image it should use the PROFILE variable

Copy link
Contributor

Choose a reason for hiding this comment

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

In iroha2-release.yml we use the tag taken from github.ref_name - it's the tag name. Should any changes be introduced?

Copy link
Contributor

Choose a reason for hiding this comment

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

Only *.wasm quality is discussed here, which would only affect e.g. this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CI will use release due to the need for additional resources (memory, fuel) and adjusted timeouts. Nevertheless, the debug flag can be set when building WASM.

Copy link
Contributor

Choose a reason for hiding this comment

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

CI will use release due to the need for additional resources (memory, fuel) and adjusted timeouts. Nevertheless, the debug flag can be set when building WASM.

I would like CI to use test when testing things and release (or deploy) when deploying. Yes, this will require you to adjust timeouts and limits in the test_network (don't modify parameters in genesis.json)

@s8sato s8sato assigned s8sato and aoyako and unassigned s8sato Oct 30, 2024
Signed-off-by: Lohachov Mykhailo <lohachov@soramitsu.co.jp>
scripts/build_wasm.sh Outdated Show resolved Hide resolved
@aoyako aoyako marked this pull request as draft October 31, 2024 08:42
Signed-off-by: Lohachov Mykhailo <lohachov@soramitsu.co.jp>
Signed-off-by: Lohachov Mykhailo <lohachov@soramitsu.co.jp>
Signed-off-by: Lohachov Mykhailo <lohachov@soramitsu.co.jp>
@aoyako aoyako marked this pull request as ready for review November 7, 2024 10:53
@@ -51,7 +51,7 @@ jobs:
with:
ref: ${{ github.event.inputs.CHECKOUT_REF }}
- name: Build wasm libs
run: ./scripts/build_wasm.sh --profile=${{ env.IROHA2_PROFILE }} --target=libs
run: ./scripts/build_wasm.sh --target=libs
Copy link
Contributor

Choose a reason for hiding this comment

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

this was good as it was, why change?

Copy link
Contributor

Choose a reason for hiding this comment

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

likewise for other files in CI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. --target=libs is easier to validate since only named arguments are used in build_wasm.sh
  2. --profile=${{ env.IROHA2_PROFILE }} works for deploy, but for PROFILE=profiling, it will require changes in memory, fuel, and timeouts. I think deploy should be used everywhere by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. ok, but how is someone to build a custom image with profile profiling now?

@@ -15,7 +16,7 @@ jobs:
steps:
- uses: actions/checkout@v4
- name: Build wasm libs
run: ./scripts/build_wasm.sh libs
run: ./scripts/build_wasm.sh --target=libs
Copy link
Contributor

Choose a reason for hiding this comment

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

why change to named argument instead of position based

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was easier to validate when all arguments were named. I can revert if these must be position-based.

Copy link
Contributor

Choose a reason for hiding this comment

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

revert

@@ -22,6 +22,7 @@ env:
WASM_TARGET_DIR: wasm/target/prebuilt
TEST_NETWORK_TMP_DIR: /tmp
NEXTEST_PROFILE: ci
PROFILE: profiling
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this not dev or test?

Copy link
Contributor

Choose a reason for hiding this comment

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

also, it seems unused. you should use it to install iroha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove.

Copy link
Contributor

@mversic mversic Nov 7, 2024

Choose a reason for hiding this comment

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

IMO you should use it for cargo install iroha. And you should set it to PROFILE: test

Copy link
Contributor

Choose a reason for hiding this comment

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

but others may disagree on what profile to use. @s8sato @0x009922 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

you will discover then that you need to increase limits because tests will get flaky

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no strong opinions on this

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm of the opinion that whenever we're running tests we should run the test build

Copy link
Contributor

Choose a reason for hiding this comment

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

but on the other hand tests will run slower. We should take that into consideration

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a very strong opinion on this too.

Speaking of the profile of irohad used by iroha_test_network, since the goal was to have a black-boxed environment close to what final users will have, it could be a point towards using release profile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add --release flag to iroha_wasm_builder
5 participants