-
Notifications
You must be signed in to change notification settings - Fork 23
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
fix: reference-types error on nightly version #375
Conversation
Also I've noticed that nightly-2024-10-20Checking contract erc721-metadata-example
stripped custom section from user wasm to remove any sensitive data
contract size: 23.3 KB
wasm data fee: 0.000129 ETH nightly-2024-09-05Checking contract erc721-metadata-example
stripped custom section from user wasm to remove any sensitive data
contract size: 22.2 KB
wasm data fee: 0.000125 ETH |
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.
Good job @qalisander, would be great if you can handle also #294 in this PR.
# the size limit. | ||
RUSTUP_TOOLCHAIN=${RUSTUP_TOOLCHAIN:-nightly} | ||
|
||
cargo build --release --target wasm32-unknown-unknown -Z build-std=std,panic_abort -Z build-std-features=panic_immediate_abort |
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.
I know you didn't add this as part of this PR, but why do we need these flags?
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.
Those are nightly compiler optimization flags. As I remember these flags been recommended for wasm size optimization in stylus docs.
I haven't tried to experiment with nightly flags further, since wasm size is fine for us.
target: wasm32-unknown-unknown | ||
components: rust-src | ||
toolchain: nightly-2024-01-01 | ||
rustflags: "" |
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.
Alas we should write rustflags: ""
everywhere. Otherwise our optimization rust flags will be replaced by -D warnings
. Strange behavior of this gh action tbh
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.
I like it, left small comments.
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.
LGTM!
It seems that reference types from WASM are not supported by
wasm-parser
crate (rustwasm/wasm-bindgen#4211).It makes us pin to the recent nightly toolchain version that works, and contribution experience gets quite convoluted.
Hopefully there is an option to disable
reference-types
with a flag:-C target-cpu=mvp
forwasm32-unknown-unknown
target.Resolves #374
Single source of rust toolchain from
rust-toolchain.toml
added for ci pipelines (actions-rust-lang/setup-rust-toolchain@v1 gh action used for that).Resolves #294