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

Don't require time crate for clock feature #478

Merged
merged 1 commit into from
Sep 25, 2020

Conversation

benesch
Copy link
Contributor

@benesch benesch commented Sep 23, 2020

@quodlibetor what do you think about this? It's a much more targeted version of #400 that only absorbs the bare minimum of time v0.1 that chrono requires.

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.

Thanks for contributing to chrono!

  • Have you added yourself and the change to the changelog? (Don't worry
    about adding the PR number)
  • If this pull request fixes a bug, does it add a test that verifies that
    we can't reintroduce it?

@benesch benesch force-pushed the no-time branch 5 times, most recently from 59d7a7c to 31b154c Compare September 23, 2020 04:51
Cargo.toml Outdated
Comment on lines 50 to 52
[target.'cfg(windows)'.dependencies]
winapi = { version = "0.3.0", features = ["std", "minwinbase", "minwindef", "timezoneapi"] }

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose cargo's lack of target-specific optional dependencies means there's no good way to make this optional?

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 mean, does it work to just mark it optional? Did that, and that... seems to work ok!

}

pub fn time_to_local_tm(sec: i64, tm: &mut Tm) {
// FIXME: Add timezone logic
Copy link
Contributor

Choose a reason for hiding this comment

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

note to self: double check that chrono's patch of time's handling of this is correct.

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'm sure there's a way to simplify by pushing some of the conditional logic in local.rs into here, but the conditionals are complicated enough (wasm32 vs wasi vs wasm-bindgen vs ...) that I was afraid I'd break something if I did that. I'm 99% confident that I've preserved the exact behavior of the time crate on WASM platforms here, so in theory that means that chrono is exactly as buggy (hopefully not at all buggy!) as before.

Copy link
Contributor

Choose a reason for hiding this comment

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

right there were some bugs around something related to this that only manifest when combined with chrono-tz that were fixed last year, I just want to verify that they catch all uses of this. I agree that this should be exactly as buggy as before.

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.
@jhpratt
Copy link

jhpratt commented Sep 25, 2020

Keep in mind that adding a new feature flags, even if default, breaks back-compatibility. As such, this change will likely break something downstream.

@benesch
Copy link
Contributor Author

benesch commented Sep 26, 2020

Backwards compatibility is always a spectrum. In theory an internal refactoring could be considered to break backwards compatibility because backtraces will now generate different stack frames. Obviously that is on the extreme end of what most people would consider within the realm of backwards compatibility, but it is nonetheless a real problem that I've run into.

I don't think there was a particularly good alternative here. Did you have something you'd like to propose, @jhpratt? Bumping chrono to v0.5 would fork the ecosystem in a painful way, and in my opinion that should wait until there is good reason to do so, like fixing various API warts. (I'm not familiar with what those warts are, specifically, but @quodlibetor assures me they exist.)

In any case, we can watch Dependabot to get a sense for breakage: https://dependabot.com/compatibility-score/?dependency-name=chrono&package-manager=cargo&previous-version=0.4.15&new-version=0.4.16. So far things look good, but the sample size is not particularly large yet.

@benesch benesch deleted the no-time branch September 26, 2020 01:39
@kaimast
Copy link

kaimast commented Sep 26, 2020

Just FYI: This change broke building chrono on the x86_64-fortanix-unknown-sgx target.

I get this error with >=0.4.16:

error[E0433]: failed to resolve: use of undeclared crate or module `inner`
  --> /home/kai/.cargo/registry/src/github.com-1ecc6299db9ec823/chrono-0.4.16/src/sys.rs:64:9
   |
64 |         inner::time_to_local_tm(self.sec, &mut tm);
   |         ^^^^^ use of undeclared crate or module `inner`

error[E0433]: failed to resolve: use of undeclared crate or module `inner`
   --> /home/kai/.cargo/registry/src/github.com-1ecc6299db9ec823/chrono-0.4.16/src/sys.rs:121:18
    |
121 |             0 => inner::utc_tm_to_time(self),
    |                  ^^^^^ use of undeclared crate or module `inner`

error[E0433]: failed to resolve: use of undeclared crate or module `inner`
   --> /home/kai/.cargo/registry/src/github.com-1ecc6299db9ec823/chrono-0.4.16/src/sys.rs:122:18
    |
122 |             _ => inner::local_tm_to_time(self),
    |                  ^^^^^ use of undeclared crate or module `inner`

error: aborting due to 3 previous errors

@jhpratt
Copy link

jhpratt commented Sep 26, 2020

Dependabot relies on CI, which doesn't necessarily paint the whole picture. Case in point: 0.4.16 was broken on some targets, but this wasn't caught before it was released. I've run into this on the time crate myself, which is why my CI is now quite thorough (checking a number of targets and every possible combination of feature flags).

Backwards compatibility in Rust is that a user should be able to rely on the public API as documented without breakage. Given chrono's clear statement that Duration is the same type as time::Duration (which still exists, by the way), changing this is a breaking change: someone could have had code that relied on this fact, and it would now be broken. Placing something behind a new feature flag also breaks this promise, as the function no longer exists when using identical settings in Cargo.toml.

My "suggested alternative" is to not break things unless accompanied by a semver-breaking version bump. I fail to see the significant benefit of absorbing time 0.1 such that it's worth breaking other people's code. Waiting until 0.5 to perform this change is the responsible thing to do.

benesch added a commit to benesch/chrono that referenced this pull request Sep 26, 2020
The main crate docs made some claims about the Duration type that became
incorrect with the new oldtime feature (chronotope#478).

Per @jhpratt.
benesch added a commit to benesch/chrono that referenced this pull request Sep 26, 2020
The main crate docs made some claims about the Duration type that became
incorrect with the new oldtime feature (chronotope#478).

Per @jhpratt.
@benesch
Copy link
Contributor Author

benesch commented Sep 26, 2020

Discussion has forked a bit across PRs, so I'm going to respond to your comment #286 (comment) here as well.

It was (and is) clearly documented that the Duration was the same type, but that's no longer he case.

Thanks for pointing that out. I've opened a PR to fix that: #484.

IMO that's still not okay, as you are knowingly pushing out a breaking change.

My "suggested alternative" is to not break things unless accompanied by a semver-breaking version bump. I fail to see the significant benefit of absorbing time 0.1 such that it's worth breaking other people's code. Waiting until 0.5 to perform this change is the responsible thing to do.

This is a gentle request to not frame things as so black and white. Describing this change as "not okay" is a bit harsh. As I mentioned above, there is no clear definition of what does and does not constitute backwards compatibility. The most mature of software projects will have a document describing the precise backwards guarantees (e.g., the Go Compatibility Promise), but Chrono doesn't have anything nearly so complete. And that's okay! I think we all agree that the goal is to minimize upgrade pain for downstream users, and over time we'll learn via empirical evidence which changes were acceptable and which were not. But I'd prefer to make those sorts of decisions based on empirical evidence, not hypothetical users.

There was a bit of fallout from this change due to straight up bugs, and for that I apologize. But so far the fallout is not caused by folks needing oldtime but having default features disabled.

I think it is important to mention that releasing new semver-incompatible versions also imposes a large cost on users. Frequent breaking changes practically guarantee that downstream projects will wind up with duplicate versions of dependencies in their tree. These duplicate dependencies at best bloat binaries and at worst cause confusing and hard-to-resolve errors e.g. if you try to pass a time_v01::Duration to a function expecting a time_v02::Duration. I've been trying to remove time v0.1 from our tree at @MaterializeInc for several months (see e.g. rusqlite/rusqlite#782), and the project is still ongoing. And it sounds like I'm going to have to embark on the same quest shortly for time v0.3.

Personally I am much more concerned with minimizing the amount of work it will take the ecosystem to upgrade from version x to version y than I am with adhering to the strictest possible interpretation of "breaking change."

@jhpratt
Copy link

jhpratt commented Sep 26, 2020

As I mentioned above, there is no clear definition of what does and does not constitute backwards compatibility.

I gave a relatively solid definition in my previous comment.

The most mature of software projects will have a document describing the precise backwards guarantees

After a quick search, I found RFC 1105, which actually does define what a breaking change is (both for the standard library and crates), in addition to what's acceptable breakage in a minor version. By my reading of that document, no_default_features is not mentioned anywhere, but changing the public API in the manner that was done here (switching time::Duration to chrono::Duration, which was previously aliased) should be considered a breaking change. Whether it's acceptable is where the debate should be at, not whether it's breaking. I thought I was clear in that my description of the change as "not okay" was my opinion, and not a statement of fact. If that was not the case, I apologize.

I will note that I applaud the effort to work around the no_default_features issue by having an "old default" flag. I think it's a great way to avoid back-compatibility. However, that only works when the flag has been around — introducing it in the same release that made the change to begin with defeats the purpose (hopefully the reason for this is clear).

There was a bit of fallout from this change due to straight up bugs, and for that I apologize. But so far the fallout is not caused by folks needing oldtime but having default features disabled.

I am aware 🙂 For what it's worth, before I developed time 0.2, I used chrono on occasion, and did find myself relying on the interoperability between time::Duration and chrono::Duration. I would be quite surprised if others have not done the same.

I think it is important to mention that releasing new semver-incompatible versions also imposes a large cost on users. Frequent breaking changes practically guarantee that downstream projects will wind up with duplicate versions of dependencies in their tree.

No argument there. With LTO, duplicate versions can be eliminated to an extent, thankfully. The difference between acceptable breakage and what requires a breaking semver release is of course thin.

A big difference, in my opinion, is that changes marked semver-compatible (via the version number) would be automatically pulled in when performing cargo update, which should be a regular part of any workflow. On the contrary, one would have to perform cargo upgrade to pull in a semver-breaking change.

I've been trying to remove time v0.1 from our tree at @MaterializeInc for several months (see e.g. rusqlite/rusqlite#782), and the project is still ongoing. And it sounds like I'm going to have to embark on the same quest shortly for time v0.3.

I had an issue open on the time repo for quite a while for questions! I don't think the issue explicitly stated, but I was also very much willing to assist with upgrades. I'm a fair amount busier now, so unfortunately I can't promise as much assistance. I will help to an extent I can, though.

Time 0.3 is still a good ways off, as I've got some major changes to be made still. I would keep the version in the 0.2 series if possible, but changes like revamped formatting are wholly incompatible and infeasible to place behind a feature flag. With changes in 0.3, it will be possible to make changes like that in the future without breakage, as I'm splitting the crate to be more modular. The upgrade from 0.2 to 0.3 should also be far easier than 0.1 to 0.2 (think Rust 2015 → 2018 versus 2018 → 2021).


Don't get me wrong on any of this: I sincerely hope there is zero breakage across the ecosystem. I just felt it necessary to point out that the change could break things in case you weren't aware. The decision on what to do with that information is of course up to you!

I think I've made both my point and my intent quite clear, so I'll respectfully bow out of this discussion. If you have any questions regarding the time crate, feel free to open a discussion over on the repo!

quodlibetor pushed a commit that referenced this pull request Sep 26, 2020
The main crate docs made some claims about the Duration type that became
incorrect with the new oldtime feature (#478).

Per @jhpratt.
lopopolo added a commit to artichoke/artichoke that referenced this pull request Sep 27, 2020
See chronotope/chrono#478 which was released as chrono v0.4.16.

This PR pegs the minimum chrono to 0.4.17 to pull in a wasm fix from
chronotope/chrono#483.
mulkieran added a commit to mulkieran/stratisd that referenced this pull request Nov 5, 2021
It turns out that it is not required by the chrono crate.

It was made optional in chrono version 0.4.16, see:
chronotope/chrono#478.

Signed-off-by: mulhern <amulhern@redhat.com>
mulkieran added a commit to mulkieran/stratisd that referenced this pull request Nov 5, 2021
It turns out that it is not required by the chrono crate.

It was made optional in chrono version 0.4.16, see:
chronotope/chrono#478.

Require chrono 0.4.16 in Cargo.toml, as before that, the clock
feature pulled in the time crate, so that specifying no defaults
and precisely "clock" and "std" will still pull in the time crate,
transitively.

Signed-off-by: mulhern <amulhern@redhat.com>
mulkieran added a commit to mulkieran/stratisd that referenced this pull request Nov 10, 2021
It turns out that it is not required by the chrono crate.

It was made optional in chrono version 0.4.16, see:
chronotope/chrono#478.

Require chrono 0.4.16 in Cargo.toml, as before that, the clock
feature pulled in the time crate, so that specifying no defaults
and precisely "clock" and "std" will still pull in the time crate,
transitively.

Signed-off-by: mulhern <amulhern@redhat.com>
mulkieran added a commit to mulkieran/stratisd that referenced this pull request Nov 10, 2021
It turns out that it is not required by the chrono crate.

It was made optional in chrono version 0.4.16, see:
chronotope/chrono#478.

Require chrono 0.4.16 in Cargo.toml, as before that, the clock
feature pulled in the time crate, so that specifying no defaults
and precisely "clock" and "std" will still pull in the time crate,
transitively.

Signed-off-by: mulhern <amulhern@redhat.com>
mulkieran added a commit to mulkieran/stratisd that referenced this pull request Nov 10, 2021
It turns out that it is not required by the chrono crate.

It was made optional in chrono version 0.4.16, see:
chronotope/chrono#478.

Require chrono 0.4.16 in Cargo.toml, as before that, the clock
feature pulled in the time crate, so that specifying no defaults
and precisely "clock" and "std" will still pull in the time crate,
transitively.

Signed-off-by: mulhern <amulhern@redhat.com>
mulkieran added a commit to mulkieran/stratisd that referenced this pull request Nov 11, 2021
It turns out that it is not required by the chrono crate.

It was made optional in chrono version 0.4.16, see:
chronotope/chrono#478.

Require chrono 0.4.16 in Cargo.toml, as before that, the clock
feature pulled in the time crate, so that specifying no defaults
and precisely "clock" and "std" will still pull in the time crate,
transitively.

Signed-off-by: mulhern <amulhern@redhat.com>
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.
pickfire pushed a commit to pickfire/chrono that referenced this pull request Jan 22, 2022
The main crate docs made some claims about the Duration type that became
incorrect with the new oldtime feature (chronotope#478).

Per @jhpratt.
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.
botahamec pushed a commit to botahamec/chrono that referenced this pull request May 26, 2022
The main crate docs made some claims about the Duration type that became
incorrect with the new oldtime feature (chronotope#478).

Per @jhpratt.
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.
pickfire pushed a commit to pickfire/chrono that referenced this pull request Jul 5, 2022
The main crate docs made some claims about the Duration type that became
incorrect with the new oldtime feature (chronotope#478).

Per @jhpratt.
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.

Update time crate to 0.2
4 participants