From da0bcb0ea2fd6aee3567c7893314a86067d1a3eb Mon Sep 17 00:00:00 2001 From: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com> Date: Fri, 27 Sep 2024 21:58:07 +0300 Subject: [PATCH] substrate/utils: enable wasm builder diagnostics propagation (#5838) # Description `substrate-wasm-builder` can be a build dependency for crates which develop FRAME runtimes. I had a tough time seeing errors happening in such crates (e.g. runtimes from the `templates` directory) in my IDE. I use a combination of rust-analyzer + nvim-lsp + nvim-lspconfig + rustacean.vim and all of this stack is not able to correctly parse errors emitted during the `build` phase. As a matter of fact there is also a cargo issue tracking specifically this issue where cargo doesn't propagate the `--message-format` type to the build phase: [here](https://github.com/rust-lang/cargo/issues/14246) initially and then [here](https://github.com/rust-lang/cargo/issues/8283). It feels like a solution for this use case isn't very close, so if it comes to runtimes development (both as an SDK user and developer), enabling wasm builder to emit diagnostics messages friendly to IDEs would be useful for regular workflows where IDEs are used for finding errors instead of manually running `cargo` commands. ## Integration It can be an issue if Substrate/FRAME SDKs users and developers rely on the runtimes' crates build phase output in certain ways. Emitting compilation messages as json will pollute the regular compilation output so people that would manually run `cargo build` or `cargo check` on their crates will have a tougher time extracting the non JSON output. ## Review Notes Rust IDEs based on rust-analyzer rely on cargo check/clippy to extract diagnostic information. The information is generated by passing flags like `--messages-format=json` to the `cargo` commands. The messages are extracted by rust-analyzer and published to LSP clients that will populate UIs accordingly. We need to build against the wasm target by using `message-format=json` too so that IDEs can show the errors for crates that have a build dependency on `substrate-wasm-builder`. --------- Signed-off-by: Iulian Barbu --- Cargo.lock | 1 + Cargo.toml | 1 + prdoc/pr_5838.prdoc | 20 +++++++++++++++++++ substrate/utils/wasm-builder/Cargo.toml | 1 + substrate/utils/wasm-builder/src/lib.rs | 7 +++++++ .../utils/wasm-builder/src/wasm_project.rs | 13 ++++++++++++ 6 files changed, 43 insertions(+) create mode 100644 prdoc/pr_5838.prdoc diff --git a/Cargo.lock b/Cargo.lock index 2e8eda3ca9e8a..c10d1e93907ed 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -24090,6 +24090,7 @@ dependencies = [ "parity-wasm", "polkavm-linker 0.9.2", "sc-executor 0.32.0", + "shlex", "sp-core 28.0.0", "sp-io 30.0.0", "sp-maybe-compressed-blob 11.0.0", diff --git a/Cargo.toml b/Cargo.toml index c40d59fce3c03..a30b5b57d3618 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1200,6 +1200,7 @@ sha1 = { version = "0.10.6" } sha2 = { version = "0.10.7", default-features = false } sha3 = { version = "0.10.0", default-features = false } shell-runtime = { path = "cumulus/parachains/runtimes/starters/shell" } +shlex = { version = "1.3.0" } slot-range-helper = { path = "polkadot/runtime/common/slot_range_helper", default-features = false } slotmap = { version = "1.0" } smallvec = { version = "1.11.0", default-features = false } diff --git a/prdoc/pr_5838.prdoc b/prdoc/pr_5838.prdoc new file mode 100644 index 0000000000000..f6ce091a12dec --- /dev/null +++ b/prdoc/pr_5838.prdoc @@ -0,0 +1,20 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: enable wasm builder diagnostics propagation + +doc: + - audience: Runtime Dev + description: | + `substrate-wasm-builder` is used as a build dependency by crates that implement FRAME runtimes. + Errors that occur in these crates can not be detected by IDEs that use rust-analyzer as a language + server because rust-analyzer needs the errors to be reported as diagnostic message in json format to + be able to publish them to language server clients. This PR adds `WASM_BUILD_CARGO_ARGS` environment + variable, which can hold a space separated list of args that will be parsed and passed to the `cargo` + command that it is used for building against wasm target. It can be used for the stated initial case, + but it is also flexible enough to allow passing other arguments or formatting the messages using another + available type. +crates: + - name: substrate-wasm-builder + bump: patch + diff --git a/substrate/utils/wasm-builder/Cargo.toml b/substrate/utils/wasm-builder/Cargo.toml index 15a1fd007ca25..8f0e8a23e54af 100644 --- a/substrate/utils/wasm-builder/Cargo.toml +++ b/substrate/utils/wasm-builder/Cargo.toml @@ -39,6 +39,7 @@ frame-metadata = { features = ["current"], optional = true, workspace = true, de codec = { optional = true, workspace = true, default-features = true } array-bytes = { optional = true, workspace = true, default-features = true } sp-tracing = { optional = true, workspace = true, default-features = true } +shlex = { workspace = true } [features] # Enable support for generating the metadata hash. diff --git a/substrate/utils/wasm-builder/src/lib.rs b/substrate/utils/wasm-builder/src/lib.rs index 07de4c15831b8..e3f2ff5cd7334 100644 --- a/substrate/utils/wasm-builder/src/lib.rs +++ b/substrate/utils/wasm-builder/src/lib.rs @@ -84,6 +84,9 @@ //! - `WASM_BUILD_STD` - Sets whether the Rust's standard library crates will also be built. This is //! necessary to make sure the standard library crates only use the exact WASM feature set that //! our executor supports. Enabled by default. +//! - `WASM_BUILD_CARGO_ARGS` - This can take a string as space separated list of `cargo` arguments. +//! It was added specifically for the use case of enabling JSON diagnostic messages during the +//! build phase, to be used by IDEs that parse them, but it might be useful for other cases too. //! - `CARGO_NET_OFFLINE` - If `true`, `--offline` will be passed to all processes launched to //! prevent network access. Useful in offline environments. //! @@ -161,6 +164,10 @@ const WASM_BUILD_WORKSPACE_HINT: &str = "WASM_BUILD_WORKSPACE_HINT"; /// Environment variable to set whether we'll build `core`/`std`. const WASM_BUILD_STD: &str = "WASM_BUILD_STD"; +/// Environment variable to set additional cargo arguments that might be useful +/// during the build phase. +const WASM_BUILD_CARGO_ARGS: &str = "WASM_BUILD_CARGO_ARGS"; + /// The target to use for the runtime. Valid values are `wasm` (default) or `riscv`. const RUNTIME_TARGET: &str = "SUBSTRATE_RUNTIME_TARGET"; diff --git a/substrate/utils/wasm-builder/src/wasm_project.rs b/substrate/utils/wasm-builder/src/wasm_project.rs index dcd5e6f1ade92..26edd2ea1f224 100644 --- a/substrate/utils/wasm-builder/src/wasm_project.rs +++ b/substrate/utils/wasm-builder/src/wasm_project.rs @@ -869,6 +869,18 @@ fn build_bloaty_blob( // We don't want to call ourselves recursively .env(crate::SKIP_BUILD_ENV, ""); + let cargo_args = env::var(crate::WASM_BUILD_CARGO_ARGS).unwrap_or_default(); + if !cargo_args.is_empty() { + let Some(args) = shlex::split(&cargo_args) else { + build_helper::warning(format!( + "the {} environment variable is not a valid shell string", + crate::WASM_BUILD_CARGO_ARGS + )); + std::process::exit(1); + }; + build_cmd.args(args); + } + #[cfg(feature = "metadata-hash")] if let Some(hash) = metadata_hash { build_cmd.env("RUNTIME_METADATA_HASH", array_bytes::bytes2hex("0x", &hash)); @@ -1140,6 +1152,7 @@ fn generate_rerun_if_changed_instructions( println!("cargo:rerun-if-env-changed={}", crate::WASM_BUILD_TOOLCHAIN); println!("cargo:rerun-if-env-changed={}", crate::WASM_BUILD_STD); println!("cargo:rerun-if-env-changed={}", crate::RUNTIME_TARGET); + println!("cargo:rerun-if-env-changed={}", crate::WASM_BUILD_CARGO_ARGS); } /// Track files and paths related to the given package to rerun `build.rs` on any relevant change.