add no-std support

[japaric]

this PR adds a new API, read_one_from_slice, which works just like read_one but operates on a slice.

this PR also adds an opt-out "std" Cargo feature. all the API that operates on IO objects is behind the "std" feature. read_one_from_slice is available in no-std mode.

this PR is best reviewed on a commit-by-commit basis

[cpu]

Thanks! Here's a first pass.

Two higher-level items:

1. It looks like this breaks cargo test --no-default-features.
2. Could you add CI coverage for the no-std mode?

[cpu]

This leaks the base64 crate into the public API no? I think one advantage of the previous approach was that it kept base64 internal.

[japaric]

Indeed. we can't use the usual Box<dyn std::error::Error> replacement because this enum has to work in no-std context. the alternatives I see are

• drop the tuple field, i.e. simply use Error::Base64Decode. this loses information
• use Base64Decode(Box<dyn core::fmt::Debug>). that trait object is not super common but it's close to Box<dyn Error>. however, it cannot be converted into Box<dyn Error> as there's no From / Into trait that glues the two trait object types
• use Base64Decode(String) where the String is the format!-ed version of Base64Decode. unlike Box<dyn Debug> this can be converted into Box<dyn Error> using the ? operator so that's a plus
• basically copy base64::DecodeError into this crate and use that. that does not expose the base64 dependency but then you have to maintain a function that maps base64::DecodeError into pemfile::DecodeError and update it if base64::DecodeError (which is not marked as non_exhaustive) ever changes.

[djc]

@ctz and I yesterday were chatting that we might want to yeet the use of the base64 crate at the first sign of trouble (since we generally haven't been that happy with it) -- maybe that's this?

[cpu]

use Base64Decode(String) where the String is the format!-ed version of Base64Decode. unlike Box this can be converted into Box using the ? operator so that's a plus

That option seems OK to me. I suspect there's not much value in downstream code being able to programmatically determine if the data was invalid for padding, length, unknown symbol etc. In most cases I expect the error is going to be displayed to a user in string form 🤔

[japaric]

In most cases I expect the error is going to be displayed to a user in string form

I agree. this is not the case where knowing the exact error cause is going to let the user fix the input and retry the operation. most likely, malformed pem files are going to be skipped or result in a fatal error.

[japaric]

@cpu thanks for the review. I have addressed your comments in new commits to ease the review process. I can squash the commits when you are all happy with the final version

[japaric]

I forgot to explicit answer to this:

1. It looks like this breaks cargo test --no-default-features.

given that the -std API is a strict subset of the +std API I don't think running cargo t --no-default-features in addition to cargo t increases coverage. given how the tests are written, I would have to cfg away the whole tests module to make cargo t --no-default-features work. or would you like to split all the existing tests in a _slice and _io version and run only the _slice ones in -std mode?

2. Could you add CI coverage for the no-std mode?

done

[cpu]

thanks for the review. I have addressed your comments in new commits to ease the review process. I can squash the commits when you are all happy with the final version

SGTM! Thank you.

given that the -std API is a strict subset of the +std API I don't think running cargo t --no-default-features in addition to cargo t increases coverage. given how the tests are written, I would have to cfg away the whole tests module to make cargo t --no-default-features work.

From my perspective I think it's less about increasing coverage and more that I found it surprising that cargo test --no-default-features doesn't build. I expect that could confuse folks into thinking it's a bug. Gating the tests module so it doesn't fail to build seems preferable to me, even if it may mask that little testing is actually being done in that configuration. WDYT?

[japaric]

I found it surprising that cargo test --no-default-features doesn't build.

I've pushed a commit to make cargo t --no-default-features compile.

I think the expectation from the point of view of the contributor is that cargo test is the only thing they need to run. the reality is that aside from small library crates, project usually have complex CI that runs more than just cargo test so if one wants to pass CI locally one will have to read the .yml file and copy/paste run the commands therein. it's very few projects that take the effort of making the entire CI easy to run locally using, for example, something like an xtask.

[cpu]

Thanks! Looks good to me once the From trait is fixed up.

[japaric]

I've squashed the commits

[djc]

I think this the no-std commit has too much going on right now. IMO it would be good to have separate commits that:

1. Split read_one_impl() out from read_one()
2. Introduce the new Error type
3. Rename the base64 module (why is this necessary?)
4. Introduce read_one_from_slice()

Additionally, I find the current order of items pretty confusing. Currently the order is kind of bottom-up (contra our conventions in other crates), but this PR then adds read_one_impl() at the end instead of at the top which would be more consistent.

[japaric]

Split the commits.

Rename the base64 module (why is this necessary?)

in a previous version it was masking the base64 dependency and making a use base64::Error import fail. does not seem necessary after removing base64::Error from the Error::Base64Decode variant

[japaric]

I dropped the bit that disabled base64's "std" feature while splitting the commit (and CI still passed). I have updated CI (in the latest commit) to catch usage of libstd in dependencies.

[djc]

Thanks for splitting, this is much better!

[japaric]

@djc applied your suggestions