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 prost, prost-derive and prost-types to 0.10, tonic, and tonic-build to 0.7 #1510

Merged
merged 14 commits into from
Apr 7, 2022

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Mar 30, 2022

Update prost, prost-derive and prost-types to 0.10

Closes #1500
Closes #1501
Closes #1502
Closes #1520
Closes #1521

@github-actions github-actions bot added the arrow-flight Changes to the arrow-flight crate label Mar 30, 2022
@alamb
Copy link
Contributor Author

alamb commented Mar 30, 2022

We probably need to wait for a new version of tonic that updates prost: https://crates.io/crates/tonic/0.6.2/dependencies

@tustvold
Copy link
Contributor

Yup, ticket to follow is here - hyperium/tonic#946

@tustvold
Copy link
Contributor

tustvold commented Apr 3, 2022

FYI tonic 0.7 has now been released and updates prost to 0.10

@alamb alamb changed the title chore: Update prost, prost-derive and prost-types to 0.10 chore: Update prost, prost-derive and prost-types to 0.10, tonic, and tonic-build to 0.7 Apr 4, 2022
@alamb
Copy link
Contributor Author

alamb commented Apr 4, 2022

I have updated to use tonic and tonic-build 0.7 and checked in the newly generated code

@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2022

Codecov Report

Merging #1510 (880ffe2) into master (0a625ed) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

❗ Current head 880ffe2 differs from pull request most recent head d6f78b1. Consider uploading reports for the commit d6f78b1 to get more accurate results

@@            Coverage Diff             @@
##           master    #1510      +/-   ##
==========================================
- Coverage   82.77%   82.74%   -0.04%     
==========================================
  Files         190      190              
  Lines       54689    54711      +22     
==========================================
- Hits        45269    45268       -1     
- Misses       9420     9443      +23     
Impacted Files Coverage Δ
arrow-flight/src/arrow.flight.protocol.rs 0.00% <0.00%> (ø)
parquet_derive/src/parquet_field.rs 65.98% <0.00%> (-0.23%) ⬇️

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 0a625ed...d6f78b1. Read the comment docs.

let proto_dir = Path::new("../format");
let proto_path = Path::new("../format/Flight.proto");

tonic_build::configure()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following model in influxdata/pbjson#40 from @tustvold

@@ -57,7 +57,7 @@ jobs:
- name: Setup Build Dependencies
run: |
apt-get update
apt-get install cmake protobuf-compiler
apt-get install -y cmake protobuf-compiler
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should be able to get away without cmake if you install protobuf-compiler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in 0a87a43

- name: Install Build Dependencies
shell: bash
run: |
apt-get update
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to avoid copy/pasting this a bunch of times in rust.yaml

@@ -6,102 +6,101 @@
pub struct HandshakeRequest {
///
/// A defined protocol version
#[prost(uint64, tag = "1")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just checked in the generated code

required: true
default: 'stable'
runs:
using: "composite"
Copy link
Member

Choose a reason for hiding this comment

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

👍

@alamb alamb merged commit c7c7af6 into apache:master Apr 7, 2022
@alamb alamb added the api-change Changes to the arrow API label Apr 15, 2022
@alamb alamb changed the title chore: Update prost, prost-derive and prost-types to 0.10, tonic, and tonic-build to 0.7 Update prost, prost-derive and prost-types to 0.10, tonic, and tonic-build to 0.7 Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API arrow-flight Changes to the arrow-flight crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants