Optionally remove time crate dependency

[jethrogb]

I would like to use the chrono crate to process time information on systems that don't have a clock. The time crate, which is used by chrono to obtain and represent the local time, can't be made to work on such a system. This PR adds a default-enabled "local" feature to the crate and moves all code that is dependent on the time crate behind a cfg-gate. Current users of chrono need to do nothing. Users of chrono that would like to use it without the time crate can set default-features = false in their Cargo.toml.

Open questions:

• Test coverage I have fixed all unit tests such that they will pass with or without the local feature. There are however 31 doc tests that use some code that has been cfged-out. I'm not sure how to selectively disable them.
• UTC UTC::now and UTC::today also depend on the time crate. I have put them behind the same cfg-gate, but that might be a misnomer. We could leave this as is, or I could add another feature with a different name or rename the current feature.

[jethrogb]

Ping. What would it take to get this merged?

[lifthrasiir]

I had been busy finishing and publishing Kailua for recent weeks. Sorry for delay!

I'm pretty okay to make libtime dependency optional, but I don't want to revive time::Duration in this way. The big problem is that we will eventually want to deprecate time::Duration or its copy in favor of something else---this had been std::time::Duration in 0.2 era, but it turned out that its unsignedness made everything less comfortable. I intend to merge this PR after the solution settles down, but not right now.

[tmccombs]

shouldn't this be #[cfg(not(feature="local"))]?

In which case, UTC::now() needs to be changed to get the current time without using oldtime.

[jethrogb]

No, this is correct. With the local feature enabled, we have extern crate time as oldtime in the crate root. This is needed to get the current time. With UTC::today and UTC::now cfged-out, this import is not used.

[tmccombs]

How about making the time crate dependency optional, and if the time crate isn't provided, define a chrono::Duration struct that allows negative durations (and probably has a similar API to time::Duration)?

It's unfortunate chrono has a dependency on time just for the definition of a single struct.

[jethrogb]

How about making the time crate dependency optional, and if the time crate isn't provided, define a chrono::Duration struct that allows negative durations (and probably has a similar API to time::Duration)?

That's pretty much exactly what this PR is? @lifthrasiir already said he doesn't want to use that definition.

It's unfortunate chrono has a dependency on time just for the definition of a single struct.

That's not quite true, it also uses the time crate to get the current time and figure out what the local timezone is. See mod local and the things with #[cfg(feature="local")] in mod utc.

[lifthrasiir]

I originally intended to merge this PR in 0.4.0, but it is now slightly delayed to 0.4.1 due to the request to make 0.4.0 available ASAP.

[jethrogb]

If I rebase this, will you merge it?

[CryZe]

Can we get this merged in some way? The wasm-unknown-unknown target can't really use chrono otherwise.

[udoprog]

I'd like to put a big 👍 on getting this fixed. I outlined the issues I had with chrono here when porting one of my projects to wasm, and I just wanted to parse ISO 8601 timestamps:
https://udoprog.github.io/rust/2018-02-19/porting-rust-to-wasm.html#whats-the-time-again

[davidhewitt]

+1 would also love to see this! I'm experimenting with isomorphic Rust (server + wasm) and would very much like to use chrono types to represent timestamps in the data passing between server and client but unfortunately this time dependency is getting in the way!

Let me know if I can do anything to help 😄

[CryZe]

@jethrogb We can potentially rebase this for you so this moves ahead faster if you want. There's a lot of interest in seeing this merged now.

[jethrogb]

@CryZe I can rebase it myself no problem. I'm just waiting for a statement from the maintainer that if and when I do, it will get merged.

[quodlibetor]

Sorry, yes, @lifthrasiir has expressed string opposition to merging this, and this is their project.

[jethrogb]

@quodlibetor can you elaborate on the "strong opposition"? Where was this expressed? Last statement from @lifthrasiir was “I originally intended to merge this PR ...”. I'm happy to make changes as requested, but there is no actual proposal on how to change this PR to suit the maintainer's wishes.

Note this PR is pretty much a prerequisite for no_std compatibility discussed in #195.

[quodlibetor]

My understanding (from this comment and this comment in another issue)was that @lifthrasiir wants to replace Duration with chrono::TimeSpan. I haven't seen any work in this space, but the opposition that I thought I saw was to Duration at all, not to getting rid of the time crate dependency.

[quodlibetor]

@jethrogb I've been made a release manager for the chrono org, if you rebase this I'll re-review it with the intention of merging it.

My biggest request at this point would be for you to enhance the commit messages a bit:

• Why do you remove span
• Could you mention where you're importing (and I slightly prefer the termvendoring for this) oldtime from (just a web link or something)

Other than that I think that the fact that this is all behind feature flags means that we can let the good beat out the perfect for at least a little while longer.

[quodlibetor]

Also, just to be clear, I'd love to get clippy working in no-std as full-featured as possible. I'm not at all invested in the embedded space so I'm not sure what that would look like, but almost the only allocations we require are parsing format strings and outputting the results of format strings, so given a Write or some other sink we could probably get rid of them. behind a std feature.

[jethrogb]

Rebased and made requested changes. Open questions from original PR remain.

[quodlibetor]

@jethrogb

• regarding the feature name, should we rename it to just "clock", since what we're doing is using the local clock?
• It's a little embarrassing, but I'm unclear on exactly what you mean by the doctests that are using features that are cfg'd out? Aren't those doctests also cfg'd out when the feature is disabled? Or is something that I'm missing still a problem?

[jethrogb]

Agreed on naming the feature clock.

It's a little embarrassing, but I'm unclear on exactly what you mean by the doctests that are using features that are cfg'd out? Aren't those doctests also cfg'd out when the feature is disabled? Or is something that I'm missing still a problem?

The doctests rely on code that doesn't exist if you don't use the local feature, so they don't compile. I don't know how to apply a cfg to a doctest. This is not a problem for CI, as the doctests are not run for every feature combination in CI.

[jethrogb]

Updated feature name

[quodlibetor]

Ah, I see we're not running with cargo test --no-default-features at all. I agree that that is unfortunate. There's no way to just not run the doctests either, afaict. I would strongly prefer to test with --no-default-features, I'll do some more research.

cc #236