Add no_std support using alloc collections by xoloki · Pull Request #148 · tafia/quick-protobuf

xoloki · 2019-09-13T17:58:14Z

This PR builds on the work of @mullr in #145 but uses the alloc crate for vector and map types when in no_std mode.

pb-rs now takes a new flag (--nostd). When this flag is passed, the generated Rust code will use alloc::vec::Vec and alloc::collections::BTreeMap. BTreeMap<K,V> will be typedef'd to HashMap<K,V>, so client code will not need to do anything different in std vs no_std modes.

This replaces all public uses of the Writer trait with the new WriterBackend, which defines only the interaction points which are actually used. It also introduces the BytesWriter struct as a new implementation of WriterBackend that will work in no_std, along with the serialize_into_slice convenience fn. This is technically a breaking change, as it will require stubs to be regenerated.

- Add the `std` feature to the quick-protobuf runtime crate - Put uses of Vec and std::io::Write behind the `std` feature - Change other uses of `std` to `core` - Add the `quick-protobuf/no-std-example` compile test

… nostd mode

…nostd mode; regenerate nostd test proto

…re std::io

xoloki · 2019-09-13T19:24:12Z

All tests now passing @tafia @nerdrew.
Not sure why github thinks there are merge commits. I'll try rebasing on upstream master.

xoloki · 2019-09-13T19:26:39Z

Weird, after reloading the page github is no longer complaining about merge commits.

Let me know if you need anything else.

nerdrew · 2019-09-14T03:59:48Z

@@ -0,0 +1,868 @@
+// Automatically generated rust module for 'test_basic_pb.proto' file


Can this file get removed from the PR? We are going to keep only a couple generated files and the no-std-example above shows what the --nostd codegen looks like. Looks like the file should get added to the gitignore.

nerdrew · 2019-09-14T04:02:29Z

@@ -1,4 +1,4 @@
-// This file is generated by rust-protobuf 2.8.0. Do not edit
+// This file is generated by rust-protobuf 2.8.1. Do not edit


Do we need this file checked in? I couldn't tell when I removed a bunch of other generate files if this one could go.

no need to check this one I think

nerdrew · 2019-09-14T04:11:10Z

 cargo run -p quick-protobuf --example pb_rs_example_v3

+# test that no_std can build
+pushd quick-protobuf/no-std-example


Can we condense the example into a single file the rest of the examples? I assume the feature toggling makes it a little trickier... If it isn't too much work, it'd be nice to have it be consistent with the other examples.

nerdrew · 2019-09-14T04:12:04Z

    repeated TestEnumDescriptor enum_field = 16 [packed=false];
 }

+message TestTypesRepeatedArrayVec {


Do we still need this new message?

Nope, removed.

tafia

Thanks for the PR!

On top of what @nerdrew said, could you please remove the HashMap aliasing and review the extern crate declarations.

tafia · 2019-09-16T05:56:56Z

-            {
-                writeln!(w, "use std::borrow::Cow;")?;
+            if config.nostd {
+                writeln!(w, "extern crate alloc;")?;


I am not sure but the extern crate may be repeated many times when implemented this way. Won't the rust compiler complain?

It doesn't appear to happen in the example, as you can see in the file quick-protobuf/examples/pb_rs_nostd/protos/no_std.rs after run_test.

I added an extern crate alloc; to pb_rs_example_nostd.rs, the compiler doesn't care that the create is extern'd twice.

Alright. Sounds weird to me.

tafia · 2019-09-16T05:58:35Z

-                writeln!(w, "use std::collections::HashMap;")?;
+                if config.nostd {
+                    writeln!(w, "use alloc::collections::BTreeMap;")?;
+                    writeln!(w, "pub type HashMap<K, V> = BTreeMap<K, V>;")?;


I'd rather use a BTreeMap directly here.

It seems good to have client code not care about which mode the proto is compiled in, in the default case. Why do you prefer to have separate APIs?

I don't like misleading APIs. HashMap conveys a O(1) and a Hash key instead of a O(log(n)) and a Ord key.

I kinda like the suggestion of depending on hashbrown directly. Would adding that dependency in a no_std env be acceptable?

i actually dont like this at all. in general in no_std you dont want too many dependencies, because the status of your code being 'no_std' depends on all the dependencies (including transitive ones) being no_std, which can get annoying. i see the utility of having support for all protobuf types, but i still think that you should feature-gate this, and have the dependency as optional for this feature (like features = ["hashmap"], etc.)

A feature gate makes sense, though I think we can probably just do a build flag instead since this is just for generated code. pb-rs --hashmap or something like that. Would you want it to error if the proto has a map type but --hashmap isn't enabled?

Also, I assume --hashmap would use hashbrown or something similar if enabled?

Hmmm. We would need a feature-gate + the pb-rs flag I think. Feature-gate for quick-protobuf and flag for pb-rs....

yeah exactly, i agree on the flag + feature. as for errors, it would make most sense to error in case of mismatch. the more interesting way to deal with this (IMHO) is actually given in the protobuf api itself. see https://developers.google.com/protocol-buffers/docs/proto#maps, under "Backwards compatibility". especially the requirement that "Any protocol buffers implementation that supports maps must both produce and accept data that can be accepted by the above definition." Which technically means we can still be wire-compatible even without depending on anything. this is arguably more work to implement, and is probably outside the scope of this PR, but something to think about. as a corollary this probably also means that pb-rs doesnt actually comply with the protobuf specification?

True. I hadn't thought about the maps-are-really-just-a-repeated-field aspect. I like that as the default. I suppose we'd just create a synthetic nested message for the map field in the generated code.

tafia · 2019-09-16T06:01:12Z

-            writeln!(w, "use std::borrow::Cow;")?;
+
+        if config.nostd {
+            writeln!(w, "extern crate alloc;")?;


same comment for the repeated extern crate calls. Usually the files generated by pb-rs will be modules to be inserted into wider project. I believe the extern crate declarations should be in the lib.rs or main.rs file, not here. We could probably trigger some cli message or add a comment in every file?

I think agree with this. The usability of requiring an extern crate ... in your code to use no_std is sub-optimal, but I like the explicitness of it. I don't really like changing global properties because you included some generated code.

see the official take on this from when alloc was stabilised:
https://blog.rust-lang.org/2019/07/04/Rust-1.36.0.html#the-alloc-crate-is-stable
so this should be on lib.rs, yes.

tafia · 2019-09-16T06:01:51Z

@@ -1,4 +1,4 @@
-// This file is generated by rust-protobuf 2.8.0. Do not edit
+// This file is generated by rust-protobuf 2.8.1. Do not edit


no need to check this one I think

…rate modules

tafia · 2019-10-11T03:39:54Z

Thanks for the changes, I didn't see them. Could you fix the fmt issue?

@nerdrew any more comment?
I am still not a fan of HashMap aliasing for BTreeMap.

TheVova · 2019-11-16T09:15:25Z

whats the status with this? i'd have much use of this being no_std... if i can help fix issues let me know. As for HashMap vs B-Tree, i agree with @tafia , changing abstractions like this isnt good. The fact that you put a no_std flag on your library already means there are certain choices you will have to make explicitly. in no_std context the client does and should care about the implementation to a degree. i use Rust pretty exclusively in embedded context (so no_std), and known performance and being sure of whats implemented is important. The correct solution (imho, and as done in most no_std crates) is to feature-gate this type (i.e normally hasmaps are not supported), and if you explicitly opt-in (opt-in to the hashmap feature, not no_std in general), then make it clear the type is changed to BtreeMap.

lexxvir · 2019-11-17T09:49:50Z

Hi all,

I'd like to join to you efforts to make quick-protobuf no_std compatible. Currently I extensively use quick-protobuf in no_std project of my company. I have to maintain my own fork where HashMap is replaced by the BTreeMap (with some other changes).

I suggest to try hashbrown for map types in no_std because std already use it under the hood.

tafia · 2019-11-30T01:36:12Z

@xoloki would you be able to make the changes we discussed on the PR or should someone else make another one?

mullr and others added 18 commits September 9, 2019 15:07

Add rust_gen_arrayvec field option for 'repeated' fields

30eb964

Support no_std in runtime crate

8ec09c0

- Add the `std` feature to the quick-protobuf runtime crate - Put uses of Vec and std::io::Write behind the `std` feature - Change other uses of `std` to `core` - Add the `quick-protobuf/no-std-example` compile test

Add no-std example to codegen script

b4db89e

Change rust_gen_arrayvec to (rust_max_length)

d5bf6f7

remove arrayvec and max_length parameter; use alloc::vec::Vec when in…

6ae5e9e

… nostd mode

let reader use Vec in nostd mode, but not File

34a8fb7

add nostd flag to pb-rs

6139007

use alloc collections in nostd mode

3dd078e

don't allow from_reader in nostd mode

93ed580

restrict serialize_into_vec to std mode; fix perftest config

56a5659

hashmap typedef needs generic args; revert nostd test protobuf

25be3e9

regenerate nostd test proto

35a94a9

packed/fixed fields use Cow, so add the appropriate use statement in …

52c8c5f

…nostd mode; regenerate nostd test proto

remove alloc Vec in nostd mode since the only code that uses it requi…

3fa017e

…re std::io

generate the nostd test modules using the --nostd flag

38ec373

remove arrayvec test code

9dfa1b1

format fixes

2291026

xoloki added 2 commits September 13, 2019 14:44

copy_from_slice requires both slices to have equal length

ca414b2

fmt fixes

5ccfe71

nerdrew reviewed Sep 14, 2019

View reviewed changes

tafia requested changes Sep 16, 2019

View reviewed changes

xoloki added 4 commits September 30, 2019 14:51

remove generated files; remove ArrayVec type from test proto

a2a06c2

move nostd example into standard examples dir and run it with the others

fcae141

remove old nostd example

1dcef86

show that compiler doesn't care about multiple extern crate from sepa…

b56b68e

…rate modules

tafia closed this Nov 22, 2019

tafia reopened this Nov 22, 2019

jpopesculian mentioned this pull request Jan 23, 2020

Add no_std support #158

Merged

tafia merged commit 1c5f753 into tafia:master May 17, 2020

		@@ -0,0 +1,868 @@
		// Automatically generated rust module for 'test_basic_pb.proto' file

		@@ -1,4 +1,4 @@
		// This file is generated by rust-protobuf 2.8.0. Do not edit
		// This file is generated by rust-protobuf 2.8.1. Do not edit

Conversation

xoloki commented Sep 13, 2019

Uh oh!

xoloki commented Sep 13, 2019

Uh oh!

xoloki commented Sep 13, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tafia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheVova Nov 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tafia commented Oct 11, 2019

Uh oh!

TheVova commented Nov 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lexxvir commented Nov 17, 2019

Uh oh!

tafia commented Nov 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

TheVova Nov 24, 2019 •

edited

Loading

TheVova commented Nov 16, 2019 •

edited

Loading