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
2 changes: 1 addition & 1 deletion .github/workflows/iroha2-custom-image.yml
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ jobs:
with:
ref: ${{ github.event.inputs.CHECKOUT_REF }}
- 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.

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?

- name: Upload wasm libs to reuse in other jobs
uses: actions/upload-artifact@v4
with:
Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/iroha2-dev-nightly.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ on: workflow_dispatch
env:
DEFAULTS_DIR: defaults
WASM_TARGET_DIR: wasm/target/prebuilt
PROFILE: profiling

jobs:
build_wasm_libs:
Expand All @@ -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

- name: Upload wasm libs to reuse in other jobs
uses: actions/upload-artifact@v4
with:
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/iroha2-dev-pr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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.


jobs:
consistency:
Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/iroha2-dev.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ env:
WASM_TARGET_DIR: wasm/target/prebuilt
ARTIFACTS_DIR: tmp/artifacts
BIN_PATH: /usr/local/bin
PROFILE: profiling

jobs:
build_wasm_libs:
Expand All @@ -20,7 +21,7 @@ jobs:
steps:
- uses: actions/checkout@v4
- name: Build wasm libs
run: ./scripts/build_wasm.sh libs
run: ./scripts/build_wasm.sh --target=libs
- name: Upload wasm libs to reuse in other jobs
uses: actions/upload-artifact@v4
with:
Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/iroha2-release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ env:
CARGO_TERM_COLOR: always
DEFAULTS_DIR: defaults
WASM_TARGET_DIR: wasm/target/prebuilt
PROFILE: deploy

jobs:
build_wasm_libs:
Expand All @@ -19,7 +20,7 @@ jobs:
steps:
- uses: actions/checkout@v4
- name: Build wasm libs
run: ./scripts/build_wasm.sh libs
run: ./scripts/build_wasm.sh --target=libs
- name: Upload wasm libs to reuse in other jobs
uses: actions/upload-artifact@v4
with:
Expand Down
Binary file modified crates/iroha_codec/samples/trigger.bin
Binary file not shown.
2 changes: 0 additions & 2 deletions crates/iroha_numeric/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -424,9 +424,7 @@ mod scale_ {
#[derive(Encode, Decode)]
// Use compact encoding for efficiency, for integer numbers scale takes only one byte
struct NumericScaleHelper {
#[codec(compact)]
mantissa: u128,
#[codec(compact)]
scale: u32,
}

Expand Down
25 changes: 21 additions & 4 deletions crates/iroha_wasm_builder/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ pub struct Builder<'path, 'out_dir> {
out_dir: Option<&'out_dir Path>,
/// Flag controlling whether to show output of the build process
show_output: bool,
release: bool,
}

impl<'path, 'out_dir> Builder<'path, 'out_dir> {
Expand All @@ -59,6 +60,7 @@ impl<'path, 'out_dir> Builder<'path, 'out_dir> {
path: relative_path.as_ref(),
out_dir: None,
show_output: false,
release: false,
}
}

Expand All @@ -83,6 +85,14 @@ impl<'path, 'out_dir> Builder<'path, 'out_dir> {
self
}

/// Enable release build optimizations.
///
/// Disabled by default.
pub fn release(mut self) -> Self {
self.release = true;
self
}

/// Apply `cargo check` to the smartcontract.
///
/// # Errors
Expand Down Expand Up @@ -114,6 +124,7 @@ impl<'path, 'out_dir> Builder<'path, 'out_dir> {
|out_dir| Ok(Cow::Borrowed(out_dir)),
)?,
show_output: self.show_output,
release: self.release,
})
}

Expand Down Expand Up @@ -167,6 +178,7 @@ mod internal {
pub absolute_path: PathBuf,
pub out_dir: Cow<'out_dir, Path>,
pub show_output: bool,
pub release: bool,
}

impl Builder<'_> {
Expand All @@ -189,9 +201,9 @@ mod internal {
})
}

fn build_options() -> impl Iterator<Item = &'static str> {
fn build_options(&self) -> impl Iterator<Item = &'static str> {
[
"--release",
if self.release { "--release" } else { "" },
Comment on lines -194 to +206
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)

"-Z",
"build-std",
"-Z",
Expand All @@ -202,6 +214,7 @@ mod internal {
"wasm32-unknown-unknown",
]
.into_iter()
.filter(|&arg| !arg.is_empty())
}

fn get_base_command(&self, cmd: &'static str) -> std::process::Command {
Expand All @@ -210,7 +223,7 @@ mod internal {
.current_dir(&self.absolute_path)
.stderr(Stdio::inherit())
.arg(cmd)
.args(Self::build_options());
.args(self.build_options());

command
}
Expand All @@ -226,7 +239,11 @@ mod internal {
.retrieve_package_name()
.wrap_err("Failed to retrieve package name")?;

let full_out_dir = self.out_dir.join("wasm32-unknown-unknown/release/");
let build_mode_dir = if self.release { "release" } else { "debug" };
let full_out_dir = self
.out_dir
.join(format!("wasm32-unknown-unknown"))
.join(build_mode_dir);
let wasm_file = full_out_dir.join(package_name).with_extension("wasm");

let previous_hash = if wasm_file.exists() {
Expand Down
8 changes: 5 additions & 3 deletions crates/iroha_wasm_builder/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ enum Cli {
common: CommonArgs,
/// Optimize WASM output.
#[arg(long)]
optimize: bool,
release: bool,
/// Where to store the output WASM. If the file exists, it will be overwritten.
#[arg(long)]
out_file: PathBuf,
Expand All @@ -44,11 +44,13 @@ fn main() -> color_eyre::Result<()> {
}
Cli::Build {
common: CommonArgs { path },
optimize,
release,
out_file,
} => {
let builder = Builder::new(&path).show_output();

let builder = if release { builder.release() } else { builder };

let output = {
// not showing the spinner here, cargo does a progress bar for us

Expand All @@ -58,7 +60,7 @@ fn main() -> color_eyre::Result<()> {
}
};

let output = if optimize {
let output = if release {
let sp = if std::env::var("CI").is_err() {
Some(spinoff::Spinner::new_with_stream(
spinoff::spinners::Binary,
Expand Down
92 changes: 76 additions & 16 deletions scripts/build_wasm.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,64 @@ set -e;
DEFAULTS_DIR="defaults"
CARGO_DIR="wasm"
TARGET_DIR="$CARGO_DIR/target/prebuilt"
PROFILE="deploy"
TARGET="all"
SHOW_HELP=false

main() {
# Parse args
for arg in "$@"; do
case $arg in
--profile=*)
PROFILE="${arg#*=}"
;;
--help)
SHOW_HELP=true
;;
--target=*)
TARGET="${arg#*=}"
;;
*)
echo "error: unrecognized arg: $arg"
exit 1
;;
esac
done

case $PROFILE in
"deploy")
RELEASE_FLAG="--release"
;;
"profiling")
RELEASE_FLAG=""
;;
*)
echo "error: unrecognized profile: $PROFILE. Profile can be either [deploy, profiling]"
exit 1
;;
esac

if $SHOW_HELP; then
print_help
exit 0
fi

# Parse target
case $TARGET in
"libs")
command "libs"
;;
"samples")
command "samples"
;;
"all")
command "libs"
command "samples"
;;
*)
echo "error: unrecognized target: $TARGET. Target can be either [libs, samples, all]"
esac
}

build() {
case $1 in
Expand All @@ -26,7 +84,7 @@ build() {
mkdir -p "$TARGET_DIR/$1"
for name in ${NAMES[@]}; do
out_file="$TARGET_DIR/$1/$name.wasm"
cargo run --bin iroha_wasm_builder -- build "$CARGO_DIR/$1/$name" --optimize --out-file "$out_file"
cargo run --bin iroha_wasm_builder -- build "$CARGO_DIR/$1/$name" $RELEASE_FLAG --out-file "$out_file"
done
echo "info: WASM $1 build complete"
echo "artifacts written to $TARGET_DIR/$1/"
Expand All @@ -46,18 +104,20 @@ command() {
esac
}

case $1 in
"")
command "libs"
command "samples"
;;
"libs")
command "libs"
;;
"samples")
command "samples"
;;
*)
echo "error: arg must be 'libs', 'samples', or empty to build both"
exit 1
Comment on lines -60 to -62
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

esac

print_help() {
cat << END
Usage: $0 [OPTIONS]

Options:
--profile=<value> Specify build profile (default: profiling)
Possible values: profiling, deploy

--target=<value> Specify build target (default: all)
Possible values: samples, libs, all

--help Show help message
END
}

main "$@"; exit
14 changes: 12 additions & 2 deletions wasm/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,21 @@ bash scripts/build_wasm.sh
## WASM libraries only

```bash
bash scripts/build_wasm.sh libs
bash scripts/build_wasm.sh --target=libs
```

## WASM samples only

```bash
bash scripts/build_wasm.sh samples
bash scripts/build_wasm.sh --target=samples
```

## WASM in specific profile
1. Release **(default)**
```bash
bash scripts/build_wasm.sh --profile=deploy
```
2. Debug
```bash
bash scripts/build_wasm.sh --profile=profiling
```
Loading