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

Replace ethcore-logger with env-logger.#10102

Merged
debris merged 15 commits into
masterfrom
td-logger
Jan 8, 2019
Merged

Replace ethcore-logger with env-logger.#10102
debris merged 15 commits into
masterfrom
td-logger

Conversation

@tomusdrw
Copy link
Copy Markdown
Collaborator

  • env_logger is sufficient and we can avoid bringing additional path dependency.

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Dec 24, 2018
@tomaka
Copy link
Copy Markdown
Contributor

tomaka commented Dec 27, 2018

Shouldn't we remove the logger crate entirely as well?

@tomusdrw
Copy link
Copy Markdown
Collaborator Author

@tomaka It's still useful - allows to share the log format between main parity binary and whisper CLI + exposes latest logs via RPC method. None of this is super critical, so I guess we could remove it entirely (and integrate the setup with the main crate) if you believe it's the way to go.

@tomaka
Copy link
Copy Markdown
Contributor

tomaka commented Dec 28, 2018

if you believe it's the way to go.

I think this is the way to go, but we can do it later.

@5chdn 5chdn added this to the 2.3 milestone Dec 28, 2018
@sorpaas
Copy link
Copy Markdown
Collaborator

sorpaas commented Jan 3, 2019

Note that if we're really in the path to remove logger completely, we need to at least preserve those lines (https://github.com/paritytech/parity-ethereum/blob/b5f510ead742b1ac4c8e398339091e31926b07aa/logger/src/lib.rs#L70).

Comment thread secret-store/src/key_server.rs Outdated
#[test]
fn document_key_generation_and_retrievement_works_over_network_with_single_node() {
//::logger::init_log();
//::env_logger::try_init();
Copy link
Copy Markdown
Collaborator

@sorpaas sorpaas Jan 3, 2019

Choose a reason for hiding this comment

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

Not caused by this PR, but can we remove those lines as they are commented out? Or just uncomment it, because it's try_init and might break individual test invoking.

@tomusdrw
Copy link
Copy Markdown
Collaborator Author

tomusdrw commented Jan 3, 2019

I've removed ethcore-logger from whisper/cli as well and moved it to parity directory.

So now it's only required by rpc as well (to get the RotatingLogger), but I've added a warning to that RPC method that it's going to be removed in the future, so we remove it gracefuly.

Comment thread secret-store/src/key_server.rs Outdated
#[test]
fn document_key_generation_and_retrievement_works_over_network_with_3_nodes() {
//::logger::init_log();
::env_logger::try_init().ok();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe let _ = ::env_logger::try_init();?

#[test]
fn error_in_generation_session_broadcasted_to_all_other_nodes() {
//::logger::init_log();
//::env_logger::try_init();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And uncomment those as well!

@ngotchac
Copy link
Copy Markdown
Contributor

ngotchac commented Jan 3, 2019

Regarding the deprecation, should this be logged somewhere so it's not forgotten?

@tomusdrw
Copy link
Copy Markdown
Collaborator Author

tomusdrw commented Jan 3, 2019

@ngotchac logged here: #10129

@5chdn
Copy link
Copy Markdown
Contributor

5chdn commented Jan 4, 2019

Thanks. Could you resolve the conflicts?

@5chdn 5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 7, 2019
@debris debris merged commit ab22d5e into master Jan 8, 2019
@debris debris deleted the td-logger branch January 8, 2019 14:07
@abunsen
Copy link
Copy Markdown

abunsen commented May 21, 2019

I was brought here by the RPC method parity_devLogs - will logging still be in the RPC?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants