API revision 0.3.0-beta

[FlorianUekermann]

A major api revision, which adresses #17 and #9.

Already published on crates.io as 0.3.0-beta.1. See https://docs.rs/rustls-acme/0.3.0-beta.1/rustls_acme/index.html for docs.

Open questions:

• Type names are a little inconsistent. Names that are likely to overlap with types from other crates (e.g. Config) have the Acme prefix, others don't. Maybe all/more types should be prefixed, or maybe none?
• Incoming only returns errors from the unterlying TcpListener and successfully established TLS streams. Maybe it should also return Event or at least AcmeState::EventError?

[joshtriplett]

I'm attempting to test 0.3.0-beta.1, and got the following errors when trying to build it:

error[E0277]: the trait bound `der_parser::error::BerError: Clone` is not satisfied
  --> /home/josh/.cargo/registry/src/github.com-1ecc6299db9ec823/x509-parser-0.10.0/src/error.rs:65:17
   |
17 | #[derive(Clone, Debug, PartialEq, thiserror::Error)]
   |          ----- in this derive macro expansion
...
65 |     Der(#[from] BerError),
   |                 ^^^^^^^^ the trait `Clone` is not implemented for `der_parser::error::BerError`
   |
   = note: this error originates in the derive macro `Clone` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `der_parser::error::BerError: Clone` is not satisfied
   --> /home/josh/.cargo/registry/src/github.com-1ecc6299db9ec823/x509-parser-0.10.0/src/extensions.rs:171:9
    |
164 | #[derive(Clone, Debug, PartialEq)]
    |          ----- in this derive macro expansion
...
171 |         error: Err<BerError>,
    |         ^^^^^^^^^^^^^^^^^^^^ the trait `Clone` is not implemented for `der_parser::error::BerError`
    |
    = note: required because of the requirements on the impl of `Clone` for `nom::Err<der_parser::error::BerError>`
    = note: this error originates in the derive macro `Clone` (in Nightly builds, run with -Z macro-backtrace for more info)

[FlorianUekermann]

I can't reproduce this (I tried cargo build -Z minimal-versions). I updated some dependencies and added tighter minimal version requirements on the patch level. Latest is published as 0.3.0-beta.2. If that doesn't do the trick, could you send me your Cargo.lock?

[joshtriplett]

Doing a cargo update with 0.3.0-beta.1 did make the problem go away. Here's the Cargo.lock for the build that failed with the above errors, in case that helps with tightening dependencies: Cargo.lock.txt

[joshtriplett]

0.3.0-beta.2 seems to compile out of the box.

[joshtriplett]

I ran into a usability issue attempting to test 0.3.0-beta.2. If I write something like:

let config = AcmeConfig::new(vec!["domain.example".to_string()])
    .contact_push("admin@example.org".to_string())
    .cache(DirCache::new("/srv/example/acme-cache-dir"));

That results in errors like this:

error[E0282]: type annotations needed
  --> src/lib.rs:15:14
   |
10 |   let config = AcmeConfig::new(vec!["domain.example".to_string()])
   |                -^^^^^^^^^^^^^^
   |                |
   |  ______________cannot infer type for type parameter `EC`
   | |
11 | |             .contact_push("admin@example.org".to_string())
   | |__________________________________________________________- this method call resolves to `AcmeConfig<EC, EA>`

[joshtriplett]

It's possible that making the cache methods only exist in an impl block for AcmeConfig<EC, EA> for appropriately matching types will allow Rust to have enough information to infer the types backwards.

[joshtriplett]

I think I have a solution that's making the inference work better; PR shortly.

[joshtriplett]

PR at https://github.com/FlorianUekermann/rustls-acme/pull/20 .

[joshtriplett]

Posted a couple more PRs to the 0.3-wip branch, the latter of which would affect the API, so it'd need merging before shipping 0.3: https://github.com/FlorianUekermann/rustls-acme/pull/21 https://github.com/FlorianUekermann/rustls-acme/pull/22

[joshtriplett]

After some further debugging (https://github.com/FlorianUekermann/rustls-acme/pull/23 and https://github.com/FlorianUekermann/rustls-acme/pull/24) I managed to successfully test 0.3 with a custom cache. Seems to work quite well!

[FlorianUekermann]

beta3 is released (docs are still in the build queue atm).

Thanks for all the feedback and PRs!

This is probably not the final 0.3.0 release because I would like to take care of the following before that:

• fix #19
• merge #23 (with a few changes)
• emit Event or AcmeState::EventError from Incoming (ergonomic ideas for the Item type welcome!)

[FlorianUekermann]

Also, would you consider a contact_push_email method, for simplicity?

Yes, I was quite close to adding it myself anyway. In the examples contact_emails() where I: IntoIterator<Item = T>, T: AsRef<str> would make the code nicer than an if or loop calling contact_push_email, so maybe that one should be added as well.

I don't think you want impl ToString here. That would allow effectively any Display to work, which seems too general. Perhaps AsRef, or perhaps just &str?

True... I used that to avoid making a clone of already owned string, which is incorrect. Should have used Into<String>. But maybe AsRef<str> is better anyway.

[FlorianUekermann]

beta4 is released (docs are still in the build queue atm).

Besides merging #20 and documentation improvements, the public interface was moved to iterators and AsRef<str>s instead of Vecs and Strings. This improves usability a bit.

I'm going on vacation in about 24h until the 8th of June. Afterwards I would still like to tackle

• emit Event or AcmeState::EventError from Incoming (ergonomic ideas for the Item type welcome!)

19 will have to wait due to lack of better options I am aware of.

Other than that, I think things are in good shape for a proper release in June. Until then the beta should do the job.

[joshtriplett]

@FlorianUekermann Thank you for all your efforts on rustls-acme! I've uploaded tide-acme 0.2.0-beta.1 based on rustls-acme 0.3.0-beta.4, and I'm successfully using it.

I'm not the best person to advise on the types for Incoming, as I'm not directly using it. I'm using the Stream instance on State to get a series of Events, which works quite well; the Event type seems ideal.

Vague idea: I think it would make sense for Incoming to return a custom enum, with variants for receiving a TlsStream or receiving an AcmeEvent. I don't think that enum should be Result (because neither of those cases is an error).

[FlorianUekermann]

I released as is. I'll may do 0.4.0 soon with changes to Incoming, but I don't want people to pick the older versions over the beta until I get around to it.