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(wasm-builder): New approach to build demos #3602

Closed
wants to merge 85 commits into from
Closed

Conversation

ark0f
Copy link
Member

@ark0f ark0f commented Dec 17, 2023

This is a new approach to build Gear programs which can replace gear-wasm-builder.

Schematic architecture:
image

Pipeline:
image

Purpose of program build script is just write features to lock file and don't use any cargo:rerun-if instructions so default cargo heuristics are enabled.

Binaries crate build script list its build dependencies and corresponding reads lock files to obtain program features then it runs cargo build -p program1 -p program2 so cargo builds WASM binaries effectively and concurrently. Then it writes to lock files to skip cargo invocation in next or in another binaries crate to avoid few seconds cache verification

Pros:

  • Advanced feature tracking.
    Features are tracked automatically so we don't need to re-run build script each time like in gear-wasm-builder (can be mitigated by cargo-metadata and Cargo.toml tracking)
  • Faster build.
    gear-wasm-builder builds program via new cargo command in the same target directory so each cargo process blocks other. wasm-dep-builder collects list of programs so only one cargo process runs.

Cons:

  • Create and import binaries crate.
    It is important to create binaries create that will list and build programs

Current problem:
image

When crate includes itself as dev dependency (any integration tests) with different feature set then cargo build and runs 2 different versions of build script - for workspace and for tests.

gear-wasm-builder case is that cargo builds and runs script just once with merged feature set.

So, how to resolve these features? Should we deliver different WASM binaries? Or just merge feature sets?
I vote for merging but we need some mechanism to obtain all of feature sets cargo resolved for each build script invocation of program before build script invocation of binaries crate if we don't want to waste time on WASM build with incomplete feature set

@ark0f ark0f added A1-inprogress Issue is in progress or PR draft is not ready to be reviewed D5-tooling Helper tools and utilities labels Dec 17, 2023
@clearloop
Copy link
Contributor

plz check this #3588 (comment) btw

Copy link
Member

@gshep gshep left a comment

Choose a reason for hiding this comment

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

Good job! 🥇

@@ -0,0 +1,21 @@
// This file is part of Gear.
//
// Copyright (C) 2023 Gear Technologies Inc.
Copy link
Member

Choose a reason for hiding this comment

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

I suggest removing the year number from the copyright since it is not consistent across the source code and we will not update it later

Copy link
Member Author

Choose a reason for hiding this comment

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

I see most of files have year when just a few don't

utils/wasm-dep-builder/src/utils.rs Show resolved Hide resolved
@@ -18,6 +18,8 @@

//! Module for memory pages.

#![allow(clippy::assertions_on_constants)]

Choose a reason for hiding this comment

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

merge master and remove this

@@ -0,0 +1 @@

Choose a reason for hiding this comment

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

why examples/signal-wait/** is empty?

@@ -213,6 +213,7 @@ gear-service = { path = "node/service", default-features = false }
gear-stack-buffer = { path = "stack-buffer" }
gear-utils = { path = "utils/utils" }
gear-wasm-builder = { path = "utils/wasm-builder", default-features = false }
wasm-dep-builder = { path = "utils/wasm-dep-builder" }
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to prefix it with gear as for the others?

[dependencies]
gcore.workspace = true
parity-scale-codec.workspace = true
gstd = { workspace = true }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
gstd = { workspace = true }
gstd.workspace = true

Comment on lines +5 to +6

[dependencies]
Copy link
Member

Choose a reason for hiding this comment

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

redundant

Comment on lines +5 to +6

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
Copy link
Member

Choose a reason for hiding this comment

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

redundant

Comment on lines +5 to +6

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
Copy link
Member

Choose a reason for hiding this comment

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

redundant

gmeta = { workspace = true }
gstd = { workspace = true, features = ["debug"] }
demo-wat.workspace = true
gclient-binaries = { path = "binaries" }
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to call it demo-binaries?

@@ -42,6 +42,7 @@ rand = { workspace = true, optional = true }

[dev-dependencies]
gsdk = { path = ".", features = ["testing"] }
gsdk-binaries = { path = "binaries" }
Copy link
Member

Choose a reason for hiding this comment

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

-> demo-binaries?

demo-meta-io.workspace = true
gtest-binaries = { path = "binaries" }
Copy link
Member

Choose a reason for hiding this comment

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

-> demo-binaries?

@@ -41,7 +41,9 @@ sp-inherents = { workspace = true, features = ["std"] }
prometheus-endpoint.workspace = true

[dev-dependencies]
gear-authorship-binaries = { path = "binaries" }
Copy link
Member

Choose a reason for hiding this comment

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

-> demo-binaries?

repository.workspace = true

[dependencies]
cargo_metadata = "0.18.1"
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to move them into the workspace deps?

@ark0f ark0f added A5-dontmerge PR should not be merged yet A3-gotissues PR occurred to have issues after the review and removed A5-dontmerge PR should not be merged yet labels Feb 7, 2024
@ark0f
Copy link
Member Author

ark0f commented Feb 7, 2024

cargo check may be failed because of missing .lock. Investigating...

@NikVolf
Copy link
Member

NikVolf commented Feb 25, 2024

Can you describe the approach in the PR outline?

@NikVolf
Copy link
Member

NikVolf commented Feb 26, 2024

Can we maybe put "dependency builder" inside gear-wasm-builder itself?

@breathx
Copy link
Member

breathx commented Feb 26, 2024

Can we maybe put "dependency builder" inside gear-wasm-builder itself?

It will replace gear_wasm_builder some day, so is there any reason to merge them now?

Just for notice: as it was mentioned, there are a few problems with these approach, and as far as we discussed with @ark0f they're already solved, but not pushed

@breathx breathx added the A5-dontmerge PR should not be merged yet label Feb 26, 2024
@ark0f
Copy link
Member Author

ark0f commented Mar 12, 2024

Can you describe the approach in the PR outline?

Done

@breathx
Copy link
Member

breathx commented May 6, 2024

Temporary closing in favour of opened referencing issue as won't be implemented

@breathx breathx closed this May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview PR is ready to be reviewed by the team A3-gotissues PR occurred to have issues after the review A5-dontmerge PR should not be merged yet D5-tooling Helper tools and utilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants