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.

Sign up for GitHub

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

Jump to bottom

feat: Add --release flag to wasm_builder #5209

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

feat: Add --release flag to wasm_builder #5209

wants to merge 32 commits into 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.

@aoyako aoyako changed the title Feat: Add --release flag to wasm_builder feat: Add --release flag to wasm_builder Oct 30, 2024
@aoyako aoyako marked this pull request as draft October 30, 2024 15:46
@aoyako
fix: use correct binary directory for debug
986a835 
Signed-off-by: Lohachov Mykhailo <[email protected]>
@aoyako
chore: cargo fmt
7a5b957 
Signed-off-by: Lohachov Mykhailo <[email protected]>
@aoyako aoyako marked this pull request as ready for review October 30, 2024 16:28
s8sato
s8sato reviewed Oct 30, 2024
View reviewed changes
scripts/build_wasm.sh Show resolved Hide resolved
crates/iroha_wasm_builder/src/lib.rs Outdated
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
s8sato
s8sato reviewed Oct 31, 2024
View reviewed changes
scripts/build_wasm.sh Outdated Show resolved Hide resolved
@aoyako aoyako marked this pull request as draft October 31, 2024 08:42
@aoyako aoyako marked this pull request as ready for review November 7, 2024 10:53
mversic
mversic reviewed Nov 12, 2024
View reviewed changes
scripts/build_wasm.sh Outdated Show resolved Hide resolved
mversic
mversic reviewed Nov 12, 2024
View reviewed changes
wasm/Cargo.toml
Comment on lines +19 to +20
[profile.deploy]
inherits = "release"
Copy link
Contributor

@mversic mversic Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
[profile.deploy]
inherits = "release"
[profile.release]
panic="abort"
[profile.deploy]
inherits = "release"
strip = "debuginfo"
lto = true
opt-level = "z"
codegen-units = 1

@aoyako
feat: use correct profile from cargo
b7820e4 
Signed-off-by: Lohachov Mykhailo <[email protected]>
@aoyako
Copy link
Contributor Author

aoyako commented Nov 12, 2024 •
edited
Loading

Recap:

  1. wasm_builder.sh now can take --profile to use profile from wasm/Cargo.toml when building wasms.
  2. wasm/Cargo.toml has two profiles:
    • deploy - optimized build, will also apply wasm-opt in the end.
    • test - no additional optimizations, same as release.
  3. PRs use test profile for testing, otherwise deploy is used as default.

@aoyako
chore: fmt
45df4a6 
Signed-off-by: Lohachov Mykhailo <[email protected]>
@aoyako
fix: rename profile test->release
dc2034e 
Signed-off-by: Lohachov Mykhailo <[email protected]>
mversic
mversic reviewed Nov 12, 2024
View reviewed changes
scripts/build_wasm.sh Outdated Show resolved Hide resolved
0x009922
0x009922 reviewed Nov 13, 2024
View reviewed changes
.config/nextest.toml
@@ -1,5 +1,5 @@
[profile.default]
retries = 2
retries = { backoff = "fixed", count = 2, delay = "3s" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For release it is good.

@aoyako
fix: readme for wasm builder
ab3054a 
Signed-off-by: Lohachov Mykhailo <[email protected]>
@aoyako
fix: use shell instead of bash
e3edc40 
Signed-off-by: Lohachov Mykhailo <[email protected]>
@github-actions github-actions bot added the config-changes Changes in configuration and start up of the Iroha label Nov 13, 2024
@github-actions GitHub Actions
Copy link

@BAStos525

mversic
mversic reviewed Nov 13, 2024
View reviewed changes
crates/iroha_wasm_builder/src/lib.rs Outdated
Comment on lines 28 to 31
/// let bytes = Builder::new("relative/path/to/smartcontract/", "release")
/// .out_dir("path/to/out/dir") // Optional: Set output directory
/// .build()? // Run build
/// .optimize()? // Optimize WASM output
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// let bytes = Builder::new("relative/path/to/smartcontract/", "release")
/// .out_dir("path/to/out/dir") // Optional: Set output directory
/// .build()? // Run build
/// .optimize()? // Optimize WASM output
/// let bytes = Builder::new("relative/path/to/smartcontract/", "release")
/// .out_dir("path/to/out/dir") // Optional: Set output directory
/// .build()? // Run build

shouldn't exist anymore, should be implied by the deploy profile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense.

@aoyako
fix: chec profile in ci
bd5ea86 
Signed-off-by: Lohachov Mykhailo <[email protected]>
@aoyako
fix: revert
0fb6a4f 
Signed-off-by: Lohachov Mykhailo <[email protected]>
@aoyako
feat: get logs from failing ci
fe1189f 
Signed-off-by: Lohachov Mykhailo <[email protected]>
@aoyako
feat: advanced logging
3182ebd 
Signed-off-by: Lohachov Mykhailo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BAStos525 BAStos525 BAStos525 left review comments

@s8sato s8sato s8sato left review comments

@mversic mversic mversic left review comments

@0x009922 0x009922 0x009922 left review comments

@SamHSmith SamHSmith Awaiting requested review from SamHSmith SamHSmith is a code owner

@dima74 dima74 Awaiting requested review from dima74 dima74 is a code owner

@outoftardis outoftardis Awaiting requested review from outoftardis outoftardis is a code owner

@DCNick3 DCNick3 Awaiting requested review from DCNick3 DCNick3 is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

@s8sato s8sato

@aoyako aoyako

Labels
config-changes Changes in configuration and start up of the Iroha
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add --release flag to iroha_wasm_builder
5 participants
@aoyako @0x009922 @mversic @s8sato @BAStos525