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

Updated existing prints to make use of log crate #114

Closed
wants to merge 1 commit into from

Conversation

seanmichwalsh
Copy link

Imported the log and simple_logger crates and updated existing print statements to use it in reference to this issue. Supported macros include error!, warn!, info!, debug!, and trace!. The custom debug! macro used previously has been replaced with the equivalent in log. Logging level is set in main.rs based on the debug_assertions read in. When running in release mode, only macros error!, warn!, and info! will print information.

This is the first time tumbleshack and I have contributed to this project. We've tried to be consistent with existing conventions, but if we've made a mistake please let us know - we're happy to learn and fix it.

Signed-off-by: Sean Walsh [email protected]

@andreeaflorescu
Copy link
Member

Hey @seanmichwalsh thanks for your contribution!

Since we are adding new dependencies, we will also need to update the Cargo.lock file.
As a high level comment, we try to keep the dependencies at a bare minimum. With that in mind, I was thinking whether we can in fact just drop the simple_logger dependency. Do you think that would be feasible?

@tumbleshack
Copy link

tumbleshack commented Apr 5, 2021

Hi @andreeaflorescu , on the simple logger comment: it's definitely feasible to drop it and write that component ourselves. That said, the crate is very small: https://github.com/borntyping/rust-simple_logger

Its source is just one file of around 300 lines. If you'd prefer to leave it out, we can move that code into our own lib.rs file

@andreeaflorescu
Copy link
Member

Hi @andreeaflorescu , on the simple logger comment: it's definitely feasible to drop it and write that component ourselves. That said, the crate is very small: https://github.com/borntyping/rust-simple_logger

@tumbleshack even though simple_logger is very tiny, it is pulling in 8 transitive dependencies. You can see the newly added dependencies by looking at the Cargo.lock file. I was thinking it should be possible to get away with less dependencies.

Its source is just one file of around 300 lines. If you'd prefer to leave it out, we can move that code into our own lib.rs file

Pulling in the code from that dependency into our own lib.rs is not what I had in mind. We can look at how we can build the most minimal initialization of a log on top of the log crate. I haven't looked at the simple logger crate, but that might be a good starting point as well. Otherwise we can also check:

@tumbleshack
Copy link

You got it @andreeaflorescu ; we have spent the last couple of days fighting transitive dependencies on a different Rust project and understand the desire to avoid them.

We'll check out Cloud Hypervisor and Firecracker logging and let you know here if we have any questions.

@lauralt
Copy link
Collaborator

lauralt commented May 25, 2021

Hi, @seanmichwalsh, @tumbleshack! How are things going? Do you need help with anything?

@andreeaflorescu
Copy link
Member

Closing this due to lack of activity. If you want to start working on this again, feel free to reopen the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants