Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Remove the time dependency where possible #8100

Merged
merged 1 commit into from
Mar 14, 2018
Merged

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Mar 13, 2018

cc #7915

The time crate is no longer maintained. This PR replaces it with std::time where possible. Wherever strftime is used, the time dependency has been kept because there is no straight-forward replacement yet.

In addition to being IMO a positive change, this should also help compile for wasm.

The logic of the code has been preserved, except in a few places where initializing a u64 to 0 has been replaced with initializing an Instant to Instant::now(). Since the 0 was mostly a place-holder (similar to None), the behaviour should be exactly the same in the end.

@tomaka tomaka added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Mar 13, 2018
@parity-cla-bot
Copy link

It looks like @tomaka signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@tomaka
Copy link
Contributor Author

tomaka commented Mar 13, 2018

Note that in the future when const fns are stable, we should replace the various u64 constants with Duration constants.

@tomaka
Copy link
Contributor Author

tomaka commented Mar 13, 2018

After this PR, time is used in the following places:

  • In hyper and related crates ; hyper will probably be removed from the client light, because we don't need anything HTTP-related.
  • ethstore because it requires strftime to generate file names. It should be possible to tweak something for wasm here.
  • zip (and msdos_time) ; used only by the dapps crate which we don't need for the light-client-in-the-browser objective.
  • node_health (and its dependency ntp) and logger because they require strftime. This is probably the hardest one to fix.
  • rust-crypto ; I should be able to submit a PR there, but the crate is unmaintained so it's probably not worth it
  • transient-hashmap ; opened a PR there: Remove the dependency on the time crate debris/transient-hashmap#10
  • vergen ; but it's only used as a build-dependency so it's not a problem.

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 13, 2018
@tomaka tomaka removed the A8-looksgood 🦄 Pull request is reviewed well. label Mar 14, 2018
@tomaka
Copy link
Contributor Author

tomaka commented Mar 14, 2018

I just realized that std::time::Instant is not steady, so replacing SteadyTime with Instant could be wrong. However considering that it's only about caching and network timeouts, I don't think that we actually need steady clocks.

@tomaka tomaka added the A8-looksgood 🦄 Pull request is reviewed well. label Mar 14, 2018
@niklasad1
Copy link
Collaborator

@tomaka
What about std::time::SystemTime it is not monotonically increasing but steady I think.

@debris debris merged commit 113c35a into openethereum:master Mar 14, 2018
@tomaka tomaka deleted the rm-time branch March 14, 2018 12:39
@5chdn 5chdn added this to the 1.11 milestone Mar 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants