-
Notifications
You must be signed in to change notification settings - Fork 697
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
subscriber: update time crate to 0.3.18 #2550
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, I'm happy to increase the minimum time
version.
There are notes in the docs here
tracing/tracing-subscriber/src/fmt/time/time_crate.rs
Lines 9 to 20 in b28c935
/// <div class="example-wrap" style="display:inline-block"> | |
/// <pre class="compile_fail" style="white-space:normal;font:inherit;"> | |
/// <strong>Warning</strong>: The <a href = "https://docs.rs/time/0.3/time/"><code>time</code> | |
/// crate</a> must be compiled with <code>--cfg unsound_local_offset</code> in order to use | |
/// local timestamps. When this cfg is not enabled, local timestamps cannot be recorded, and | |
/// events will be logged without timestamps. | |
/// | |
/// Alternatively, [`OffsetTime`] can log with a local offset if it is initialized early. | |
/// | |
/// See the <a href="https://docs.rs/time/0.3.4/time/#feature-flags"><code>time</code> | |
/// documentation</a> for more details. | |
/// </pre></div> |
tracing/tracing-subscriber/src/fmt/time/time_crate.rs
Lines 99 to 110 in b28c935
/// <div class="example-wrap" style="display:inline-block"> | |
/// <pre class="compile_fail" style="white-space:normal;font:inherit;"> | |
/// <strong>Warning</strong>: The <a href = "https://docs.rs/time/0.3/time/"> | |
/// <code>time</code> crate</a> must be compiled with <code>--cfg | |
/// unsound_local_offset</code> in order to use local timestamps. When this | |
/// cfg is not enabled, local timestamps cannot be recorded, and | |
/// events will be logged without timestamps. | |
/// | |
/// See the <a href="https://docs.rs/time/0.3.4/time/#feature-flags"> | |
/// <code>time</code> documentation</a> for more details. | |
/// </pre></div> | |
/// |
unsound_local_offset
cfg. If we're going to upgrade to a version of time
where this is no longer required, we should also remove these notes from the documentation, so that users don't think they still need to enable that flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, thanks!
Thanks for reviewing and approving! There are some failures in CI. I'm not sure if they are related to this change (my guess is not, except the MSRV failures). Should I debug these CI errors? |
It looks like a majority of the failing checks aren't related to this change. I you have the time to dig into them, that would be appreciated, but let's try and land any changes that fix stuff unrelated to this change in separate PRs please? |
Sure. I started with Clippy warnings: #2552 . |
Should this PR be pointing at the 0.1 branch? Has this been fixed on master? |
It's the same on master. I submitted the change for 0.1.x as that's what I'm using ;-). I don't know what's the process of re-applying / cherrypicking a fix, but I can submit it again for master after this PR lands successfully. |
In general, we prefer to land new changes on master and then the maintainers will backport them to v0.1.x. If you have the time and energy to cherry-pick your own PR, though, that would be much appreciated! |
Yeah, what Eliza said. It's a lot easier for us to cherry-pick in a single direction. If you're using this, I'd be happy to prioritize a cherry-pick quickly. I'd suggest using |
I have changed the base to |
e1e3627
to
61bf69e
Compare
I've rebased the diff against |
Bump up the version of the `time` crate so that we don't need to build with `--cfg unsound_local_offset` for using `fmt::time::LocalTime`.
61bf69e
to
d1e5adf
Compare
Rebased new changes again to the latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, thanks!
CI MSRV checked failed with rust 1.56.0. I think it's because I guess we can't update |
Whoops, I didn't see this comment when this PR was merged. That's unfortunate, I think we may have to revert this change to maintain our current MSRV policy. However, if we go back to specifying a more permissive dependency on |
This reverts commit 9744ec0. This change breaks MSRV compatibility, and was accidentally auto-merged due to what appears to be an issue with the CI configuration, which (apparently) doesn't require the MSRV minimal-versions check runs to complete before allowing a branch to merge. We'll have to solve the original issue here through documentation for now. See [this comment] for details. [this comment]: #2550 (comment)
Did you mean we revert and add
and also document like you said? (I never used EDIT: well, even the original |
This reverts commit 9744ec0. This change breaks MSRV compatibility, and was accidentally auto-merged due to what appears to be an issue with the CI configuration, which (apparently) doesn't require the MSRV minimal-versions check runs to complete before allowing a branch to merge. We'll have to solve the original issue here through documentation for now. See [this comment] for details. [this comment]: #2550 (comment)
This reverts commit 9744ec0. This change breaks MSRV compatibility, and was accidentally auto-merged due to what appears to be an issue with the CI configuration, which (apparently) doesn't require the MSRV minimal-versions check runs to complete before allowing a branch to merge. We'll have to solve the original issue here through documentation for now. See [this comment] for details. [this comment]: #2550 (comment)
Motivation
As I asked earlier in discussions, I'm trying to use
LocalTime
intracing-subscriber::fmt
, i.e. something like this:However, with the
time
crate 0.3.2, I need to build with--cfg unsound_local_offset
, which is not desirable. In fact,time
crate has removed this requirement since v0.3.18. Hence I propose to updatetime
crate version in this branch.Solution
Update
time
crate to version0.3.18
. I tried locally to useLocalTime
after updating, there is no problem without--cfg unsound_local_offset
.