-
Notifications
You must be signed in to change notification settings - Fork 279
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
base: main
Are you sure you want to change the base?
Changes from 10 commits
a7e2a5f
acddbdc
1fdecf7
5384b00
986a835
7a5b957
3e6eb1f
65e547f
5c6e4a8
ade7dc3
441d939
fc8f661
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ on: workflow_dispatch | |
env: | ||
DEFAULTS_DIR: defaults | ||
WASM_TARGET_DIR: wasm/target/prebuilt | ||
PROFILE: profiling | ||
|
||
jobs: | ||
build_wasm_libs: | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why change to named argument instead of position based There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ env: | |
WASM_TARGET_DIR: wasm/target/prebuilt | ||
TEST_NETWORK_TMP_DIR: /tmp | ||
NEXTEST_PROFILE: ci | ||
PROFILE: profiling | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, it seems unused. you should use it to install iroha There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will remove. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO you should use it for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have no strong opinions on this There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
jobs: | ||
consistency: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> { | ||
|
@@ -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, | ||
} | ||
} | ||
|
||
|
@@ -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 | ||
|
@@ -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, | ||
}) | ||
} | ||
|
||
|
@@ -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<'_> { | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think some CI workflows should keep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CI will use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I would like CI to use |
||
"-Z", | ||
"build-std", | ||
"-Z", | ||
|
@@ -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 { | ||
|
@@ -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 | ||
} | ||
|
@@ -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() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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/" | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--target=libs
is easier to validate since only named arguments are used inbuild_wasm.sh
--profile=${{ env.IROHA2_PROFILE }}
works fordeploy
, but forPROFILE=profiling
, it will require changes in memory, fuel, and timeouts. I thinkdeploy
should be used everywhere by default.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
profiling
now?