Skip to content
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

Update tonic, prost, and friends #660

Merged
merged 4 commits into from Jan 10, 2022

Conversation

davidpdrsn
Copy link
Contributor

Updates tonic and tonic-build to 0.6 and prost, prost-build, and prost-types to 0.8.

When building this locally I get this error:

error: failed to run custom build command for `opentelemetry-zpages v0.1.0 (opentelemetry-rust/opentelemetry-zpages)`

Caused by:
  process didn't exit successfully: `opentelemetry-rust/target/debug/build/opentelemetry-zpages-5a7a6985eea043c2/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'protoc binary not found: cannot find binary path', ~/.cargo/registry/src/github.com-1ecc6299db9ec823/protoc-2.25.2/src/lib.rs:203:17
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

However I also get that main so not sure if its a an issue in my setup or a general issue.

@davidpdrsn davidpdrsn requested a review from a team as a code owner October 26, 2021 07:49
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 26, 2021

CLA Signed

The committers are authorized under a signed CLA.

@davidpdrsn
Copy link
Contributor Author

I'm working on getting the CLA signed. I work at Embark Studios which have contributed before so shouldn't be too hard 🤞

@codecov
Copy link

codecov bot commented Oct 26, 2021

Codecov Report

Merging #660 (d22fdf9) into main (9f3abed) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #660   +/-   ##
=======================================
  Coverage   71.39%   71.39%           
=======================================
  Files         101      101           
  Lines        8564     8564           
=======================================
  Hits         6114     6114           
  Misses       2450     2450           
Impacted Files Coverage Δ
opentelemetry-otlp/src/span.rs 0.00% <ø> (ø)

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 9f3abed...d22fdf9. Read the comment docs.

@TommyCpp
Copy link
Contributor

Thanks! may need to run cargo fmt. The error was because opentelemetry-zpages uses grpcio for Grpc, it requires some dependencies. It's unrelated to your change so you should be good.

@lsunsi
Copy link

lsunsi commented Nov 21, 2021

I'm interested in this!

@davidpdrsn
Copy link
Contributor Author

Sorry for taking so long to finish this, but I have not forgotten about it 😅

However I've realized that tonic 0.6 switched to the 2021 edition which isn't compatible with opentelemetry's MSRV. So we have to move tonic back to 2018 first. I'm doing that here.

@jtescher
Copy link
Member

jtescher commented Dec 8, 2021

@davidpdrsn looks good just one clippy lint left looks like

@davidpdrsn
Copy link
Contributor Author

I just published tonic 0.6.2 which reverted to using rust 2018 so this can now be compatible with opentelemetry's MSRV. I'll update this PR tomorrow.

@davidpdrsn
Copy link
Contributor Author

The lint error from CI seems to be unrelated to these changes. Let me know if you want me to address them somehow.

Otherwise this should be good to go now 😊

@lsunsi
Copy link

lsunsi commented Dec 9, 2021

Thanks @davidpdrsn !

@jtescher
Copy link
Member

@davidpdrsn mind rebasing (or letting otel maintainers edit your branch so we can rebase for you)? The otel org has set all repos to require up to date branches to be merged unfortunately

@davidpdrsn
Copy link
Contributor Author

@jtescher doesn't seem like you can enable maintainer edits after creating the PR. I'll get this rebased within the next few days!

@jtescher
Copy link
Member

@davidpdrsn ok thanks!

@djc
Copy link
Contributor

djc commented Jan 10, 2022

@davidpdrsn the next release is coming soon. Do you have time to rebase this? (May need to open a new PR.)

@davidpdrsn
Copy link
Contributor Author

@djc I rebased just now and it went clean through. 🤞 for CI

@TommyCpp TommyCpp merged commit 5d6d000 into open-telemetry:main Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants