Skip to content

Commit

Permalink
Move protoc generation to binary crate, unpin prost/tonic build (#3876)…
Browse files Browse the repository at this point in the history
… (#3927)

* Move protoc generation to binary crate (#3876)

* Review feedback

* Format

* Fix link
  • Loading branch information
tustvold authored Mar 24, 2023
1 parent 33bbaa5 commit f330496
Show file tree
Hide file tree
Showing 8 changed files with 199 additions and 111 deletions.
15 changes: 13 additions & 2 deletions .github/workflows/arrow_flight.yml
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ on:
- .github/**

jobs:
# test the crate
linux-test:
name: Test
runs-on: ubuntu-latest
Expand All @@ -62,7 +61,19 @@ jobs:
- name: Test --examples
run: |
cargo test -p arrow-flight --features=flight-sql-experimental,tls --examples
- name: Verify workspace clean
vendor:
name: Verify Vendored Code
runs-on: ubuntu-latest
container:
image: amd64/rust
steps:
- uses: actions/checkout@v3
- name: Setup Rust toolchain
uses: ./.github/actions/setup-builder
- name: Run gen
run: ./arrow-flight/regen.sh
- name: Verify workspace clean (if this fails, run ./arrow-flight/regen.sh and check in results)
run: git diff --exit-code

clippy:
Expand Down
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ members = [
"arrow-csv",
"arrow-data",
"arrow-flight",
"arrow-flight/gen",
"arrow-integration-test",
"arrow-integration-testing",
"arrow-ipc",
Expand Down
41 changes: 41 additions & 0 deletions arrow-flight/CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<!---
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->

# Flight

## Generated Code

The prost/tonic code can be generated by running, which in turn invokes the Rust binary located in [gen](./gen)

This is necessary after modifying the protobuf definitions or altering the dependencies of [gen](./gen), and requires a
valid installation of [protoc](https://github.com/protocolbuffers/protobuf#protocol-compiler-installation).

```bash
./regen.sh
```

### Why Vendor

The standard approach to integrating `prost-build` / `tonic-build` is to use a `build.rs` script that automatically generates the code as part of the standard build process.

Unfortunately this caused a lot of friction for users:

- Requires all users to have a protoc install in order to compile the crate - [#2616](https://github.com/apache/arrow-rs/issues/2616)
- Some distributions have very old versions of protoc that don't support required functionality - [#1574](https://github.com/apache/arrow-rs/issues/1574)
- Inconsistent support within IDEs for code completion of automatically generated code
7 changes: 0 additions & 7 deletions arrow-flight/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,6 @@ tempfile = "3.3"
tokio-stream = { version = "0.1", features = ["net"] }
tower = "0.4.13"

[build-dependencies]
# Pin specific version of the tonic-build dependencies to avoid auto-generated
# (and checked in) arrow.flight.protocol.rs from changing
proc-macro2 = { version = "=1.0.53", default-features = false }
prost-build = { version = "=0.11.8", default-features = false }
tonic-build = { version = "=0.8.4", default-features = false, features = ["transport", "prost"] }

[[example]]
name = "flight_sql_server"
required-features = ["flight-sql-experimental", "tls"]
Expand Down
102 changes: 0 additions & 102 deletions arrow-flight/build.rs

This file was deleted.

37 changes: 37 additions & 0 deletions arrow-flight/gen/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.

[package]
name = "gen"
description = "Code generation for arrow-flight"
version = "0.1.0"
edition = "2021"
rust-version = "1.62"
authors = ["Apache Arrow <[email protected]>"]
homepage = "https://github.com/apache/arrow-rs"
repository = "https://github.com/apache/arrow-rs"
license = "Apache-2.0"
publish = false

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
# Pin specific version of the tonic-build dependencies to avoid auto-generated
# (and checked in) arrow.flight.protocol.rs from changing
proc-macro2 = { version = "=1.0.53", default-features = false }
prost-build = { version = "=0.11.8", default-features = false }
tonic-build = { version = "=0.8.4", default-features = false, features = ["transport", "prost"] }
86 changes: 86 additions & 0 deletions arrow-flight/gen/src/main.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
// Licensed to the Apache Software Foundation (ASF) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The ASF licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

use std::{
fs::OpenOptions,
io::{Read, Write},
path::Path,
};

fn main() -> Result<(), Box<dyn std::error::Error>> {
let proto_dir = Path::new("../format");
let proto_path = Path::new("../format/Flight.proto");

tonic_build::configure()
// protoc in unbuntu builder needs this option
.protoc_arg("--experimental_allow_proto3_optional")
.out_dir("src")
.compile_with_config(prost_config(), &[proto_path], &[proto_dir])?;

// read file contents to string
let mut file = OpenOptions::new()
.read(true)
.open("src/arrow.flight.protocol.rs")?;
let mut buffer = String::new();
file.read_to_string(&mut buffer)?;
// append warning that file was auto-generate
let mut file = OpenOptions::new()
.write(true)
.truncate(true)
.open("src/arrow.flight.protocol.rs")?;
file.write_all("// This file was automatically generated through the build.rs script, and should not be edited.\n\n".as_bytes())?;
file.write_all(buffer.as_bytes())?;

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

tonic_build::configure()
// protoc in ubuntu builder needs this option
.protoc_arg("--experimental_allow_proto3_optional")
.out_dir("src/sql")
.compile_with_config(prost_config(), &[proto_path], &[proto_dir])?;

// read file contents to string
let mut file = OpenOptions::new()
.read(true)
.open("src/sql/arrow.flight.protocol.sql.rs")?;
let mut buffer = String::new();
file.read_to_string(&mut buffer)?;
// append warning that file was auto-generate
let mut file = OpenOptions::new()
.write(true)
.truncate(true)
.open("src/sql/arrow.flight.protocol.sql.rs")?;
file.write_all("// This file was automatically generated through the build.rs script, and should not be edited.\n\n".as_bytes())?;
file.write_all(buffer.as_bytes())?;

// Prost currently generates an empty file, this was fixed but then reverted
// https://github.com/tokio-rs/prost/pull/639
let google_protobuf_rs = Path::new("src/sql/google.protobuf.rs");
if google_protobuf_rs.exists() && google_protobuf_rs.metadata().unwrap().len() == 0 {
std::fs::remove_file(google_protobuf_rs).unwrap();
}

// As the proto file is checked in, the build should not fail if the file is not found
Ok(())
}

fn prost_config() -> prost_build::Config {
let mut config = prost_build::Config::new();
config.bytes([".arrow"]);
config
}
21 changes: 21 additions & 0 deletions arrow-flight/regen.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#!/usr/bin/env bash

# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.

SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
cd $SCRIPT_DIR && cargo run --manifest-path gen/Cargo.toml

0 comments on commit f330496

Please sign in to comment.