proto: bootstrap crate by greg-szabo · Pull Request #508 · cometbft/tendermint-rs

greg-szabo · 2020-08-05T03:43:11Z

First step of #504 - creates Rust structs from Tendermint proto files.

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG.md

xla

Extra dope! Bunch of questions inline.

I also wonder, if the proto-compiler needs to be its own crate, wouldn't it suffice that it lives as a bin in the proto crate? It would spare some overhead.

In the generated files, do we depend on the tendermint. prefix? It strikes me as odd and doesn't really add any value, maybe worth looking into dropping those.

Cargo.toml

tendermint-proto/Cargo.toml

tendermint-proto/README.md

tendermint-proto/compiler/Cargo.toml

tendermint-proto/compiler/build.rs

xla · 2020-08-05T06:40:53Z

tendermint-proto/compiler/build.rs

+    // Assume that the tendermint Go repository was cloned into the current repository's
+    // target/tendermint folder.


As tendermint is an public repository, can't the build.rs do the checkout. Would reduce some overhead and souce of errors.

xla · 2020-08-05T06:54:38Z

tendermint-proto/compiler/src/main.rs

@@ -0,0 +1,36 @@
+use std::fs::remove_dir_all;


As the compiler is run on demand and not included as a dependency, do we need the extra build.rs or can we consolidate them in here?

I tried, but the prost-build crate is weird: it expects to run during build, then it emits an OUT_DIR env.var which is only available during execution.
Hence, I have no choice but to do the actual translation during build and the move during execution.

The OUT_DIR is a variable that contains a temp.path within the target folder. Normally, prost-build expects you to build the proto files during each build process and include them during the execution with this OUT_DIR env.var.

Instead we copy the files into another crate, the proto crate, so IDEs have an easier time identifying what's what. And it doesn't make sense for us to rebuild the proto files with each build.

Really good to know, let's put your comment into the docuemtnation so others are aware of the limitations.

tendermint-proto/compiler/src/main.rs

tendermint-proto/src/lib.rs

tendermint-proto/README.md

xla

Wanted to state that I didn't review the generated files in depth.

xla · 2020-08-05T07:03:17Z

tendermint-proto/tests/unit.rs

@@ -0,0 +1,10 @@
+#[test]
+pub fn import_evidence_info() {


What are we asserting with this test?

I just didn't want to submit the generated files with no test at all. It's a weak try at "at least do something" with the generated structs.

We can either implement decvent tests for all structs or completely remove testing the automated structs. I'm not sure what's the best way of dealing with it, so this is a reminder to discuss it. :)

I'd ditch this test and capture a follow-up issue or inform an existing which already tracks testing. The challenge here is to find the right level of granularity. As this mostly around serialisation it might make more sense to test it indirectly through other means.

Co-authored-by: Alexander Simmerl <a.simmerl@gmail.com>

…int-rs into greg/proto-crate

greg-szabo · 2020-08-06T01:05:36Z

All comments addressed and apart from the last one about testing the automatically generated structs, all of them are fixed or a reason given why we can't really do much about it.

Please do another sweep and let's see if there are other things we need to discuss.

codecov-commenter · 2020-08-06T01:16:06Z

Codecov Report

Merging #508 into master will decrease coverage by 4.1%.
The diff coverage is 0.1%.

@@           Coverage Diff            @@
##           master    #508     +/-   ##
========================================
- Coverage    30.7%   26.6%   -4.2%     
========================================
  Files         140     150     +10     
  Lines        6181    6106     -75     
  Branches     2051    2001     -50     
========================================
- Hits         1901    1627    -274     
- Misses       2886    3427    +541     
+ Partials     1394    1052    -342

Impacted Files	Coverage Δ
light-client/src/predicates.rs	`85.0% <ø> (ø)`
proto/src/prost/tendermint.abci.rs	`0.0% <0.0%> (ø)`
proto/src/prost/tendermint.blockchain.rs	`0.0% <0.0%> (ø)`
proto/src/prost/tendermint.consensus.rs	`0.0% <0.0%> (ø)`
proto/src/prost/tendermint.crypto.rs	`0.0% <0.0%> (ø)`
proto/src/prost/tendermint.evidence.rs	`0.0% <0.0%> (ø)`
proto/src/prost/tendermint.libs.bits.rs	`0.0% <0.0%> (ø)`
proto/src/prost/tendermint.mempool.rs	`0.0% <0.0%> (ø)`
proto/src/prost/tendermint.p2p.rs	`0.0% <0.0%> (ø)`
proto/src/prost/tendermint.privval.rs	`0.0% <0.0%> (ø)`
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99e8b9d...0ce1ea0. Read the comment docs.

tac0turtle · 2020-08-06T09:48:19Z

Could the generated files be added to codecov ignore section. These generated files should not be included in the code coverage

tac0turtle · 2020-08-06T09:50:37Z

In the generated files, do we depend on the tendermint. prefix? It strikes me as odd and doesn't really add any value, maybe worth looking into dropping those.

not sure if this was answered but this is from the package syntax in the proto files. I havent found a way to avoid this in the generation of the proto stubs. https://github.com/danburkert/prost#packages

xla · 2020-08-06T10:08:36Z

not sure if this was answered but this is from the package syntax in the proto files. I havent found a way to avoid this in the generation of the proto stubs. https://github.com/danburkert/prost#packages

Aren't we doing a manual copy step where we could force the omission of the prefix? @greg-szabo

greg-szabo · 2020-08-07T05:26:14Z

Hey guys! Apologies for the late reply, I was busy today.

I forgot to address the two notes Xla had at the beginning of his review. (Or end or whatever, the one that was not inline.)

So, we could start renaming the files but I don't really see the benefit. The file names follow the directory layout in the Tendermint repo. To be honest, we could suggest getting rid of the tendermint folder in that repo under the proto/ folder, since there's nothing else in there.

Additionally, I don't export the tendermint folder in the path, I use pub use tendermint::*; which then allows the developer to import structures directly with use tendermint_proto::abci:Request and such. And the file names are not seen by the developer, either.

So, I think it's overoptimizing. If you guys still think it's worth doing it, just make a note here and I'll get it done.

The other note I forgot to address was about the proto-compile being its own crate. So, currently, I moved it to the top folder and excluded it from the workspace. So it doesn't get built with a global cargo build but I understand, it's still seen and maybe not needed to be seen.

So, would you want me to move it under proto/bin? I'm not familiar with that kind of directory structure but I'm happy to move it around. Just note, that it's still source code.

greg-szabo · 2020-08-07T05:32:41Z

I've excluded the generated files from codecov, as requested by Marko.

xla

Your rationale is super sound and I don't think we need to optimise any of the new structure. Let's see how it plays out and if we run into limitations. Great work!

🌸 ✈️ 🍃 🔔

liamsi · 2020-08-11T11:23:46Z

Why was this renamed?

light-node/light_node.toml.example → light-node/light_node.toml

I think the file light_node.toml.example is referenced in the Readme of the light node.

liamsi · 2020-08-11T11:25:40Z

proto-compiler/Cargo.toml

+name = "tendermint-proto-compiler"
+version = "0.1.0"
+authors = ["Greg Szabo <greg@informal.systems>"]
+edition = "2018"


It would be good to explicitly mention that this is not meant to be published (yet) via: https://doc.rust-lang.org/cargo/reference/manifest.html#the-publish-field

Opened an issue for that, good point.

liamsi · 2020-08-11T11:27:45Z

proto-compiler/README.md

+* `git clone https://github.com/tendermint/tendermint` into the repository `target/` folder.
+* `cargo run` in the compiler folder.
+
+The resultant structs will be created in the `tendermint-proto/src/prost` folder.


Wasn't the folder renamed from tendermint-proto to proto? Would be good to double check if the above still makes sense.

Yes, good point. Opened a quick issue about it.

greg-szabo · 2020-08-12T05:38:20Z

The example file was renamed accidentally, I've opened an issue about it to fix it.

greg-szabo added 3 commits August 4, 2020 23:40

Tendermint proto crate

7023841

Updated CHANGELOG

ddd7736

Removed debug message

2048fb3

greg-szabo added this to the Go v0.34 Compatibility milestone Aug 5, 2020

xla changed the title ~~Tendermint proto crate~~ proto: bootstrap crate Aug 5, 2020

xla suggested changes Aug 5, 2020

View reviewed changes

xla reviewed Aug 5, 2020

View reviewed changes

xla assigned greg-szabo Aug 5, 2020

xla removed this from the Go v0.34 Compatibility milestone Aug 5, 2020

xla added go Compatibility with Go code serialization structure High level repo-wide structural concerns labels Aug 5, 2020

greg-szabo and others added 9 commits August 5, 2020 08:30

Update tendermint-proto/compiler/Cargo.toml

56d735d

Co-authored-by: Alexander Simmerl <a.simmerl@gmail.com>

Update tendermint-proto/compiler/build.rs

6323d57

Co-authored-by: Alexander Simmerl <a.simmerl@gmail.com>

Moved folders around, removed proto-compiler from workspace

48a55a6

Merge branch 'greg/proto-crate' of github.com:informalsystems/tenderm…

3a49059

…int-rs into greg/proto-crate

Added code to clone Tendermint git

39e73b8

File fixes

694bef2

Removed pub(crate)

4e3bef7

Comment update

3930812

Doc update

0ce1ea0

Remove unneccesary files

a86549a

tac0turtle mentioned this pull request Aug 6, 2020

Merge abci-rs in tendermint-rs repository #489

Closed

Exclude auto-generated struct files from codecov

6a7cfa3

xla approved these changes Aug 7, 2020

View reviewed changes

greg-szabo merged commit da9e177 into master Aug 7, 2020

greg-szabo deleted the greg/proto-crate branch August 7, 2020 20:26

liamsi reviewed Aug 11, 2020

View reviewed changes

This was referenced Aug 12, 2020

TOML example file was accidentally renamed #517

Closed

Explicitly mention that proto-compiler should not be published #518

Closed

Fix inconsistent folder names in documentation #519

Closed

		// Assume that the tendermint Go repository was cloned into the current repository's
		// target/tendermint folder.

Conversation

greg-szabo commented Aug 5, 2020

Uh oh!

xla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

xla 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!

greg-szabo commented Aug 6, 2020

Uh oh!

codecov-commenter commented Aug 6, 2020

Codecov Report

Uh oh!

tac0turtle commented Aug 6, 2020

Uh oh!

tac0turtle commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xla commented Aug 6, 2020

Uh oh!

greg-szabo commented Aug 7, 2020

Uh oh!

greg-szabo commented Aug 7, 2020

Uh oh!

xla left a comment

Choose a reason for hiding this comment

Uh oh!

liamsi commented Aug 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamsi Aug 11, 2020 • 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!

greg-szabo commented Aug 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tac0turtle commented Aug 6, 2020 •

edited

Loading

liamsi Aug 11, 2020 •

edited

Loading