Ensure 0.3 works with the next major release

alexcrichton commented

2017-05-19 14:59:32 -05:00

(Migrated from github.com)

We'll want to publish a version of 0.3 that works as a compatibility layer to the 0.4 version of the log crate

We'll want to publish a version of 0.3 that works as a compatibility layer to the 0.4 version of the `log` crate

daboross commented

2017-05-23 23:17:34 -05:00

(Migrated from github.com)

If it's alright and no one else has started, I can start working on a shared backend crate for this.

I'm trying to think of the best way to have it be efficient, while still allowing for maximum change. Right now my best idea is to have a versioned API that would be somewhat like the following:

pub fn register_v1_log(log: *const V1Log) -> Result<(), ...>;

pub fn log_v1(record: V1Record);

trait V1Log {
    fn log(&self, record: V1Record);
}

V1Record would be a mirror of what the log crate's current LogRecord struct is, and the log crate would simply have a newtype wrapper around it for the exported LogRecord type.

V1Record would be constructed with a V1RecordBuilder: #[inline(always)] builder methods should allow for the same performance we have now without a builder, and this would allow us to add more fields without bumping to V2Record immediately.

V1Log would have a blanket impl like this in log:

#[doc(hidden)]
impl<T: Log> V1Log for T {
    fn log(&self, record: V1Record) {
        <Self as Log>>::log(&LogRecord { inner: record });
    }
}

For the next version of log which brings an incompatible LogRecord format, log_impl would introduce V2Log and V2Record to match, and would impl From<V1Record> for V2Record and vice versa. I would name V1 rather than V0_3 just because I wouldn't expect every single log crate release to need a new, incompatible record version.

Internally, log_impl would store an enum VersionedLog which could be a box of any of the V*Log traits, and would convert records accordingly when logged.

The log_impl would provide one other feature, a set of macros if_trace_enabled!() through if_error_enabled!(), which would be controlled through max_level_* and release_max_level_* crate features, similar to log's current ones. These macros would then be used in log instead of the features, and the log features would simply forward to depending on the log_impl ones.

There would be overhead of converting records if the log macro version and logger version don't match, but I believe a scheme like this would fully support both public breaking API changes in log, and breaking changes in how LogRecord is internally structured.

If we don't implement any breaking changes in LogRecords format, we could even just have log_impl with V1* always, and there would be no overhead.

Unresolved question with this idea: v0.3 takes in Box<Log>, which I don't believe can be converted to Box<V1Log>, even if a blanket implementation exists. Might make this unreasonable.

If it's alright and no one else has started, I can start working on a shared backend crate for this. I'm trying to think of the best way to have it be efficient, while still allowing for maximum change. Right now my best idea is to have a versioned API that would be somewhat like the following: ```rust pub fn register_v1_log(log: *const V1Log) -> Result<(), ...>; pub fn log_v1(record: V1Record); trait V1Log { fn log(&self, record: V1Record); } ``` `V1Record` would be a mirror of what the `log` crate's current `LogRecord` struct is, and the `log` crate would simply have a newtype wrapper around it for the exported `LogRecord` type. `V1Record` would be constructed with a `V1RecordBuilder`: `#[inline(always)]` builder methods should allow for the same performance we have now without a builder, and this would allow us to add more fields without bumping to `V2Record` immediately. `V1Log` would have a blanket impl like this in `log`: ```rust #[doc(hidden)] impl<T: Log> V1Log for T { fn log(&self, record: V1Record) { <Self as Log>>::log(&LogRecord { inner: record }); } } ``` For the next version of `log` which brings an incompatible `LogRecord` format, `log_impl` would introduce `V2Log` and `V2Record` to match, and would `impl From<V1Record> for V2Record` and vice versa. I would name `V1` rather than `V0_3` just because I wouldn't expect every single `log` crate release to need a new, incompatible record version. Internally, `log_impl` would store an enum `VersionedLog` which could be a box of any of the `V*Log` traits, and would convert records accordingly when logged. The `log_impl` would provide one other feature, a set of macros `if_trace_enabled!()` through `if_error_enabled!()`, which would be controlled through `max_level_*` and `release_max_level_*` crate features, similar to `log`'s current ones. These macros would then be used in `log` instead of the features, and the `log` features would simply forward to depending on the `log_impl` ones. There would be overhead of converting records if the `log` macro version and logger version don't match, but I believe a scheme like this would fully support both public breaking API changes in `log`, and breaking changes in how `LogRecord` is internally structured. If we don't implement any breaking changes in `LogRecord`s format, we could even just have `log_impl` with `V1*` always, and there would be no overhead. Unresolved question with this idea: v0.3 takes in `Box<Log>`, which I don't believe can be converted to `Box<V1Log>`, even if a blanket implementation exists. Might make this unreasonable.

brson commented

2017-06-02 21:48:16 -05:00

(Migrated from github.com)

I'm not clear on what needs to be done here. @alexcrichton @sfackler have comments on @daboross's comments?

daboross commented

2017-06-02 22:42:10 -05:00

(Migrated from github.com)

I'll implement what I described if it is deemed an alright thing.

The one difference may need to be that log just re-exports log_inner::V1Log as Log in order to not need double boxing but I'm not sure how this can be shown well in documentation... (without having users see the log_inner implementation detail of log).

I'll implement what I described if it is deemed an alright thing. The one difference may need to be that log just re-exports log_inner::V1Log as Log in order to not need double boxing but I'm not sure how this can be shown well in documentation... (without having users see the log_inner implementation detail of log).

sfackler commented

2017-06-02 22:47:47 -05:00

(Migrated from github.com)

I think it'll need to be a bit weirder than that because we're getting rid of shutdown in 0.4. One possible route is that 0.3 depends on 0.4 and forwards log events over to it. We'd then have a wrapping shim in 0.3 that would handle shutdown if you registered a logger through it.

daboross commented

2017-06-03 03:38:56 -05:00

(Migrated from github.com)

We'll want 0.4 to be able to log events in a 0.3 logger too though, right?

With an inner/backend crate though, what if it just took (fn(&Metadata) -> bool, fn(&Record)) instead of Box<Log>? That way we could have both log 0.3 and log 0.4 have separate ways of storing the Log, and then we'd be able to retain support for shutdown in log 0.3 while removing it in 0.4. Both versions would just register their specific function to access their global log storage when they register a logger.

I think passing just an fn pair to the backend might also solve the issue of having to double-box things in order to have a nice API. This would involve another indirect dynamic call, but I'm thinking that's a cost we could live with?

We'll want 0.4 to be able to log events in a 0.3 logger too though, right? With an inner/backend crate though, what if it just took `(fn(&Metadata) -> bool, fn(&Record))` instead of `Box<Log>`? That way we could have both `log 0.3` and `log 0.4` have separate ways of storing the `Log`, and then we'd be able to retain support for shutdown in log 0.3 while removing it in 0.4. Both versions would just register their specific function to access their global log storage when they register a logger. I think passing just an `fn` pair to the backend might also solve the issue of having to double-box things in order to have a nice API. This would involve another indirect dynamic call, but I'm thinking that's a cost we could live with?

sfackler commented

2017-06-03 10:42:17 -05:00

(Migrated from github.com)

Yeah - this would handle that - an 0.3 logger is turned into an 0.4 logger with some shim to handle shutdown.

I'd also like to avoid allocating on the log path.

Yeah - this would handle that - an 0.3 logger is turned into an 0.4 logger with some shim to handle shutdown. I'd also like to avoid allocating on the log path.

daboross commented

2017-06-03 13:12:35 -05:00

(Migrated from github.com)

Definitely agree here with avoiding allocating.

I still think having a shared backend would be a more long-term solution, as then we wouldn't have to update 0.3 and 0.4 both to depend on 1.0 when we release it.

Is the benefit of having 0.3 depend on 0.4 just less code complexity, or is there another reason as well? I mean having less duplication in the project structure is valid, I just didn't think of it as too much of a concern.

Definitely agree here with avoiding allocating. I still think having a shared backend would be a more long-term solution, as then we wouldn't have to update 0.3 and 0.4 both to depend on 1.0 when we release it. Is the benefit of having 0.3 depend on 0.4 just less code complexity, or is there another reason as well? I mean having less duplication in the project structure is valid, I just didn't think of it as too much of a concern.

sfackler commented

2017-06-03 14:50:50 -05:00

(Migrated from github.com)

It seems pretty reasonable to me to trade way less code complexity for making two commits when we cut 1.0.

daboross commented

2017-06-07 18:38:43 -05:00

(Migrated from github.com)

Alright! I can get behind that.

sfackler commented

2017-08-13 00:12:26 -05:00

(Migrated from github.com)

This commit should do what we need: github.com/sfackler/log@55549a34b1.

This commit should do what we need: https://github.com/sfackler/log/commit/55549a34b18fc6a92d03d02d7c2ba25577ee9dc9.

👍 1

KodrAus commented

2017-09-24 16:32:17 -05:00

(Migrated from github.com)

Is there anything more we need to do here @sfackler? Do we just need to do a bump of 0.3 when it comes time to release 0.4?

Is there anything more we need to do here @sfackler? Do we just need to do a bump of `0.3` when it comes time to release `0.4`?

sfackler commented

2017-09-24 16:47:17 -05:00

(Migrated from github.com)

Nope, should be good to go.

👍 1

sfackler commented

2017-10-13 23:08:18 -05:00

(Migrated from github.com)

I just noticed one caveat: log 0.4 requires Rust 1.16.0+. Otherwise you run into this:

error: borrowed value does not live long enough                                                                                                                                                                                                                                    
   --> src/lib.rs:734:36                                                                                                                                                                                                                                                           
    |                                                                                                                                                                                                                                                                              
734 |                 args: format_args!(""),                                                                                                                                                                                                                                      
    |                                    ^^ does not live long enough                                                                                                                                                                                                              
...                                                                                                                                                                                                                                                                                
741 |     }                                                                                                                                                                                                                                                                        
    |     - temporary value only lives until here                                                                                                                                                                                                                                  
    |                                                                                                                                                                                                                                                                              
note: borrowed value must be valid for the lifetime 'a as defined on the body at 731:38...                                                                                                                                                                                         
   --> src/lib.rs:731:39                                                                                                                                                                                                                                                           
    |                                                                                                                                                                                                                                                                              
731 |     pub fn new() -> RecordBuilder<'a> {                                                                                                                                                                                                                                      
    |                                       ^

Since 0.3 will depend on 0.4, this adds the same requirement there as well. Are we okay with that?

I just noticed one caveat: log 0.4 requires Rust 1.16.0+. Otherwise you run into this: ``` error: borrowed value does not live long enough --> src/lib.rs:734:36 | 734 | args: format_args!(""), | ^^ does not live long enough ... 741 | } | - temporary value only lives until here | note: borrowed value must be valid for the lifetime 'a as defined on the body at 731:38... --> src/lib.rs:731:39 | 731 | pub fn new() -> RecordBuilder<'a> { | ^ ``` Since 0.3 will depend on 0.4, this adds the same requirement there as well. Are we okay with that?

alexcrichton commented

2017-10-14 09:29:40 -05:00

(Migrated from github.com)

This seems fine to me, the current stable is 1.21 and that's 5 versions back

This seems fine to me, the current stable is 1.21 and that's *5* versions back

👍 2

daboross commented

2017-10-14 13:24:56 -05:00

(Migrated from github.com)

That's still a compatibility break in a minor version for a very widely used crate though, right? I'm sure some of the 1120 reverse dependencies have a guarantee on their rust version support - even if it is a required upgrade to a 7-month-old compiler.

That's still a compatibility break in a minor version for a very widely used crate though, right? I'm sure _some_ of the 1120 reverse dependencies have a guarantee on their rust version support - even if it is a required upgrade to a 7-month-old compiler.

sfackler commented

2017-10-14 14:34:26 -05:00

(Migrated from github.com)

@daboross what's the alternative?

daboross commented

2017-10-15 12:05:18 -05:00

(Migrated from github.com)

@sfackler you're right there. I was trying to think of a way to fix this, but there isn't really a reasonable one. Restricting log 0.4 to earlier versions of rust wouldn't be great at all, and it is an old version already that 0.3 is requiring an upgrade to.

dhardy commented

2017-12-27 06:37:54 -06:00

(Migrated from github.com)

So rand just got hit by the log 0.3 depends on log 0.4 thing @sfackler mentioned above (CI pins rustc 1.15). I guess we can solve that (either by pinning log <= 0.3.8 or updating to rustc 1.16), but in general it's not cool having 0.3.9 depend on 0.4.0 (especially since I had to search around to find out why; put a note in the 0.3.x readme maybe?).

Ok, I understand that you want to avoid Cargo linking two copies of log into peoples dependency graphs, but what you've done automatically updates people with an explicit dependency on log ^0.3 to use 0.4. Two evils it seems.

So `rand` [just got hit](https://travis-ci.org/rust-lang-nursery/rand/jobs/322149567) by the `log 0.3` depends on `log 0.4` thing @sfackler mentioned above (CI pins rustc 1.15). I guess we can solve that (either by pinning log <= 0.3.8 or updating to rustc 1.16), but in general it's not cool having 0.3.9 depend on 0.4.0 (especially since I had to search around to find out why; put a note in the 0.3.x readme maybe?). Ok, I understand that you want to avoid Cargo linking two copies of `log` into peoples dependency graphs, but what you've done automatically updates people with an explicit dependency on log `^0.3` to use `0.4`. Two evils it seems.

sfackler commented

2017-12-27 10:16:32 -06:00

(Migrated from github.com)

I understand that you want to avoid Cargo linking two copies of log into peoples dependency graphs

That is not what I want to avoid! What I want to avoid is totally bifurcating the log messages of the entire crate ecosystem for an arbitrary amount of time, with the 0.4 messages disappearing into the ether if a crate is using a logger implementation which uses 0.3 and vice versa.

One one side we have the cost of bumping a number in a yaml file in rand's repository, and in the other we have massive ecosystem trauma. It seems like a pretty straightforward choice to me?

> I understand that you want to avoid Cargo linking two copies of log into peoples dependency graphs That is not what I want to avoid! What I want to avoid is totally bifurcating the log messages of the entire crate ecosystem for an arbitrary amount of time, with the 0.4 messages disappearing into the ether if a crate is using a logger implementation which uses 0.3 and vice versa. One one side we have the cost of bumping a number in a yaml file in rand's repository, and in the other we have massive ecosystem trauma. It seems like a pretty straightforward choice to me?

dtolnay commented

2017-12-27 10:26:53 -06:00

(Migrated from github.com)

Looks like it would be possible to add support for 1.15 by tossing in a transmute.

format_args!("")

// expands to
core::fmt::Arguments::new_v1(&[], &[])

// on 1.15 the &[] do not live as long as 'static, but it should be fine right?
unsafe { transmute(format_args!("")) }

Looks like it would be possible to add support for 1.15 by tossing in a transmute. ```rust format_args!("") // expands to core::fmt::Arguments::new_v1(&[], &[]) // on 1.15 the &[] do not live as long as 'static, but it should be fine right? unsafe { transmute(format_args!("")) } ```

👍 1

dhardy commented

2017-12-27 10:38:24 -06:00

(Migrated from github.com)

What you're saying @sfackler is that log 0.3 is essentially dead, any crate using log must be using 0.4, and as a side-effect Rustc < 1.16 is also dead (for anything using log, which is a lot of things). I don't really care about Rustc 1.15 but it seems like there should be some kind of warning and advisory handling (e.g. I just pinned log <= 0.3.8 which it sounds like is exactly what you don't want people to do; luckily it's only a dev-dependency so doesn't affect anything else).

I wouldn't normally advise caring about old Rust versions without known requirements, but in this case I think using the workaround posted would make sense, just because so many people use log (if it's not already too late).

What you're saying @sfackler is that log 0.3 is essentially dead, any crate using log must be using 0.4, and as a side-effect Rustc < 1.16 is also dead (for anything using log, which is a lot of things). I don't really care about Rustc 1.15 but it seems like there should be some kind of warning and advisory handling (e.g. I just pinned log <= 0.3.8 which it sounds like is exactly what you *don't* want people to do; luckily it's only a dev-dependency so doesn't affect anything else). I wouldn't normally advise caring about old Rust versions without known requirements, but in this case I think using the workaround posted would make sense, just because so many people use `log` (if it's not already too late).

sfackler commented

2017-12-27 13:58:38 -06:00

(Migrated from github.com)

@dtolnay I'd be happy to do that, but I'd want to first figure out what changed in 1.16 to make it compile. If it was just the borrow checker becoming a bit smarter then we're fine, but if it was due to a change in e.g. format_args's internal guts then we could run into memory safety issues.

"Let's just transmute, what's the worst that could happen" is not an approach I'm on board with.

@dhardy in what library is it not the case that the 0.x series is dead once 0.(x-1) comes out and crates depending on it should move forward? If you don't care about 1.15, then why did you pin the log version instead of bumping to 1.16?

@dtolnay I'd be happy to do that, but I'd want to first figure out what changed in 1.16 to make it compile. If it was just the borrow checker becoming a bit smarter then we're fine, but if it was due to a change in e.g. format_args's internal guts then we could run into memory safety issues. "Let's just transmute, what's the worst that could happen" is not an approach I'm on board with. @dhardy in what library is it *not* the case that the 0.x series is dead once 0.(x-1) comes out and crates depending on it should move forward? If you don't care about 1.15, then why did you pin the log version instead of bumping to 1.16?

dtolnay commented

2017-12-27 14:11:56 -06:00

(Migrated from github.com)

It was the borrow checker becoming smarter. This is what was newly allowed in 1.16.0:

struct Argument;

fn main() {
    let _: &'static [Argument] = &match () { () => [] };
}

The format_args!("") contains a slice with some data pointer and zero length. All we do with it in std::fmt is iterate over the arguments, so casting the zero-length slice to a longer lifetime seems fine.

It was the borrow checker becoming smarter. This is what was newly allowed in 1.16.0: ```rust struct Argument; fn main() { let _: &'static [Argument] = &match () { () => [] }; } ``` The `format_args!("")` contains a slice with some data pointer and zero length. All we do with it in std::fmt is iterate over the arguments, so casting the zero-length slice to a longer lifetime seems fine.

cardoe commented

2018-02-14 00:10:24 -06:00

(Migrated from github.com)

I've requested this in a number of other nursery crates but can we just add a minimum version in the Travis file and test against that. When it needs to be bumped then it needs to be bumped but it will at least let people know "uh oh something was hit" and others can splunk through the history to find what works with what.

sfackler commented

2018-02-14 00:21:21 -06:00

(Migrated from github.com)

@cardoe we already do that: https://github.com/rust-lang-nursery/log/blob/master/.travis.yml#L4

cardoe commented

2018-02-14 00:24:12 -06:00

(Migrated from github.com)

Yeah I was responding to an issue on a crate I support for Rust 1.15.1 that still had log 0.3 in it.

Rows
Columns

Ensure 0.3 works with the next major release #148