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

substrate/utils: enable wasm builder diagnostics propagation #5838

Merged

Conversation

iulianbarbu
Copy link
Contributor

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 initially and then here. 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 <iulian.barbu@parity.io>
@iulianbarbu iulianbarbu self-assigned this Sep 25, 2024
@iulianbarbu iulianbarbu requested a review from koute as a code owner September 25, 2024 18:38
@iulianbarbu iulianbarbu added the I5-enhancement An additional feature request. label Sep 25, 2024
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
This reverts commit c57e9ef.
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
@bkchr
Copy link
Member

bkchr commented Sep 25, 2024

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.

Yeah that doesn't sound really nice. You could for example introduce a env variable to do this.

@iulianbarbu
Copy link
Contributor Author

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.

Yeah that doesn't sound really nice. You could for example introduce a env variable to do this.

Great idea! Do you see it as a feature guarded by an env variable that should be either false/true? Will enable the change soon and also add some crate level docs to advertise this aspect.

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
@iulianbarbu iulianbarbu requested a review from bkchr September 26, 2024 12:01
iulianbarbu and others added 5 commits September 26, 2024 15:02
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
substrate/utils/wasm-builder/src/wasm_project.rs Outdated Show resolved Hide resolved
substrate/utils/wasm-builder/src/wasm_project.rs Outdated Show resolved Hide resolved
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
@iulianbarbu iulianbarbu added this pull request to the merge queue Sep 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 27, 2024
@iulianbarbu iulianbarbu added this pull request to the merge queue Sep 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 27, 2024
@bkchr bkchr added this pull request to the merge queue Sep 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 27, 2024
@bkchr bkchr enabled auto-merge September 27, 2024 21:12
@iulianbarbu iulianbarbu added this pull request to the merge queue Sep 28, 2024
Merged via the queue into paritytech:master with commit 58ade7a Sep 28, 2024
220 checks passed
@iulianbarbu iulianbarbu deleted the ib-wasmbuilder-propagate-diagnostics branch September 28, 2024 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants