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

Absorb the time crate #286

Closed
wants to merge 8 commits into from
Closed

Absorb the time crate #286

wants to merge 8 commits into from

Conversation

jjpe
Copy link
Contributor

@jjpe jjpe commented Nov 2, 2018

I've discussed this with one of the time crate maintainers (Alex Chrichton), and
with his blessing I've purged the dependency on the deprecated time
crate in favor of folding its code into chrono. See issue #285.

This enables further refactoring.

@quodlibetor
Copy link
Contributor

Hey, Awesome! Thank you and sorry for the delay.

Based on current thinking I would like this to be another crate (chrono-time) inside the chrono organization, that prevents making chrono into even more of a monolith, and would make it possible for us to keep git history etc. If I created that would you be interested in doing some of the integration work?

@jjpe
Copy link
Contributor Author

jjpe commented Nov 27, 2018

I merged the contents into chrono because I figured a lot of the code could ultimately be stripped away (that's the impression I got at the time, at least).

But if you'd have it be a separate crate then sure I'd be willing to do the integration work, with the only caveat being that it would have to wait until middle December due to work demands. It would also be nice to have a way to communicate slightly more directly when I start with that, as you're bound to know much more about chrono than I.

If that's fine with you, then let's do it :)

@quodlibetor
Copy link
Contributor

There is a chrono gitter.im channel linked to from the main README, you can chat in the public channel or DM me! I try to be online as much as possible.

jjpe and others added 8 commits December 26, 2019 20:58
I've discussed this with one of the maintainers (Alex Chrichton), and
with his blessing I've purged the dependency on the deprecated time
crate in favor of folding its code into chrono.

This enables further refactoring.
The little buggers snuck through.
@fenhl
Copy link

fenhl commented May 6, 2020

What's the status on this?

@jedisct1
Copy link

jedisct1 commented May 6, 2020

I would also like to see this being merged.

@bpfoley
Copy link

bpfoley commented May 21, 2020

I'd like to see this merged too and would be happy to commit some time to working on it.

@CryZe
Copy link
Contributor

CryZe commented May 22, 2020

It might make more sense to look into using time 0.2 instead.

@bpfoley
Copy link

bpfoley commented May 22, 2020

It might make more sense to look into using time 0.2 instead.

Doesn't #400 conclude not to update to time 0.2.x and to instead absorb the time crate?

quodlibetor added a commit to quodlibetor/rust-chrono that referenced this pull request Jul 1, 2020
It causes a different _increased_ API when features are _disabled_.

It'll be re-added when chronotope#286 is finished
quodlibetor added a commit that referenced this pull request Jul 1, 2020
It causes a different _increased_ API when features are _disabled_.

It'll be re-added when #286 is finished
benesch added a commit to benesch/chrono that referenced this pull request Sep 23, 2020
Absorb just enough of the time crate that it is no longer required for
the clock feature. v0.1 of the time crate is long deprecated, and v0.2 of
the crate is a complete rewrite.

Vendoring v0.1 allows chrono to control its own destiny. It also means
that downstream users that have upgraded to the time v0.2 ecosystem do
not wind up with both time v0.1 and v0.2 in the dependency tree.

Even with this patch, the dependency on the old time crate remains by
default for backwards compatibility. Specifically, the
`chrono::Duration` type is a re-export of the `time::Duration` type when
the `oldtime` feature is enabled, as it is by default. The intent is
that the `oldtime` feature will be removed when chrono v0.5 is released.

Supersedes chronotope#286.
Fixes chronotope#400.
benesch added a commit to benesch/chrono that referenced this pull request Sep 23, 2020
Absorb just enough of the time crate that it is no longer required for
the clock feature. v0.1 of the time crate is long deprecated, and v0.2 of
the crate is a complete rewrite.

Vendoring v0.1 allows chrono to control its own destiny. It also means
that downstream users that have upgraded to the time v0.2 ecosystem do
not wind up with both time v0.1 and v0.2 in the dependency tree.

Even with this patch, the dependency on the old time crate remains by
default for backwards compatibility. Specifically, the
`chrono::Duration` type is a re-export of the `time::Duration` type when
the `oldtime` feature is enabled, as it is by default. The intent is
that the `oldtime` feature will be removed when chrono v0.5 is released.

Supersedes chronotope#286.
Fixes chronotope#400.
benesch added a commit to benesch/chrono that referenced this pull request Sep 23, 2020
Absorb just enough of the time crate that it is no longer required for
the clock feature. v0.1 of the time crate is long deprecated, and v0.2 of
the crate is a complete rewrite.

Vendoring v0.1 allows chrono to control its own destiny. It also means
that downstream users that have upgraded to the time v0.2 ecosystem do
not wind up with both time v0.1 and v0.2 in the dependency tree.

Even with this patch, the dependency on the old time crate remains by
default for backwards compatibility. Specifically, the
`chrono::Duration` type is a re-export of the `time::Duration` type when
the `oldtime` feature is enabled, as it is by default. The intent is
that the `oldtime` feature will be removed when chrono v0.5 is released.

Supersedes chronotope#286.
Fixes chronotope#400.
benesch added a commit to benesch/chrono that referenced this pull request Sep 23, 2020
Absorb just enough of the time crate that it is no longer required for
the clock feature. v0.1 of the time crate is long deprecated, and v0.2 of
the crate is a complete rewrite.

Vendoring v0.1 allows chrono to control its own destiny. It also means
that downstream users that have upgraded to the time v0.2 ecosystem do
not wind up with both time v0.1 and v0.2 in the dependency tree.

Even with this patch, the dependency on the old time crate remains by
default for backwards compatibility. Specifically, the
`chrono::Duration` type is a re-export of the `time::Duration` type when
the `oldtime` feature is enabled, as it is by default. The intent is
that the `oldtime` feature will be removed when chrono v0.5 is released.

Supersedes chronotope#286.
Fixes chronotope#400.
benesch added a commit to benesch/chrono that referenced this pull request Sep 23, 2020
Absorb just enough of the time crate that it is no longer required for
the clock feature. v0.1 of the time crate is long deprecated, and v0.2 of
the crate is a complete rewrite.

Vendoring v0.1 allows chrono to control its own destiny. It also means
that downstream users that have upgraded to the time v0.2 ecosystem do
not wind up with both time v0.1 and v0.2 in the dependency tree.

Even with this patch, the dependency on the old time crate remains by
default for backwards compatibility. Specifically, the
`chrono::Duration` type is a re-export of the `time::Duration` type when
the `oldtime` feature is enabled, as it is by default. The intent is
that the `oldtime` feature will be removed when chrono v0.5 is released.

Supersedes chronotope#286.
Fixes chronotope#400.
benesch added a commit to benesch/chrono that referenced this pull request Sep 23, 2020
Absorb just enough of the time crate that it is no longer required for
the clock feature. v0.1 of the time crate is long deprecated, and v0.2 of
the crate is a complete rewrite.

Vendoring v0.1 allows chrono to control its own destiny. It also means
that downstream users that have upgraded to the time v0.2 ecosystem do
not wind up with both time v0.1 and v0.2 in the dependency tree.

Even with this patch, the dependency on the old time crate remains by
default for backwards compatibility. Specifically, the
`chrono::Duration` type is a re-export of the `time::Duration` type when
the `oldtime` feature is enabled, as it is by default. The intent is
that the `oldtime` feature will be removed when chrono v0.5 is released.

Supersedes chronotope#286.
Fixes chronotope#400.
benesch added a commit to benesch/chrono that referenced this pull request Sep 23, 2020
Absorb just enough of the time crate that it is no longer required for
the clock feature. v0.1 of the time crate is long deprecated, and v0.2 of
the crate is a complete rewrite.

Vendoring v0.1 allows chrono to control its own destiny. It also means
that downstream users that have upgraded to the time v0.2 ecosystem do
not wind up with both time v0.1 and v0.2 in the dependency tree.

Even with this patch, the dependency on the old time crate remains by
default for backwards compatibility. Specifically, the
`chrono::Duration` type is a re-export of the `time::Duration` type when
the `oldtime` feature is enabled, as it is by default. The intent is
that the `oldtime` feature will be removed when chrono v0.5 is released.

Supersedes chronotope#286.
Fixes chronotope#400.
benesch added a commit to benesch/chrono that referenced this pull request Sep 23, 2020
Absorb just enough of the time crate that it is no longer required for
the clock feature. v0.1 of the time crate is long deprecated, and v0.2 of
the crate is a complete rewrite.

Vendoring v0.1 allows chrono to control its own destiny. It also means
that downstream users that have upgraded to the time v0.2 ecosystem do
not wind up with both time v0.1 and v0.2 in the dependency tree.

Even with this patch, the dependency on the old time crate remains by
default for backwards compatibility. Specifically, the
`chrono::Duration` type is a re-export of the `time::Duration` type when
the `oldtime` feature is enabled, as it is by default. The intent is
that the `oldtime` feature will be removed when chrono v0.5 is released.

Supersedes chronotope#286.
Fixes chronotope#400.
quodlibetor pushed a commit that referenced this pull request Sep 25, 2020
Absorb just enough of the time crate that it is no longer required for
the clock feature. v0.1 of the time crate is long deprecated, and v0.2 of
the crate is a complete rewrite.

Vendoring v0.1 allows chrono to control its own destiny. It also means
that downstream users that have upgraded to the time v0.2 ecosystem do
not wind up with both time v0.1 and v0.2 in the dependency tree.

Even with this patch, the dependency on the old time crate remains by
default for backwards compatibility. Specifically, the
`chrono::Duration` type is a re-export of the `time::Duration` type when
the `oldtime` feature is enabled, as it is by default. The intent is
that the `oldtime` feature will be removed when chrono v0.5 is released.

Supersedes #286.
Fixes #400.
@quodlibetor
Copy link
Contributor

The main goal of this was implemented by #478 , so I'm going to close it. If other features of the time crate are necessary I'm open to considering adding them to chrono.

@jhpratt
Copy link

jhpratt commented Sep 25, 2020

@quodlibetor Keep in mind that the PR you referenced requires a feature flag. Moving existing behavior behind a feature flag is not back-compatible, even if that flag is on by default. That change will almost certainly result in breakage somewhere in what is otherwise a semver-compatible change.

@quodlibetor
Copy link
Contributor

yes, folks who have no-default-features turned on and who were relying on interop between time::Duration and chrono will have to add a feature, the same would have been true with this PR. So far folks have not been too mad about that being the backcompat story in chrono, compared to all the other ways that it strives to maintain backwards-compatibility. If it causes serious problems for folks I can yank 0.4.16 and think through some other way forward.

@jhpratt
Copy link

jhpratt commented Sep 25, 2020

IMO that's still not okay, as you are knowingly pushing out a breaking change. It was (and is) clearly documented that the Duration was the same type, but that's no longer he case. But that's your decision to make, of course. I just know that a number of people have been bit by changes like this in the past.

@CryZe
Copy link
Contributor

CryZe commented Sep 25, 2020

Doesn't seem like chrono even builds by itself:

 Compiling chrono v0.4.16
error[E0433]: failed to resolve: use of undeclared type or module `oldtime`
   --> C:\Users\Christopher Serr\.cargo\registry\src\github.com-1ecc6299db9ec823\chrono-0.4.16\src\offset\local.rs:155:27       
    |
155 |         local -= oldtime::Duration::seconds(offset.local_minus_utc() as i64);
    |                           ^^^^^^^^ not found in `oldtime`
    |
help: consider importing one of these items
    |
9   | use Duration;
    |
9   | use wasm_bindgen::__rt::core::time::Duration;
    |

@CryZe
Copy link
Contributor

CryZe commented Sep 25, 2020

Seems like wasm specific code in chrono here seems to unconditionally depend on oldtime and I'm not using default-features, so yeah exactly what @jhpratt described broke my crate. Though it would probably fine if the wasm code would be fixed to not depend on that module.

@quodlibetor
Copy link
Contributor

Hm yeah I can fix that. Could you describe exactly what features you're building with? I thought that I tested every possible combination of features but I see that can't be the case.

@quodlibetor
Copy link
Contributor

Seems like wasm specific code in chrono here seems to unconditionally depend on oldtime

This isn't actually the case, we have a shim module in the root of chrono that is called oldtime to match the crate import for when the feature is enabled. This seems like some weird namespacing issue.

quodlibetor added a commit to quodlibetor/rust-chrono that referenced this pull request Sep 25, 2020
wasm only works on new-enough rust that we can unconditionally use the crate
keyword in that method, so let's try that.

chronotope#286 (comment)
quodlibetor added a commit to quodlibetor/rust-chrono that referenced this pull request Sep 25, 2020
pickfire pushed a commit to pickfire/chrono that referenced this pull request Jan 22, 2022
Absorb just enough of the time crate that it is no longer required for
the clock feature. v0.1 of the time crate is long deprecated, and v0.2 of
the crate is a complete rewrite.

Vendoring v0.1 allows chrono to control its own destiny. It also means
that downstream users that have upgraded to the time v0.2 ecosystem do
not wind up with both time v0.1 and v0.2 in the dependency tree.

Even with this patch, the dependency on the old time crate remains by
default for backwards compatibility. Specifically, the
`chrono::Duration` type is a re-export of the `time::Duration` type when
the `oldtime` feature is enabled, as it is by default. The intent is
that the `oldtime` feature will be removed when chrono v0.5 is released.

Supersedes chronotope#286.
Fixes chronotope#400.
botahamec pushed a commit to botahamec/chrono that referenced this pull request May 26, 2022
Absorb just enough of the time crate that it is no longer required for
the clock feature. v0.1 of the time crate is long deprecated, and v0.2 of
the crate is a complete rewrite.

Vendoring v0.1 allows chrono to control its own destiny. It also means
that downstream users that have upgraded to the time v0.2 ecosystem do
not wind up with both time v0.1 and v0.2 in the dependency tree.

Even with this patch, the dependency on the old time crate remains by
default for backwards compatibility. Specifically, the
`chrono::Duration` type is a re-export of the `time::Duration` type when
the `oldtime` feature is enabled, as it is by default. The intent is
that the `oldtime` feature will be removed when chrono v0.5 is released.

Supersedes chronotope#286.
Fixes chronotope#400.
pickfire pushed a commit to pickfire/chrono that referenced this pull request Jul 5, 2022
Absorb just enough of the time crate that it is no longer required for
the clock feature. v0.1 of the time crate is long deprecated, and v0.2 of
the crate is a complete rewrite.

Vendoring v0.1 allows chrono to control its own destiny. It also means
that downstream users that have upgraded to the time v0.2 ecosystem do
not wind up with both time v0.1 and v0.2 in the dependency tree.

Even with this patch, the dependency on the old time crate remains by
default for backwards compatibility. Specifically, the
`chrono::Duration` type is a re-export of the `time::Duration` type when
the `oldtime` feature is enabled, as it is by default. The intent is
that the `oldtime` feature will be removed when chrono v0.5 is released.

Supersedes chronotope#286.
Fixes chronotope#400.
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.

7 participants