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 default features for time and compression #28

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
8 changes: 7 additions & 1 deletion .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,11 @@ jobs:
- os: windows-latest
steps:
- uses: actions/checkout@v3
- name: Test
- name: Test (default features)
run: cargo test
- name: Test (no default features)
run: cargo test --tests --no-default-features
- name: Test (compression feature only)
run: cargo test --tests --no-default-features --features compression
- name: Test (time feature only)
run: cargo test --tests --no-default-features --features time
9 changes: 7 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,14 @@ repository = "https://github.com/kstrafe/file-rotate"
keywords= ["log", "rotate", "logrotate"]
license = "MIT"

[features]
default = ["time", "compression"]
time = ["dep:chrono"]
compression = ["dep:flate2"]

[dependencies]
chrono = { version = "0.4.20", default-features = false, features = ["clock"] }
flate2 = "1.0"
chrono = { version = "0.4.20", default-features = false, features = ["clock"], optional = true }
flate2 = { version = "1.0", optional = true }

[dev-dependencies]
filetime = "0.2"
Expand Down
36 changes: 21 additions & 15 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,9 @@
unused_qualifications
)]

#[cfg(feature = "time")]
use chrono::prelude::*;
#[cfg(feature = "compression")]
use compression::*;
use std::io::{BufRead, BufReader};
use std::{
Expand All @@ -306,6 +308,7 @@ use suffix::*;
#[cfg(unix)]
use std::os::unix::fs::OpenOptionsExt;

#[cfg(feature = "compression")]
pub mod compression;
pub mod suffix;
#[cfg(test)]
Expand All @@ -315,6 +318,7 @@ mod tests;

/// At which frequency to rotate the file.
#[derive(Clone, Copy, Debug)]
#[cfg(feature = "time")]
pub enum TimeFrequency {
/// Rotate every hour.
Hourly,
Expand All @@ -336,6 +340,7 @@ pub enum ContentLimit {
/// Cut the log file at line breaks.
Lines(usize),
/// Cut the log at time interval.
#[cfg(feature = "time")]
Time(TimeFrequency),
/// Cut the log file after surpassing size in bytes (but having written a complete buffer from a write call.)
BytesSurpassed(usize),
Expand Down Expand Up @@ -385,15 +390,14 @@ impl<Repr: Representation> PartialOrd for SuffixInfo<Repr> {
pub struct FileRotate<S: SuffixScheme> {
basepath: PathBuf,
file: Option<File>,
modified: Option<DateTime<Local>>,
#[cfg(feature = "time")] modified: Option<DateTime<Local>>,
content_limit: ContentLimit,
count: usize,
compression: Compression,
#[cfg(feature = "compression")] compression: Compression,
suffix_scheme: S,
/// The bool is whether or not there's a .gz suffix to the filename
suffixes: BTreeSet<SuffixInfo<S::Repr>>,
#[cfg(unix)]
mode: Option<u32>,
#[cfg(unix)] mode: Option<u32>,
}

impl<S: SuffixScheme> FileRotate<S> {
Expand All @@ -411,7 +415,7 @@ impl<S: SuffixScheme> FileRotate<S> {
path: P,
suffix_scheme: S,
content_limit: ContentLimit,
compression: Compression,
#[cfg(feature = "compression")] compression: Compression,
Copy link
Owner

Choose a reason for hiding this comment

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

Not a fan of making the API contingent upon features; it makes swapping between features in a project using this library harder because the user will need to modify code each time. I suggest we instead panic if this argument is something that's not Compression::None when compression is disabled.

Copy link
Author

Choose a reason for hiding this comment

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

I agree that feature-gating args out of a parameter list is unclean. However, I'm not a fan of API that looks correct and smells correct but panics at runtime; that seems like it would just move the developer pain from compile-time to runtime and be more difficult to catch.

Some other ideas:

  • Instead of panicking if an API user tries to enable compression without the feature we no-op. This is still misleading, but at least it doesn't cause unexpected panics.
  • A builder pattern where we have feature-gated extension traits that add more setter functions. This would still require API users to make code changes when they enable/disable features, but it's limited to adding/removing a use on the extension trait. I've seen other popular libraries do this: as an arbitrary example, here's a trait of mac-specific builder functions that winit only implements for that specific platform.

Also, I'd hate to make you feel pressured to merge a PR you dislike so please don't feel like you need to merge this now just to clean it up later. If there's a specific way you want to see this implemented I'm happy to alter the PR. Or if this isn't something you want to consider right now that's also fine: I'm not in any rush.

Copy link
Owner

Choose a reason for hiding this comment

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

No-op sounds like a good solution. Ideally with a warning message printed at runtime (or compile time, if possible, but no compile/runtime errors) so we don't get bug reports here and users can easily identify what's wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me present an argument for panicking. The panic will happen only on creation of FileRotate, and I can only really see this object being constructed in the initialization (or very early) in the application when it runs. So any simple test of the application should show this - manual or automatic integration tests.

Here is one example from a popular library, not really of a panic but of silently leaving an application non-functioning due to a programmer error: If you use actix-web to build a web server, and you make a mistake in a request handler's parameters asking for app data of a type that has not been registered, it will not warn you at compile time... but your API will always return some 5xx http error saying that it's wrongly configured. Has it ever been a problem for me as a user of actix-web? No, because it makes all my tests fail and it's easily fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That said, a builder pattern sounds like a good solution as well. And it doesn't have to be through an extension trait IMO but maybe just a feature gated function on the builder? I agree that changing the number of parameters with #[cfg] can lead to confusion, but what do you think about feature gating builder functions, @kstrafe ?

#[cfg(unix)] mode: Option<u32>,
Copy link
Owner

Choose a reason for hiding this comment

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

Unrelated to this commit, but this code forces cross-platform code that uses this library to specify #[cfg(unix)]. I do not think this design is good. @Ploppz

) -> Self {
match content_limit {
Expand All @@ -421,7 +425,7 @@ impl<S: SuffixScheme> FileRotate<S> {
ContentLimit::Lines(lines) => {
assert!(lines > 0);
}
ContentLimit::Time(_) => {}
#[cfg(feature = "time")] ContentLimit::Time(_) => {}
ContentLimit::BytesSurpassed(bytes) => {
assert!(bytes > 0);
}
Expand All @@ -433,15 +437,14 @@ impl<S: SuffixScheme> FileRotate<S> {

let mut s = Self {
file: None,
modified: None,
#[cfg(feature = "time")] modified: None,
basepath,
content_limit,
count: 0,
compression,
#[cfg(feature = "compression")] compression,
suffixes: BTreeSet::new(),
suffix_scheme,
#[cfg(unix)]
mode,
#[cfg(unix)] mode,
};
s.ensure_log_directory_exists();
s.scan_suffixes();
Expand Down Expand Up @@ -473,6 +476,7 @@ impl<S: SuffixScheme> FileRotate<S> {
ContentLimit::Lines(_) => {
self.count = BufReader::new(file).lines().count();
}
#[cfg(feature = "time")]
ContentLimit::Time(_) => {
self.modified = mtime(file);
}
Expand Down Expand Up @@ -607,6 +611,7 @@ impl<S: SuffixScheme> FileRotate<S> {
}

// Compression
#[cfg(feature = "compression")]
if let Compression::OnRotate(max_file_n) = self.compression {
let n = (self.suffixes.len() as i32 - max_file_n as i32).max(0) as usize;
// The oldest N files should be compressed
Expand Down Expand Up @@ -652,6 +657,7 @@ impl<S: SuffixScheme> Write for FileRotate<S> {
file.write_all(buf)?;
}
}
#[cfg(feature = "time")]
ContentLimit::Time(time) => {
let local: DateTime<Local> = now();

Expand Down Expand Up @@ -741,7 +747,7 @@ impl<S: SuffixScheme> Write for FileRotate<S> {
}

/// Get modification time, in non test case.
#[cfg(not(test))]
#[cfg(all(not(test), feature = "time"))]
fn mtime(file: &File) -> Option<DateTime<Local>> {
if let Ok(time) = file.metadata().and_then(|metadata| metadata.modified()) {
return Some(time.into());
Expand All @@ -751,19 +757,19 @@ fn mtime(file: &File) -> Option<DateTime<Local>> {
}

/// Get modification time, in test case.
#[cfg(test)]
#[cfg(all(test, feature = "time"))]
fn mtime(_: &File) -> Option<DateTime<Local>> {
Some(now())
}

/// Get system time, in non test case.
#[cfg(not(test))]
#[cfg(all(not(test), feature = "time"))]
fn now() -> DateTime<Local> {
Local::now()
}

/// Get mocked system time, in test case.
#[cfg(test)]
#[cfg(all(test, feature = "time"))]
pub mod mock_time {
use super::*;
use std::cell::RefCell;
Expand All @@ -781,5 +787,5 @@ pub mod mock_time {
}
}

#[cfg(test)]
#[cfg(all(test, feature = "time"))]
pub use mock_time::now;
17 changes: 15 additions & 2 deletions src/suffix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@
//! This behaviour is fully extensible through the [SuffixScheme] trait, and two behaviours are
//! provided: [AppendCount] and [AppendTimestamp]
//!
#[cfg(feature = "time")]
use super::now;
use crate::SuffixInfo;
#[cfg(feature = "time")]
use chrono::{format::ParseErrorKind, offset::Local, Duration, NaiveDateTime};
#[cfg(feature = "time")]
use std::cmp::Ordering;
use std::{
cmp::Ordering,
collections::BTreeSet,
io,
path::{Path, PathBuf},
Expand Down Expand Up @@ -141,6 +144,7 @@ impl SuffixScheme for AppendCount {
}

/// Add timestamp from:
#[cfg(feature = "time")]
pub enum DateFrom {
/// Date yesterday, to represent the timestamps within the log file.
DateYesterday,
Expand All @@ -156,6 +160,7 @@ pub enum DateFrom {
/// Current limitations:
/// - Neither `format` nor the base filename can include the character `"."`.
/// - The `format` should ensure that the lexical and chronological orderings are the same
#[cfg(feature = "time")]
pub struct AppendTimestamp {
/// The format of the timestamp suffix
pub format: &'static str,
Expand All @@ -165,6 +170,7 @@ pub struct AppendTimestamp {
pub date_from: DateFrom,
}

#[cfg(feature = "time")]
impl AppendTimestamp {
/// With format `"%Y%m%dT%H%M%S"`
pub fn default(file_limit: FileLimit) -> Self {
Expand All @@ -186,13 +192,16 @@ impl AppendTimestamp {

/// Structured representation of the suffixes of AppendTimestamp.
#[derive(Debug, Clone, PartialEq, Eq)]
#[cfg(feature = "time")]
pub struct TimestampSuffix {
/// The timestamp
pub timestamp: String,
/// Optional number suffix if two timestamp suffixes are the same
pub number: Option<usize>,
}
#[cfg(feature = "time")]
impl Representation for TimestampSuffix {}
#[cfg(feature = "time")]
impl Ord for TimestampSuffix {
fn cmp(&self, other: &Self) -> Ordering {
// Most recent = smallest (opposite as the timestamp Ord)
Expand All @@ -203,11 +212,13 @@ impl Ord for TimestampSuffix {
}
}
}
#[cfg(feature = "time")]
impl PartialOrd for TimestampSuffix {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Some(self.cmp(other))
}
}
#[cfg(feature = "time")]
impl std::fmt::Display for TimestampSuffix {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
match self.number {
Expand All @@ -217,6 +228,7 @@ impl std::fmt::Display for TimestampSuffix {
}
}

#[cfg(feature = "time")]
impl SuffixScheme for AppendTimestamp {
type Repr = TimestampSuffix;

Expand Down Expand Up @@ -304,12 +316,13 @@ pub enum FileLimit {
/// Delete the oldest files if number of files is too high
MaxFiles(usize),
/// Delete files whose age exceeds the `Duration` - age is determined by the suffix of the file
#[cfg(feature = "time")]
Age(Duration),
/// Never delete files
Unlimited,
}

#[cfg(test)]
#[cfg(all(test, feature = "time"))]
mod test {
use super::*;
use std::fs::File;
Expand Down
Loading
Loading