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

Bump to 0.7.0 #87

Merged
merged 5 commits into from
Dec 3, 2023
Merged

Bump to 0.7.0 #87

merged 5 commits into from
Dec 3, 2023

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Dec 2, 2023

And bump dependencies from alphas to released versions, fix examples to accommodate breaking changes.

@cpu
Copy link
Member

cpu commented Dec 2, 2023

I figured I'd upload a partial PR and see if anyone else can figure it out.

I think you're just missing a bump of the pki-types dep in this crate to 1.0 to match the other deps. I'll push a revision.

@ctz
Copy link
Member

ctz commented Dec 2, 2023

Note we should avoid "=" version specs. Those are only useful for pre-releases, and are actively bad for public releases.

@cpu
Copy link
Member

cpu commented Dec 2, 2023

cpu force-pushed the release-0.7.0 branch from 8277c2d to 0f55cc4

  • Added the pki-types update to fix the build breakage.
  • Updated the examples to accommodate rustls 0.22 API changes to fix the tests.
  • Removed the = version specs for the updated deps that are no longer pinned to alphas.

@cpu
Copy link
Member

cpu commented Dec 2, 2023

Hm I think we should also probably separate out the crate version update into its own commit.

@cpu
Copy link
Member

cpu commented Dec 2, 2023

separate out the crate version update into its own commit.

Done.

Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
cpu and others added 2 commits December 2, 2023 16:46
Bump dependencies from alphas to released versions, fix examples to
accommodate breaking changes.
@jsha
Copy link
Contributor Author

jsha commented Dec 2, 2023

Looks good to me. Thanks for the fixups.

@djc
Copy link
Member

djc commented Dec 2, 2023

Hmm, should we avoid the collect() in load_pem_certs() and yield an iterator like we do in rustls-pemfile?

@jsha
Copy link
Contributor Author

jsha commented Dec 3, 2023

Hmm, should we avoid the collect() in load_pem_certs() and yield an iterator like we do in rustls-pemfile?

Tempting. It would be nice to have symmetry between the two packages. Just now I tried going down this road a little ways:

fn load_pem_certs(
    path: &Path,
) -> impl Iterator<Item = Result<CertificateDer<'static>, Error>> + '_ {
    let mut f = BufReader::new(File::open(path)?);
    rustls_pemfile::certs(&mut f).map(|result| match result {
        Ok(der) => Ok(der),
        Err(err) => Err(Error::new(
            ErrorKind::InvalidData,
            format!("could not load PEM file {path:?}: {err}"),
        )),
    })
}

That first ? is a problem because this function doesn't return Result, it returns impl Iterator<Item = Result<_>>. The natural thing to do from an implementation perspective is to return Result<impl Iterator<Item = Result<CertificateDer<'static>, Error>>, Error>. But from an API design perspective that's bad; we'd prefer to reduce the depth at which we nest results and the places at which we can return error.

I tried turning that first potential error into an iter::once containing an error, but then we get conflicting iterator types: Once vs Map. We could fix that by boxing up the return value, but I don't like that either.

My slight inclination is to leave the return type for load_pem_certs() as a Vec. There's some missed optimization - someone could map the result with webpki::anchor_from_trusted_cert and thus quickly deallocating each Vec rather than storing it. But I think that doesn't really amount to much.

Thanks to all for the fixups! This looks good to me.

@djc
Copy link
Member

djc commented Dec 3, 2023

Agreed.

@cpu cpu enabled auto-merge December 3, 2023 14:52
@cpu cpu added this pull request to the merge queue Dec 3, 2023
@cpu cpu removed this pull request from the merge queue due to a manual request Dec 3, 2023
@cpu cpu enabled auto-merge December 3, 2023 14:59
@cpu
Copy link
Member

cpu commented Dec 3, 2023

Pushed an update to the README changelog, and a fix for the out-of-sync description of the load_native_certs fn.

@cpu cpu added this pull request to the merge queue Dec 3, 2023
Merged via the queue into rustls:main with commit 03974af Dec 3, 2023
14 checks passed
@cpu
Copy link
Member

cpu commented Dec 3, 2023

  • Pushed a v/0.7.0 tag
  • Published rustls-native-certs v0.7.0 at registry crates-io

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.

None yet

4 participants