Skip to content

Bump protobuf + grpc versions #306

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Bump protobuf + grpc versions #306

wants to merge 3 commits into from

Conversation

colin353
Copy link
Collaborator

Sorry for the enormous diff, this is almost all generated by cargo raze.

I had to make a few manual tweaks here to get this to compile, will point them out with comments. But it seems to work.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@colin353 colin353 force-pushed the bump-proto branch 2 times, most recently from 1927030 to 6684888 Compare April 18, 2020 16:53
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@colin353 colin353 changed the title WIP: Bump protobuf + grpc versions Bump protobuf + grpc versions Apr 18, 2020
+ " export TARGET='x86_64-unknown-linux-gnu';"
+ " export RUST_BACKTRACE=1;"
+ " export RUSTC=echo;"
+ " export CARGO_PKG_VERSION=0;"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to add the previous two lines to get this to work. This is definitely a hack, but the build.rs script for this target is just trying to generate a VERSION constant which for debugging/logging purposes, so I'm passing in some fake values.

Copy link
Collaborator

@dfreese dfreese Apr 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also generates an identifier, such as VERSION_2_8_2: () = ();

That is used here:
https://github.com/stepancheg/rust-protobuf/blob/v2.8/protobuf-codegen/src/lib.rs#L160
to make sure protobuf-codegen and protobuf are the same version.

I had looked as using raze to patch the lib.rs file directly, similar to:

diff --git a/src/lib.rs b/src/lib.rs
index b487e6e..be9b345 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -112,11 +112,7 @@ mod protobuf {
     pub use well_known_types;
 }
 
-/// This symbol is in generated `version.rs`, include here for IDE
-#[cfg(never)]
-pub const VERSION: &str = "";
-/// This symbol is in generated `version.rs`, include here for IDE
-#[cfg(never)]
+pub const VERSION: &'static str = "2.8.2";
 #[doc(hidden)]
-pub const VERSION_IDENT: &str = "";
-include!(concat!(env!("OUT_DIR"), "/version.rs"));
+pub const VERSION_IDENT: &'static str = "VERSION_2_8_2";
+pub const VERSION_2_8_2: () = ();

but hadn't completely worked through that yet. The idea was to allow for building without the build.rs file.

@@ -36,6 +36,6 @@ rust_library(
],
version = "0.2.6",
deps = [
"@raze__log__0_3_9//:log",
"@raze__log__0_4_8//:log",
Copy link
Collaborator Author

@colin353 colin353 Apr 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know why, but tests were failing for me until I changed this as well. The log-0.3.9 crate refused to compile. I removed it and replaced it with a later version, which seems to work.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you just force the log version in the Cargo.toml file?

@colin353 colin353 mentioned this pull request Apr 18, 2020
@mfarrugi
Copy link
Collaborator

The only manual changes are to proto/raze/Cargo.toml?

Is there anyway to extract or at least emphasize the generated BUILD files that were patched (looks like no, given google/cargo-raze#58)? This is the sort of thing that creates future pain.

@colin353
Copy link
Collaborator Author

The only manual changes are to proto/raze/Cargo.toml?

No, I also made manual changes to proto/raze/remote/protobuf-2.8.2.BUILD and wasm_bindgen/raze/remote/mime-0.2.6.BUILD.

I agree it's kind of annoying, since anyone who re-runs cargo raze will overrwrite them. Would it be alright if I just wrote some documentation for the manual overrides in the proto/raze directory? I'm not aware of any cargo-raze features that allow overrides like this.

@dfreese
Copy link
Collaborator

dfreese commented Apr 21, 2020

@colin353 I had been taking a slightly different approach with this here, by patching from raze

but had ran into

error[E0599]: no function or associated item named `new` found for type `grpc::ServerBuilder<tls_api_stub::TlsAcceptor>` in the current scope                                                                      
  --> proto/helloworld/greeter_server/greeter_server.rs:38:72
   |
38 |     let mut server = grpc::ServerBuilder::<tls_api_stub::TlsAcceptor>::new();
   |                                                                        ^^^ function or associated item not found in `grpc::ServerBuilder<tls_api_stub::TlsAcceptor>`
   |
   = note: the method `new` exists but the following trait bounds were not satisfied:
           `tls_api_stub::TlsAcceptor : tls_api::TlsAcceptor`

error[E0277]: the trait bound `tls_api_stub::TlsAcceptor: tls_api::TlsAcceptor` is not satisfied
  --> proto/helloworld/greeter_server/greeter_server.rs:38:22
   |
38 |     let mut server = grpc::ServerBuilder::<tls_api_stub::TlsAcceptor>::new();
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `tls_api::TlsAcceptor` is not implemented for `tls_api_stub::TlsAcceptor`
   |
   = note: required by `grpc::ServerBuilder`

error: aborting due to 2 previous errors

Didn't look like we had different versions of protobuf, grpc-rust, tls-api, or tls-api-stub, so wasn't sure if that was something you had run into, or might be somehow a consequence of me pinning the log version.

@dfreese
Copy link
Collaborator

dfreese commented Apr 21, 2020

Nevermind, figured it out. grpc-rust 0.6.2 still depends on tls-api 0.1, so there was a conflict between what was being generated and what the crate was expecting.

I have a branch that builds and tests locally here. It doesn't look like cargo raze is propagating the sha256 values anymore, so I will need to add those.

Any preference on what approach would be best to take?

@colin353
Copy link
Collaborator Author

@dfreese Very interesting, I didn't realize that patching was an option. That's really cool. I think it's a much better approach than manually editing the generated files!

@colin353
Copy link
Collaborator Author

@dfreese want to make a PR for that? Then we can close this and try and get yours merged

@dfreese
Copy link
Collaborator

dfreese commented Apr 21, 2020

Sure, I'll go ahead and do that this afternoon. Feel free to leave this open until one PR or the other is accepted.

@dfreese
Copy link
Collaborator

dfreese commented Apr 22, 2020

PR #310 has just been put up. Please let me know if you have any questions.

@damienmg
Copy link
Collaborator

PR #310 adressed all the reserved on this one and was looking good to me so I went ahead and merge it.

Thanks for both or your work, this is amazing!

@damienmg damienmg closed this Apr 22, 2020
@mfarrugi mfarrugi deleted the bump-proto branch October 8, 2020 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants