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

fix(wasm-builder): allow building with stable toolchain #3459

Merged
merged 31 commits into from
Jan 18, 2024

Conversation

StackOverflowExcept1on
Copy link
Member

@StackOverflowExcept1on StackOverflowExcept1on commented Oct 30, 2023

This PR fixes build with +stable

Tests:

cd utils/wasm-builder/test-program
cargo +stable build --release

@gear-tech/dev

@StackOverflowExcept1on StackOverflowExcept1on added the A0-pleasereview PR is ready to be reviewed by the team label Oct 30, 2023
@clearloop
Copy link
Contributor

plz provide a test of stable build with test-program btw

https://github.com/gear-tech/gear/tree/master/utils/wasm-builder/test-program

mb override the rust-toolchain of this crate or using environment to do the test

@StackOverflowExcept1on
Copy link
Member Author

We have smoke tests in https://github.com/gear-tech/gear/blob/master/utils/wasm-builder/tests/smoke.rs but it's marked with ignored to do not waste time on make pre-commit. It's tested every Monday here: https://github.com/gear-tech/gear/actions/workflows/time-consuming-tests.yml

I think we can test this program with both toolchains in time-consuming-tests environment.

For the check action, we can also add cargo +stable build

- name: "Check: Compiling gstd on stable"
run: cargo +stable check -p gstd

@NikVolf
Copy link
Member

NikVolf commented Oct 30, 2023

We have smoke tests in https://github.com/gear-tech/gear/blob/master/utils/wasm-builder/tests/smoke.rs but it's marked with ignored to do not waste time on make pre-commit. It's tested every Monday here: https://github.com/gear-tech/gear/actions/workflows/time-consuming-tests.yml

I think we can test this program with both toolchains in time-consuming-tests environment.

For the check action, we can also add cargo +stable build

- name: "Check: Compiling gstd on stable"
run: cargo +stable check -p gstd

yeah, add this pls

@clearloop
Copy link
Contributor

clearloop commented Oct 30, 2023

- name: "Check: Compiling gstd on stable"
run: cargo +stable check -p gstd

wait, I believe this test should be

cargo +stable check -p gstd --target wasm32-unknown-unkown

the current

cargo +stable check -p gstd 

missed the wasm32-unknown-unknown target, so we failed to find the issue in CI

@StackOverflowExcept1on
Copy link
Member Author

@clearloop I think we shouldn't use --target because the wasm-builder will switch to nightly toolchain.

@StackOverflowExcept1on StackOverflowExcept1on added the D5-tooling Helper tools and utilities label Oct 30, 2023
@breathx
Copy link
Member

breathx commented Oct 31, 2023

@ark0f please take a look

@StackOverflowExcept1on StackOverflowExcept1on changed the title fix(wasm-builder): remove build script environment variables fix(wasm-builder): allow building with stable toolchain Oct 31, 2023
utils/wasm-builder/src/cargo_command.rs Outdated Show resolved Hide resolved
utils/wasm-builder/src/cargo_toolchain.rs Outdated Show resolved Hide resolved
@breathx breathx added A2-mergeoncegreen PR is ready to merge after CI passes and removed A0-pleasereview PR is ready to be reviewed by the team labels Nov 10, 2023
@StackOverflowExcept1on
Copy link
Member Author

Will be continued after #3527

@StackOverflowExcept1on StackOverflowExcept1on added A0-pleasereview PR is ready to be reviewed by the team and removed A5-dontmerge PR should not be merged yet labels Jan 10, 2024
Copy link
Member

@breathx breathx left a comment

Choose a reason for hiding this comment

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

@StackOverflowExcept1on @ark0f @breathx
Relates #3527 (review)

# Wasm builder changes
# Use the same toolchain: `gear_wasm_builder::build::<Metadata>();`
# Use recommended nightly: `gear_wasm_builder::recommended_nightly::<Metadata>();` // ERROR IF VERSION IS DIFFERENT

utils/wasm-builder/src/cargo_command.rs Outdated Show resolved Hide resolved
utils/wasm-builder/src/cargo_toolchain.rs Show resolved Hide resolved
.github/workflows/check.yml Outdated Show resolved Hide resolved
.github/workflows/check.yml Show resolved Hide resolved
.github/workflows/check.yml Outdated Show resolved Hide resolved
utils/wasm-builder/src/lib.rs Outdated Show resolved Hide resolved
utils/wasm-builder/src/lib.rs Outdated Show resolved Hide resolved
utils/wasm-builder/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Dmitry Novikov <[email protected]>
examples/out-of-memory/build.rs Outdated Show resolved Hide resolved
@StackOverflowExcept1on StackOverflowExcept1on added A2-mergeoncegreen PR is ready to merge after CI passes and removed A0-pleasereview PR is ready to be reviewed by the team labels Jan 17, 2024
@StackOverflowExcept1on StackOverflowExcept1on merged commit 59e253b into master Jan 18, 2024
13 checks passed
@StackOverflowExcept1on StackOverflowExcept1on deleted the av/wasm-builder-clean-env branch January 18, 2024 09:54
@shamilsan shamilsan added the B1-releasenotes The feature deserves to be added to the Release Notes label Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A2-mergeoncegreen PR is ready to merge after CI passes B1-releasenotes The feature deserves to be added to the Release Notes D5-tooling Helper tools and utilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants