quick-protobuf
quick-protobuf copied to clipboard
tafia
Add no_std support
This is a series of changes to add no_std support to quick-protobuf. It is unfortunately a breaking change; this can't be avoided, as the existing API directly uses the std-only std::io::Write trait. But it only requires regenerating the stubs; after that, everything works the same way it used to.
There are two major changes here:
- You can now choose to use
ArrayVecinstead ofVecfor a given field, using the[rust_gen_arrayvec = <capacity>]option in the schema. This works for regular builds as well, but is required forno_std. - There is a new
WriterBackendtrait, removing the hard dependency onstd::io::Write.
Appveyor failure looks unrelated:
'cargo-fmt.exe' is not installed for the toolchain 'stable-x86_64-pc-windows-msvc'
Travis passes on stable; the other failures appear to be because the test script always wants to be on stable.
This diff is really large :) Mind breaking the "cleanup" commits into a separate PR so we can quickly review and merge them? I think I have many of the same cleanups on a branch somewhere that I've been meaning to PR. Did you run cargo clippy by any chance for the cleanups?
What are people's thoughts on gitignore-ing all the generated files? I find they cause PRs to be quite a bit larger than they should be. I like to see some of the generated code if a PR is changing the code generation, but I don't know if we need to check it all in. Thoughts?
What are people's thoughts on gitignore-ing all the generated files? I find they cause PRs to be quite a bit larger than they should be. I like to see some of the generated code if a PR is changing the code generation, but I don't know if we need to check it all in. Thoughts?
+1 on this; it was pretty awkward dealing with all the generated code diffs in this commit.
This diff is really large :) Mind breaking the "cleanup" commits into a separate PR so we can quickly review and merge them? I think I have many of the same cleanups on a branch somewhere that I've been meaning to PR. Did you run
cargo clippyby any chance for the cleanups?
That's a good idea. I'll make a separate PR for that and base this on that branch. I didn't clippy, but I will.
@tafia Do you have an opinion on gitignoring the generated proto rust files? If you don't object, I'll open a PR.
@mullr, apart from the rust_gen_arrayvec comment, I have a another one: I'd like the default behavior to be as close as today if possible, which means using std::io::Read and not having WriterBackend per default. The reason is to keep the generated code as lean as possible.
That would add a fair bit of complexity to the codegen; you'd have to choose regular mode, and no_std mode. I don't think you'd gain very much at all from it, either: all of the write calls are using static dispatch (not trait objects), so the everything will get monomorphized. S if the caller is using the std::io backend, the compiler will end up generating the exact same code.
Similarly I believe we should not have the arrayvec crate in default features. (default should be std only).
Agreed. I tried to do that, but I couldn't figure out how to enable non-default features for tests. Is there a way?
That would add a fair bit of complexity to the codegen.
Wouldn't it be just replace WriterBackend with std::io::Write and change the use ... at the top?
In terms of knowing when to use what, it could be either an explicit flag or by checking features.
But I agree that this is not super important. @nerdrew, any preference?
I couldn't figure out how to enable non-default features for tests
Haven't tried but something like cargo test --no-default-features --feature xyz should work?
Wouldn't it be just replace WriterBackend with std::io::Write and change the use ... at the top? In terms of knowing when to use what, it could be either an explicit flag or by checking features.
This makes sense to me. I think you'd need a flag for pb-rs and then a feature for quick-protobuf.
By default I like keeping std and std::io::{Error, Read, Write} for ease of use in the common (or should I say... "standard") case.
Question about ArrayVec: it looks like you can use Vec in a no_std environment. I don't really know much about it, but this doc says you can if you can add alloc and collections. When people say no_std do they also usually mean no alloc and no global allocator?
Question about ArrayVec: it looks like you can use Vec in a no_std environment. I don't really know much about it, but this doc says you can if you can add alloc and collections. When people say no_std do they also usually mean no alloc and no global allocator?
Yes, that's right.
Ok, I've rebased this branch on master, with one small change: the field option is now '(rust_max_length)', so it could in principle be used for other kinds of fields, works with protoc, and could in principle be registered as an official extension.
w.r.t. the other requested design changes: I'm out of time to work on this. At this point it's good enough for the use case I have, and I can't really justify any more time spent on reworking it. You guys are welcome to do with it what you want. Of course I'd be thrilled if there was upstream support for no_std.
w.r.t. the other requested design changes: I'm out of time to work on this. At this point it's good enough for the use case I have, and I can't really justify any more time spent on reworking it. You guys are welcome to do with it what you want. Of course I'd be thrilled if there was upstream support for no_std.
Thanks a lot for your work!
Hi @nerdrew @tafia, I'm thinking about taking this branch, removing the arrayvec code, and using the alloc collections when in no_std mode. This removes the need for the length annotations as well. Would you be interested in a PR with those changes?
Looks like this can be closed? no_std support looks like it was merged separately.
I don't think this should be closed entirely yet, as this PR (If i'm reading it correctly) not only provides no_std, but also no_alloc usage?
This is a huge advantage for embedded use-cases, where an allocator is broadly not available or not desired.
What's blocking this PR? Is it just the merge conflicts? It sounds like all that's left for no_alloc support is to merge the rust_max_length option with arrayvec support. (And yes, no_alloc is very important for embedded applications.)
