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

Replace ethcore-logger with env-logger. #10102

Merged
merged 15 commits into from
Jan 8, 2019
Merged

Replace ethcore-logger with env-logger. #10102

merged 15 commits into from
Jan 8, 2019

Conversation

tomusdrw
Copy link
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
Contributor

tomaka commented Dec 27, 2018

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

@tomusdrw
Copy link
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
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
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).

@@ -325,7 +325,7 @@ pub mod tests {

#[test]
fn document_key_generation_and_retrievement_works_over_network_with_single_node() {
//::logger::init_log();
//::env_logger::try_init();
Copy link
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
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.

@@ -347,7 +347,7 @@ pub mod tests {

#[test]
fn document_key_generation_and_retrievement_works_over_network_with_3_nodes() {
//::logger::init_log();
::env_logger::try_init().ok();
Copy link
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();?

@@ -1360,7 +1360,7 @@ pub mod tests {

#[test]
fn error_in_generation_session_broadcasted_to_all_other_nodes() {
//::logger::init_log();
//::env_logger::try_init();
Copy link
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
Contributor

ngotchac commented Jan 3, 2019

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

@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Jan 3, 2019

@ngotchac logged here: #10129

@5chdn
Copy link
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

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