Make prost a no_std compatible library, and prost-build able to gen…

[cbeck88]

…erate no_std code

The alternative is to get collections types from core and alloc.
In the no_std mode in prost_build, we force it to always use BTreeMap
since HashMap was not stabilized in alloc::collections library.

The functionality is identical and the only incompatibilities in the interface
are that we cannot use std::error::Error or std::io::Error in no_std mode
because these types have not been moved to alloc and there is no alternative.

[cbeck88]

Hi -- would love feedback on this experimental patch.

Are you guys interested in supporting no_std in this way?
I believe that this patch is not a breaking change, and it would help us a lot to be able to use prost in a no_std environment.

We are currently using serde and rmp but it is very painful, we have to patch rmp extensively because it uses std::io::Read and std::io::Write traits and there is seemingly no well-maintained core-only analogue of these. Your decision to base prost on bytes makes it very easy to port it to no_std environment. Now that alloc is stabilized the bytes crate can easily be made #![no_std] and just pull the buffer types it needs from alloc.

There seem to be very few if any good serialization libs in rust that will work in no_std without invasive patching. I think it's possible that Prost can be the best one.

[danburkert]

I don't have time to do a full review, but high level looks good to me. I'm curious if @ameets or @Bascule have thoughts, since they previously iterated on no-std support in #52. Thanks for the PR!

[cbeck88]

Hi @danburkert I realized that the tokio/bytes crate actually currently does implement some std::io traits, for convenience of users, that don't have analogues in core. You aren't using any of that so that won't make this patch invalid, but this PR is premature until there is a feature to turn that stuff off in bytes.

[cbeck88]

fyi: our no_std patch in bytes crate got merged today, so hopefully after forthcoming bytes 0.5 release we can revive this PR

tokio-rs/bytes#281

one thing worth mentioning is, in discussion in that PR, bytes 0.5 is expected to have an MSRV of rust 1.36. Is that going to be okay with prost, or would that be a blocking issue for prost upreving its version of bytes?

[danburkert]

I'm fine with bumping the MSRV to 1.36; I consider both that and moving to a new breaking change of bytes to be a breaking change for prost, but that's no big deal. My sense is the community has just recently moved from 1.32 to 1.36 as a standard version (and I expect everyone will rapidly move to 1.39 as soon as it's released).

[cbeck88]

@danburkert so one thing worth noting is, bytes 0.5 removed the IntoBuf trait in favor of adding a to_bytes method in Buf which does something related: tokio-rs/bytes#288

that's going to require some changes in prost because prost was using IntoBuf for some reason

It's not actually clear to me why prost was using this trait

Would you like to see a separate PR that gets rid of IntoBuf and gets us building against bytes 0.5 first, or are you okay with one big PR that does that and no_std in prost ?

[danburkert]

@danburkert so one thing worth noting is, bytes 0.5 removed the IntoBuf trait in favor of adding a to_bytes method in Buf which does something related: tokio-rs/bytes#288

That's interesting. I can't tell from that PR or the linked issue what the motivation there is. If I recall correctly, the Message trait takes IntoBuf instances instead of Buf so that you can pass in types which impl IntoBuf and not Buf, notably &[u8] and &Vec<u8>. All in all it's a much more ergonomic API that way. I can't tell from that PR what the proposed equivalent in 0.5 is.

[cbeck88]

I think &[u8] does implement Buf now, and they've made it so that when you advance the slice, it drops one bytes off the front:
https://github.com/DoumanAsh/bytes/blob/300ca5d0933f970ae94a64674a6ce0e7590d1a3b/src/buf/buf.rs#L945

I think treating slice this way as Buf may be more efficient than using IntoBuf because IIUC IntoBuf would usually allocate a new Bytes object which might be an unnecessary allocation, maybe I'm missing something though

[danburkert]

Gotcha. It appears that's only the case of the 0.5.x branch, in which case I think you'll be forced to make the change inline with this PR. The tests pass &[u8] to these APIs pretty pervasively, and I'd prefer not to switch all those over to something like Bytes.

[cbeck88]

yeah so what I'm finding is &[u8] works directly, and when passing reference to Vec<u8> I just do something like &v[..] to coerce it to &[u8] and then it builds.

[cbeck88]

Hi @danburkert -- So I need some feedback from you on the following.

When doing code-gen, after this PR, we hope to support both no_std and std configurations. Both prost-build and prost-derive sometimes emit e.g. std::vec::Vec before this revision.

So my approach was:

• In prost-build, the Config object has an enum that specifies whether we use ::std or ::alloc, and in people's build.rs they can say to use alloc.
• In prost-derive, we add a configuration feature std, and if it is off, we try to use ::alloc.

One subtlety here is, is it important that users that consume prost should have to put extern crate alloc; when they use prost-derive macros? I assume that they will do this in the alloc mode, but they might not do this in std mode.

Is it acceptable to require that all users of prost will write extern crate alloc;? Or we should avoid ever emitting ::alloc:... in the std configuration

A second issue I've run into is that, the configuration feature on prost-derive crate currently seems not to work. When I use cargo-expand on the prost-derive crate it seems to always select the feature even when I say --no-default-features.

I'm not sure if proc-macro crates CAN be configured with features. There is a rust issue here: rust-lang/rust#54863

They seem to say that we should just have two versions of the proc macro and the user selects.
That sounds pretty terrible.

I'm trying to figure out if there's a simple way to get this to work, if I've overlooked something, etc.

Finally, in order to test that the alloc configuration is working, I ported the tests into a test-alloc target. Since rust libtest relies on std, I did not use that, instead I compile a simple no_std binary that is exercised with cargo run -p tests-alloc, and it panics if a test fails.

We are likely going to start using this in tree, but I'm going to come back to this and try to iterate to make the best possible patch to go upstream. Let me know if there's anything about the direction, the way I changed the tests, etc. that you want to see work differently, and esp. if you see a good way to deal with the prost-derive configuration issue.

If it's acceptable to require users of prost to do extern crate alloc; (or maybe we can do this similarly to how the prost-derive macros import bytes and prost right now?) then we might be able to avoid doing any cfg stuff in prost-derive, and always use alloc for everything except hashmap. If you'd rather that in std mode, the code-gen we produce is exactly the same as it is today then I need to dig into this proc-macro cfg stuff more I think, or we need to have multiple versions of the derive macro.

I think these are the only remaining development issues -- modulo the cfg stuff working in prost-derive, the tests, tests-2015, and tests-alloc are passing for me, just not all at once.

[cbeck88]

Fyi: I also posted this issue on failure crate which we have now run into: rust-lang-deprecated/failure#322

[cbeck88]

@danburkert as a temporary work around so that we can use prost in our project, what I have done is:
(1) clone failure crate, publish a copy of it on crates.io as alt-failure
(2) make prost-derive depend on alt-failure, but in Cargo.toml rename it to failure (per guidance: rust-lang/cargo#5653)

The purpose of this is that it allows prost-derive to have its own version of failure crate that doesn't have cargo feature unification with the build-plan wide failure crate. I don't know if there is a simpler way to accomplish this in cargo today, my understanding is that there isn't.

It doesn't actually matter for our build that prost-derive uses std when it runs, because it is a proc-macro crate and is not linked to the binary, only the code gen that it outputs is.

In the meantime we can proceed with using prost in our project, hopefully surfacing any remaining technical issues.

Would love any feedback

[danburkert]

I'll go back and answer the thread fully, but WRT failure, I'd also be open to just removing it. I don't think it's adding much value to the derive crate, I mostly used it as an experiment to see what it could do, and it doesn't seem to pull a lot of weight, at least in the derive context. Just replacing it with something like Box<dyn Error> and a custom bail! macro is fine with me.

[cbeck88]

@danburkert I've reviewed what happened in tests crate in more detail -- it appears that during merge resolution, all upstream (prost) changes to tests were taken, and now tests-alloc and test-infra has no code-sharing purpose.

I'm going to push a commit that nukes both of those crates, then I think you should tell me what if any "alloc" configuration test coverage there should be and we'll develop that from the ground up as needed. It's possible that the answer is none because there are no additional configuration options now, in prost-derive and prost-build

[cbeck88]

Hmm its possible this should not have the leading :: and thats why I was getting build failures without extern crate alloc in the protobuf crate

[cbeck88]

I removed leading :: in prost-types and prost-build, which allows to avoid extern crate alloc; in src/tests/lib.rs. However, I still can't get rid of it in protobuf, I think because of leading :: in prost-derive, but I'm not sure.

I had been under the impression that when using rust macros, identifiers outside of the macro e.g. to the standard library are supposed to be fully qualified, because macro expansion can occur at any possible scope, so you either use e.g. $crate:: or :: usually:

https://doc.rust-lang.org/reference/macros-by-example.html#hygiene

However maybe this doesn't actually apply for proc-macros? Since they are more like code-gen than macros?

I'm not totally sure, we're reaching the limits of my understanding of rust. I'm considering trying to remove :: in prost-derive as well, then trying to build against that in my project and see if it works. If so I think we might be able to remove any need for extern crate alloc; except in rust-2015 edition.

@tarcieri I suspect you might know this better than me :)

[tarcieri]

@cbeck88 yeah, it's probably safer to use absolute paths in that case so as to avoid clashing with other things that might be in local scope

[cbeck88]

but if i use absolute paths, does that mean that all prost users (even std users) must do like extern crate alloc; or else they get build errors like I did in protobuf crate?

error[E0433]: failed to resolve: could not find `alloc` in `{{root}}`
   --> /home/chris/prost/prost/target/debug/build/protobuf-0d3e1b26d91b17a5/out/protobuf_unittest.rs:911:30
    |
911 |     pub unpacked_sfixed64: ::alloc::vec::Vec<i64>,
    |                              ^^^^^ could not find `alloc` in `{{root}}`

error[E0433]: failed to resolve: could not find `alloc` in `{{root}}`
   --> /home/chris/prost/prost/target/debug/build/protobuf-0d3e1b26d91b17a5/out/protobuf_unittest.rs:913:27
    |
913 |     pub unpacked_float: ::alloc::vec::Vec<f32>,
    |                           ^^^^^ could not find `alloc` in `{{root}}`

IDK, I'm willing to try removing even the leading :: in prost-derive, and run that against our project, if that will make it nicer for prost users that don't care about no_std, but if there's some reason that removing those is definitely broken I'd prefer to spend my time doing that. I'm not sure though, I feel like I should try it and see if it works

[tarcieri]

You could re-export the types you need via prost so you have a consistent way to import them

[cbeck88]

Ooh, that sounds good

[danburkert]

Yeah I think that sounds reasonable. See e.g. how bytes is now handled: https://github.com/danburkert/prost/blob/master/src/lib.rs#L82-L84

[cbeck88]

@danburkert i ran our project against this revision of prost and all tests are passing

lmk if you see anything you want to be different! thanks

[TheVova]

could we merge this already? :P

[eranrund]

Would love to see it merged as well!

[danburkert]

Closing in favor of #319, which is simply this branch squashed, rebased over master, and with some cleanups applied. Thanks again to @garbageslam and @dflemstr for iterating on this, and sorry it took so long for me to merge. Your contributions are really appreciated.