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

Add Target::Pipe #195

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 81 additions & 0 deletions examples/custom_target.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/*!
Using `env_logger`.

Before running this example, try setting the `MY_LOG_LEVEL` environment variable to `info`:

```no_run,shell
$ export MY_LOG_LEVEL='info'
```

Also try setting the `MY_LOG_STYLE` environment variable to `never` to disable colors
or `auto` to enable them:

```no_run,shell
$ export MY_LOG_STYLE=never
```
*/

#[macro_use]
extern crate log;
Copy link
Contributor

Choose a reason for hiding this comment

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

2018 idoms, you should be able to just call use log::{info, error, warn};. Is there a policy in-place for examples and what edition they should target? If not, I would use the newer syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would agree that this should be changed, but all other examples are very similar and use this line. So a bulk change of all the examples would be better. Should I still change it?


use env_logger::{Builder, Env, Target};
use std::{
io,
sync::mpsc::{channel, Sender},
};

// This struct is used as an adaptor, it implements io::Write and forwards the buffer to a mpsc::Sender
struct WriteAdapter {
sender: Sender<u8>,
}

impl io::Write for WriteAdapter {
// On write we forward each u8 of the buffer to the sender and return the length of the buffer
fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
for chr in buf {
self.sender.send(*chr).unwrap();
}
Ok(buf.len())
}

fn flush(&mut self) -> io::Result<()> {
Ok(())
}
}

fn main() {
// The `Env` lets us tweak what the environment
// variables to read are and what the default
// value is if they're missing
let env = Env::default()
.filter_or("MY_LOG_LEVEL", "trace")
// Normaly using a pipe as target would asume this as false, but this forces it to true
TilCreator marked this conversation as resolved.
Show resolved Hide resolved
.write_style_or("MY_LOG_STYLE", "always");

// Create the channel for the log messages
let (rx, tx) = channel();

Builder::from_env(env)
// The Sender of the channel is given to the logger
// The wrapper is used, because Sender itself doesn't implement io::Write
TilCreator marked this conversation as resolved.
Show resolved Hide resolved
.target(Target::Pipe(Box::new(WriteAdapter { sender: rx })))
.init();

trace!("some trace log");
debug!("some debug log");
info!("some information log");
warn!("some warning log");
error!("some error log");

// Collect all messages send to the channel and parse the result as a string
String::from_utf8(tx.try_iter().collect::<Vec<u8>>())
.unwrap()
// Split the result into lines so a prefix can be added to each line
.split("\n")
.for_each(|msg| {
// Print the message with a prefix if it has any content
if msg.len() > 0 {
println!("from pipe: {}", msg)
}
});
}
83 changes: 70 additions & 13 deletions src/fmt/writer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ mod termcolor;

use self::atty::{is_stderr, is_stdout};
use self::termcolor::BufferWriter;
use std::{fmt, io};
use std::{
fmt, io,
sync::{Arc, Mutex},
};

pub(in crate::fmt) mod glob {
pub use super::termcolor::glob::*;
Expand All @@ -12,13 +15,15 @@ pub(in crate::fmt) mod glob {

pub(in crate::fmt) use self::termcolor::Buffer;

/// Log target, either `stdout` or `stderr`.
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
/// Log target, either `stdout`, `stderr` or a custom pipe.
#[non_exhaustive]
pub enum Target {
/// Logs will be sent to standard output.
Stdout,
/// Logs will be sent to standard error.
Stderr,
/// Logs will be sent to a custom pipe.
Pipe(Box<dyn io::Write + Send + 'static>),
}

impl Default for Target {
Expand All @@ -27,6 +32,48 @@ impl Default for Target {
}
}

impl fmt::Debug for Target {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::result::Result<(), std::fmt::Error> {
write!(
f,
"{}",
match self {
Self::Stdout => "stdout",
Self::Stderr => "stderr",
Self::Pipe(_) => "pipe",
}
)
}
}

/// Log target, either `stdout`, `stderr` or a custom pipe.
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
#[non_exhaustive]
pub(in crate::fmt) enum TargetType {
/// Logs will be sent to standard output.
Stdout,
/// Logs will be sent to standard error.
Stderr,
/// Logs will be sent to a custom pipe.
Pipe,
}

impl From<&Target> for TargetType {
fn from(target: &Target) -> Self {
match target {
Target::Stdout => Self::Stdout,
Target::Stderr => Self::Stderr,
Target::Pipe(_) => Self::Pipe,
}
}
}

impl Default for TargetType {
fn default() -> Self {
Self::from(&Target::default())
}
}

/// Whether or not to print styles to the target.
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
pub enum WriteStyle {
Expand Down Expand Up @@ -68,7 +115,8 @@ impl Writer {
///
/// The target and style choice can be configured before building.
pub(crate) struct Builder {
target: Target,
target_type: TargetType,
target_pipe: Option<Arc<Mutex<dyn io::Write + Send + 'static>>>,
Comment on lines +118 to +119
Copy link
Contributor

Choose a reason for hiding this comment

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

Why split the target into two fields and introduce a new TargetType type? I think target can just be wrapped in Option for the build method to work (where you can then use self.target.take().unwrap_or_default() to get the actual value).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did try that, because I would like it way more that way, too. But the Target gets passed (and cloned) everywhere (for color checking and the shim_impl) that is why TargetType needs Clone, Copy, Debug, Eq, Hash, PartialEq implemented which is hard to impossible with dyn io::Write + Send + 'static.

write_style: WriteStyle,
is_test: bool,
built: bool,
Expand All @@ -78,7 +126,8 @@ impl Builder {
/// Initialize the writer builder with defaults.
pub(crate) fn new() -> Self {
Builder {
target: Default::default(),
target_type: Default::default(),
target_pipe: None,
write_style: Default::default(),
is_test: false,
built: false,
Expand All @@ -87,7 +136,11 @@ impl Builder {

/// Set the target to write to.
pub(crate) fn target(&mut self, target: Target) -> &mut Self {
self.target = target;
self.target_type = TargetType::from(&target);
self.target_pipe = match target {
Target::Stdout | Target::Stderr => None,
Target::Pipe(pipe) => Some(Arc::new(Mutex::new(pipe))),
};
self
}

Expand Down Expand Up @@ -119,9 +172,10 @@ impl Builder {

let color_choice = match self.write_style {
WriteStyle::Auto => {
if match self.target {
Target::Stderr => is_stderr(),
Target::Stdout => is_stdout(),
if match self.target_type {
TargetType::Stderr => is_stderr(),
TargetType::Stdout => is_stdout(),
TargetType::Pipe => false,
} {
WriteStyle::Auto
} else {
Expand All @@ -131,9 +185,12 @@ impl Builder {
color_choice => color_choice,
};

let writer = match self.target {
Target::Stderr => BufferWriter::stderr(self.is_test, color_choice),
Target::Stdout => BufferWriter::stdout(self.is_test, color_choice),
let writer = match self.target_type {
TargetType::Stderr => BufferWriter::stderr(self.is_test, color_choice),
TargetType::Stdout => BufferWriter::stdout(self.is_test, color_choice),
TargetType::Pipe => {
BufferWriter::pipe(color_choice, self.target_pipe.as_ref().unwrap().clone())
}
};

Writer {
Expand All @@ -152,7 +209,7 @@ impl Default for Builder {
impl fmt::Debug for Builder {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.debug_struct("Logger")
.field("target", &self.target)
.field("target", &self.target_type)
.field("write_style", &self.write_style)
.finish()
}
Expand Down
49 changes: 38 additions & 11 deletions src/fmt/writer/termcolor/extern_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@ use std::cell::RefCell;
use std::fmt;
use std::io::{self, Write};
use std::rc::Rc;
use std::sync::{Arc, Mutex};

use log::Level;
use termcolor::{self, ColorChoice, ColorSpec, WriteColor};

use crate::fmt::{Formatter, Target, WriteStyle};
use crate::fmt::{Formatter, TargetType, WriteStyle};

pub(in crate::fmt::writer) mod glob {
pub use super::*;
Expand Down Expand Up @@ -70,46 +71,72 @@ impl Formatter {

pub(in crate::fmt::writer) struct BufferWriter {
inner: termcolor::BufferWriter,
test_target: Option<Target>,
test_target_type: Option<TargetType>,
target_pipe: Option<Arc<Mutex<dyn io::Write + Send + 'static>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why Arc<Mutex<>> is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the pipe is still partly owned by the Builder (it doesn't get deconstructed on build()).

}

pub(in crate::fmt) struct Buffer {
inner: termcolor::Buffer,
test_target: Option<Target>,
test_target_type: Option<TargetType>,
}

impl BufferWriter {
pub(in crate::fmt::writer) fn stderr(is_test: bool, write_style: WriteStyle) -> Self {
BufferWriter {
inner: termcolor::BufferWriter::stderr(write_style.into_color_choice()),
test_target: if is_test { Some(Target::Stderr) } else { None },
test_target_type: if is_test {
Some(TargetType::Stderr)
} else {
None
},
target_pipe: None,
}
}

pub(in crate::fmt::writer) fn stdout(is_test: bool, write_style: WriteStyle) -> Self {
BufferWriter {
inner: termcolor::BufferWriter::stdout(write_style.into_color_choice()),
test_target: if is_test { Some(Target::Stdout) } else { None },
test_target_type: if is_test {
Some(TargetType::Stdout)
} else {
None
},
target_pipe: None,
}
}

pub(in crate::fmt::writer) fn pipe(
write_style: WriteStyle,
target_pipe: Arc<Mutex<dyn io::Write + Send + 'static>>,
) -> Self {
BufferWriter {
// The inner Buffer is never printed from, but it is still needed to handle coloring and other formating
inner: termcolor::BufferWriter::stderr(write_style.into_color_choice()),
test_target_type: None,
target_pipe: Some(target_pipe),
}
}

pub(in crate::fmt::writer) fn buffer(&self) -> Buffer {
Buffer {
inner: self.inner.buffer(),
test_target: self.test_target,
test_target_type: self.test_target_type,
}
}

pub(in crate::fmt::writer) fn print(&self, buf: &Buffer) -> io::Result<()> {
if let Some(target) = self.test_target {
if let Some(pipe) = &self.target_pipe {
pipe.lock().unwrap().write_all(&buf.bytes())
} else if let Some(target) = self.test_target_type {
// This impl uses the `eprint` and `print` macros
// instead of `termcolor`'s buffer.
// This is so their output can be captured by `cargo test`
let log = String::from_utf8_lossy(buf.bytes());

match target {
Target::Stderr => eprint!("{}", log),
Target::Stdout => print!("{}", log),
TargetType::Stderr => eprint!("{}", log),
TargetType::Stdout => print!("{}", log),
TargetType::Pipe => unreachable!(),
}

Ok(())
Expand Down Expand Up @@ -138,7 +165,7 @@ impl Buffer {

fn set_color(&mut self, spec: &ColorSpec) -> io::Result<()> {
// Ignore styles for test captured logs because they can't be printed
if self.test_target.is_none() {
if self.test_target_type.is_none() {
self.inner.set_color(spec)
} else {
Ok(())
Expand All @@ -147,7 +174,7 @@ impl Buffer {

fn reset(&mut self) -> io::Result<()> {
// Ignore styles for test captured logs because they can't be printed
if self.test_target.is_none() {
if self.test_target_type.is_none() {
self.inner.reset()
} else {
Ok(())
Expand Down
Loading