Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Make debug builds more usable #4683

Merged
merged 4 commits into from
Jan 21, 2020
Merged

Make debug builds more usable #4683

merged 4 commits into from
Jan 21, 2020

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Jan 20, 2020

This pr makes debug builds more usable in terms of cargo run -- --dev.

  1. --dev activates --execution native, iff --execution is not
    given or no sub --execution-* is given.
  2. It was probably a mistake to compile WASM in debug for a debug build.
    So, we now build the WASM binary always as release (if not requested
    differently by the user). So, we trade compilation time for a better
    debug experience.

This pr makes debug builds more usable in terms of `cargo run -- --dev`.

1. `--dev` activates `--execution native`, iff `--execution` is not
given or no sub `--execution-*` is given.
2. It was probably a mistake to compile WASM in debug for a debug build.
So, we now build the WASM binary always as `release` (if not requested
differently by the user). So, we trade compilation time for a better
debug experience.
@bkchr bkchr added the A0-please_review Pull request needs code review. label Jan 20, 2020
@bkchr bkchr requested a review from andresilva January 20, 2020 12:38
@bkchr bkchr requested a review from pepyakin as a code owner January 20, 2020 12:38
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Seems reasonable.

let exec = &cli.execution_strategies;
let exec_all_or = |strat: ExecutionStrategy| exec.execution.unwrap_or(strat).into();
let exec_all_or = |strat: ExecutionStrategy| execution.unwrap_or(strat).into();
Copy link
Contributor

Choose a reason for hiding this comment

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

This will default to execution even if strat is ovewritten in CLI, I thought you wanted to use strat and only default to execution, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we need to test whether exec was explicitly set or if it's a default as part of is_dev being true.

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

overall lgtm. issue regarding default handling.

let exec = &cli.execution_strategies;
let exec_all_or = |strat: ExecutionStrategy| exec.execution.unwrap_or(strat).into();
let exec_all_or = |strat: ExecutionStrategy| execution.unwrap_or(strat).into();
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we need to test whether exec was explicitly set or if it's a default as part of is_dev being true.

@bkchr
Copy link
Member Author

bkchr commented Jan 20, 2020

Okay, ignore the last commit.

client/cli/src/lib.rs Outdated Show resolved Hide resolved
@xlc
Copy link
Contributor

xlc commented Jan 21, 2020

I use this for development: SKIP_WASM_BUILD= cargo run -- --dev --execution=native -lruntime=debug. This saves the --execution=native part. Can we also have runtime logs enabled so we can see errors from failed extrinsics?

@bkchr
Copy link
Member Author

bkchr commented Jan 21, 2020

I use this for development: SKIP_WASM_BUILD= cargo run -- --dev --execution=native -lruntime=debug. This saves the --execution=native part. Can we also have runtime logs enabled so we can see errors from failed extrinsics?

I'm not sure, I looked at the output and it clearly changes between both. My intention of this pr was to make cargo run -- --dev working and preventing to have people being stuck over and over again.
For now I'm against changing the log filter. You can test a lot of stuff with the --dev chain, not just transactions and I see people coming and wanting to enable x or y logging. Not sure.

@bkchr bkchr merged commit cb9c181 into master Jan 21, 2020
@bkchr bkchr deleted the bkchr-better-debug branch January 21, 2020 11:33
@xlc
Copy link
Contributor

xlc commented Jan 21, 2020

The previous behavior was by default printing runtime logs including extrinsic failing messages and it was disabled by some logger changes and people were wondering why they can no longer see the logging messages from runtime. Those logs are not useful for a normal full node but super useful for developments purpose.

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -19,7 +19,7 @@ use wasm_builder_runner::{build_current_project_with_rustflags, WasmBuilderSourc
fn main() {
build_current_project_with_rustflags(
"wasm_binary.rs",
WasmBuilderSource::Crates("1.0.8"),
WasmBuilderSource::Crates("1.0.9"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to bump this on other substrate node template based projects?

Copy link
Member Author

Choose a reason for hiding this comment

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

When you want to always have release" builds, yes.

@bkchr
Copy link
Member Author

bkchr commented Jan 21, 2020

The previous behavior was by default printing runtime logs including extrinsic failing messages and it was disabled by some logger changes and people were wondering why they can no longer see the logging messages from runtime. Those logs are not useful for a normal full node but super useful for developments purpose.

Yeah these logs were disabled because of people complaining about the noise in the output when syncing :) FWIW, I will make sure that we document this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants