Stop using time crate #44

ctz · 2017-04-17T19:08:11Z

This is briansmith/SystemTime with an extra commit on top to keep the crate no_std, as desired in issue #9.

This introduces a new non-default feature use_std. Personally I could take it or leave it; it saves only a few lines of code in consuming crates.

You can make one of these using `webpki::Time::from_seconds_from_unix_epoch`. - Move ASN1 conversion functions to "calendar.rs", and add some tests. - The new feature `use_std` adds `from<std::time::SystemTime>` to `webpki::Time`. - Fixate time in tests/integration to prevent future expiry. - Add a library-external test of `use_std` feature. - Run tests with `use_std` and without.

briansmith

Hi. Sorry about the long delay in reviewing this. In general this looks good though there are some minor issues that should be addressed. In particular, The most concerning to me is the the unwrap() in the From implementation.

If you won't have time to address the review comments soon (understandable, especially given the long delay), or if you'd rather just have me make them, please LMK and I'll take care of them.

briansmith · 2017-08-18T20:53:03Z

a.txt

@@ -1,13 +0,0 @@
-alg-ecdsa-p256.der


Thanks. I got rid of this file in a separate commit.

briansmith · 2017-08-18T20:53:36Z

Cargo.toml

@@ -21,7 +21,7 @@ license-file = "LICENSE"
 name = "webpki"
 readme = "README.md"
 repository = "https:/briansmith/webpki"
-version = "0.10.2"
+version = "0.11.0"


Let's just undo the version number. I'll bump the version number when I publish the new version on crates.io.

briansmith · 2017-08-18T20:54:05Z

Cargo.toml

@@ -57,11 +57,11 @@ path = "src/webpki.rs"
 [features]
 default = ["trust_anchor_util"]
 trust_anchor_util = []
+use_std = []


Let's name this "std". See https://rust-lang-nursery.github.io/api-guidelines/naming.html#c-feature.

briansmith · 2017-08-18T20:58:34Z

src/time.rs

+ impl convert::From<time::SystemTime> for Time {
+ fn from(st: time::SystemTime) -> Time {
+ Time(st.duration_since(time::UNIX_EPOCH)
+ .unwrap() // it's definitely after 1970 now


I was hoping that TryFrom would be stable by now, but it isn't yet. Regardless, let's give this TryFrom semantics by just adding a normal (non-trait) try_from constructor in the impl Time section that returns a Result.

In particular, although it is true that it is definitely after 1970, we can't assume that the system will know that, especially on non-Unix systems that have an epoch that starts before the Unix epoch. So, unfortunately, this will need to be a fallible conversion the way it is written now.

briansmith · 2017-08-18T20:59:47Z

src/time.rs

+
+ impl convert::From<time::SystemTime> for Time {
+ fn from(st: time::SystemTime) -> Time {
+ Time(st.duration_since(time::UNIX_EPOCH)


Instead of constructing a Time directly using its constructor, let's use the constructor function Time::from_seconds_from_unix_epoch().

briansmith · 2017-08-18T21:02:25Z

src/time.rs

+#[cfg(feature = "use_std")]
+pub mod stdsupport {
+ use std::time;
+ use std::convert;


std::convert::From is in the prelude so this use isn't needed.

briansmith · 2017-08-18T21:03:19Z

src/time.rs

+pub struct Time(u64);
+
+#[cfg(feature = "use_std")]
+pub mod stdsupport {


Why is it useful to put this into a stdsupport submodule?

I think you created the stdsupport submodule because we want to pub use stdsupport::* in order to export the traits from the root module of the crate, right? That's reasonable, but let's document that here with a comment so that it is clear to readers.

However, until we have TryFrom I think it's a moot point and we can just remove stdsupport in favor of adding a try_from() constructor function to Time.

briansmith · 2017-08-18T21:08:54Z

src/time.rs

+ use std::convert;
+ use super::Time;
+
+ impl convert::From<time::SystemTime> for Time {


Unless I'm missing something, we don't need to qualify From because it's in the prelude. For example we have impl From<untrusted::EndOfInput> for Unspecified { ... } elsewhere.

briansmith · 2017-08-18T21:11:12Z

src/time.rs

+
+#[cfg(feature = "use_std")]
+pub mod stdsupport {
+ use std::time;


Let's just use std and then qualify everything with an additional std::, which, IIRC, is the style we've been using in ring and webpki up to this point.

briansmith · 2017-08-18T21:11:45Z

src/time.rs

+pub mod stdsupport {
+ use std::time;
+ use std::convert;
+ use super::Time;


submodules can already see their enclosing module's contents so this use isn't needed.

briansmith · 2017-08-18T22:05:13Z

I went ahead and rebased this on the current master and made most (all?) of the changes I suggested in the branch brian-time-type. LMK what you think.

ctz · 2017-08-20T09:24:53Z

looks good to me!

briansmith · 2017-08-23T18:45:22Z

Thanks! This was all committed, with some clean-ups, as:

edbbb81 Drop dependency on the time crate; use std::time::SystemTime.
7f38a1d Expose our own time type webpki::Time
827d085 Conform to draft Rust API guidelines regarding "std" feature name.
9eefad3 Rename Time::from_seconds_from_unix_epoch() to from_seconds_since_unix_epoch().
222e4af Replace impl From<SystemTime> for Time with a TryFrom-like polyfill.
b02fbef Recommend using Time::try_from() over from_seconds_since_unix_epoch().

Thanks again!

briansmith and others added 3 commits April 17, 2017 09:57

Drop dependency on the time crate; use std::time::SystemTime.

8acb6d2

Delete unused file

c11d1c1

ctz force-pushed the jbp-time-type branch from 298b5b5 to c11d1c1 Compare April 17, 2017 19:26

briansmith requested changes Aug 18, 2017

View reviewed changes

briansmith mentioned this pull request Aug 18, 2017

Optionally expire TrustAnchors #24

Open

briansmith closed this Aug 23, 2017

briansmith mentioned this pull request Sep 20, 2017

Stop using the time crate #9

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop using time crate #44

Stop using time crate #44

ctz commented Apr 17, 2017

briansmith left a comment

briansmith Aug 18, 2017

briansmith Aug 18, 2017

briansmith Aug 18, 2017

briansmith Aug 18, 2017

briansmith Aug 18, 2017

briansmith Aug 18, 2017

briansmith Aug 18, 2017

briansmith Aug 18, 2017

briansmith Aug 18, 2017

briansmith Aug 18, 2017

briansmith commented Aug 18, 2017

ctz commented Aug 20, 2017

briansmith commented Aug 23, 2017

Stop using time crate #44

Stop using time crate #44

Conversation

ctz commented Apr 17, 2017

briansmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansmith commented Aug 18, 2017

ctz commented Aug 20, 2017

briansmith commented Aug 23, 2017