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

refactor: evmbin: CLI flag for coloured log levels. CLI flag for output to log file #10761

Closed
wants to merge 13 commits into from

Conversation

ltfschoen
Copy link
Contributor

@ltfschoen ltfschoen commented Jun 19, 2019

Extension of PR #10657.
Not to be merged prior to #10657 and #10742

  • Temporarily added evmbin/res/create2callPrecompiles.json and evmbin/res/teststate.json just as input files for testing the code with CLI commands
  • Use fern crate for log level colours
  • Add CLI flag --logging to choose log level (consistent with parity-ethereum CLI)
  • Add CLI flag --logging-to-file to save outputs to output.log
  • Restrict use of logging levels in production (i.e. cargo build -p evmbin --release) to info! in Cargo.toml. In development users may use debug! or trace!.
  • Question 1 I made changes that to std_json.rs to add colours and log levels, but the tests failed so I reverted the changes in commit a687150. Were those changes on the right track?
    ANS: Note that we cannot convert writing to the stdout buffer with writeln! in std_json.rs to writing directly to stdout by use of macros with color like info!. See the module doc comments in std_json.rs

The changes may be tested as follows:

std-json

cargo build -p evmbin --release;
./target/release/parity-evm state-test ./evmbin/res/create2callPrecompiles.json --only create2callPrecompiles --chain Constantinople --logging 2 --std-json --std-dump-json

json

cargo build -p evmbin --release;
./target/release/parity-evm state-test ./evmbin/res/create2callPrecompiles.json --only create2callPrecompiles --chain Constantinople --logging 2 --json

output to a file output.log

cargo build -p evmbin --release;
./target/release/parity-evm state-test ./evmbin/res/create2callPrecompiles.json --only create2callPrecompiles --chain Constantinople --logging 2 --json --logging-to-file

@parity-cla-bot
Copy link

It looks like @ltfschoen signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@ltfschoen ltfschoen changed the base branch from master to luke-9459-evmbin-serde June 19, 2019 04:50
@ltfschoen ltfschoen added the A0-pleasereview 🤓 Pull request needs code review. label Jun 19, 2019
@ltfschoen ltfschoen changed the title refactor: evmbin: Additional serialization. CLI flag for log levels with colour and output to log file refactor: evmbin: CLI flag for coloured log levels. CLI flag for output to log file Jun 19, 2019
Update based on feedback from @todr
- Max level trace in development
- Max level info in production (i.e. cargo build -p evmbin --release)
- Only show date and target file name when debug or trace log level used.
… default log level

* Switch to more visible log colour
* Add missing license header
* Move use of default log level to CLI declaration, for same approach as was done in #10810
…um and Whisper CLIs

* Update Readme and CLI config so they are consistent.
* Use logging level 0 (Error) for evmbin CLI consistency in Readme and CLI config
* Rename `log_level` to `logging` (consistent with parity-ethereum crate CLI that is frequently used), as suggested here #10810 (comment)
* Update test to test all CLI commands including logging level and logging to file
@ltfschoen ltfschoen added the A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 26, 2019
@ordian
Copy link
Collaborator

ordian commented Aug 7, 2019

closing as stale

@ordian ordian closed this Aug 7, 2019
@niklasad1 niklasad1 deleted the luke-evmbin-logs branch November 12, 2019 20:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants