From 2de785aca480779ab0d4442d4bc91a696e3e6c89 Mon Sep 17 00:00:00 2001 From: Dan Burkert Date: Mon, 20 Apr 2020 18:47:34 -0600 Subject: [PATCH] move to GitHub Actions for CI (#304) Transitions the project's CI infrastructure, and fixes a bunch of associated tests, clippy warnings, and rustfmt issues. --- .appveyor.yml | 30 ---- .../continuous-integration-workflow.yaml | 84 +++++++++++ .travis.yml | 29 ---- benches/varint.rs | 3 +- clippy.toml | 1 + conformance/tests/conformance.rs | 2 + prost-build/src/ast.rs | 6 +- prost-build/src/code_generator.rs | 21 +-- prost-build/src/ident.rs | 3 + prost-build/src/lib.rs | 28 ++-- prost-derive/src/field/group.rs | 2 +- prost-derive/src/field/map.rs | 49 ++++--- prost-derive/src/field/message.rs | 2 +- prost-derive/src/field/mod.rs | 10 +- prost-derive/src/field/oneof.rs | 2 +- prost-derive/src/field/scalar.rs | 16 +-- protobuf/Cargo.toml | 1 + protobuf/benches/dataset.rs | 18 ++- protobuf/build.rs | 132 ++++++++++-------- protobuf/src/lib.rs | 2 + src/encoding.rs | 13 +- tests/src/bootstrap.rs | 3 + tests/src/build.rs | 2 +- tests/src/deprecated_field.rs | 5 +- tests/src/lib.rs | 8 +- tests/src/unittest.rs | 2 + 26 files changed, 266 insertions(+), 208 deletions(-) delete mode 100644 .appveyor.yml create mode 100644 .github/workflows/continuous-integration-workflow.yaml delete mode 100644 .travis.yml create mode 100644 clippy.toml diff --git a/.appveyor.yml b/.appveyor.yml deleted file mode 100644 index 426577ca0..000000000 --- a/.appveyor.yml +++ /dev/null @@ -1,30 +0,0 @@ -environment: - matrix: - - TARGET: i686-pc-windows-msvc - - TARGET: x86_64-pc-windows-msvc - -install: - # Install rust, x86_64-pc-windows-msvc host - - appveyor-retry appveyor DownloadFile https://win.rustup.rs/ -FileName rustup-init.exe - - rustup-init.exe -y --default-host x86_64-pc-windows-msvc - - set PATH=%PATH%;C:\Users\appveyor\.cargo\bin - - # Install the target we're compiling for - - if NOT "%TARGET%" == "x86_64-pc-windows-msvc" rustup target add %TARGET% - - # let's see what we got - - where gcc rustc cargo - - rustc -vV - - cargo -vV - - set CARGO_TARGET_DIR=%CD%\target - -build: false - -test_script: - - SET RUST_BACKTRACE=1 - # The Protobuf conformance test runner can't be compiled on Windows. - - cargo test --target %TARGET% --all --exclude conformance - -cache: - - C:\Users\appveyor\.cargo\registry - - target diff --git a/.github/workflows/continuous-integration-workflow.yaml b/.github/workflows/continuous-integration-workflow.yaml new file mode 100644 index 000000000..ffae271be --- /dev/null +++ b/.github/workflows/continuous-integration-workflow.yaml @@ -0,0 +1,84 @@ +name: continuous integration +on: pull_request + +jobs: + + rustfmt: + runs-on: ubuntu-latest + steps: + - name: checkout + uses: actions/checkout@v2 + - name: install toolchain + uses: actions-rs/toolchain@v1 + with: + toolchain: stable + override: true + components: rustfmt + - name: rustfmt + uses: actions-rs/cargo@v1 + with: + command: fmt + args: --all -- --check + + clippy: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v1 + - name: install toolchain + uses: actions-rs/toolchain@v1 + with: + toolchain: stable + override: true + components: clippy + - name: install ninja + uses: seanmiddleditch/gha-setup-ninja@v1 + - name: cache target directory + uses: actions/cache@v1 + with: + path: target + key: ${{ runner.os }}-clippy-target-${{ hashFiles('**/Cargo.toml') }}-${{ hashFiles('**/*.rs') }} + restore_keys: | + ${{ runner.os }}-clippy-target-${{ hashFiles('**/Cargo.toml') }}- + ${{ runner.os }}-clippy-target- + + - name: clippy + uses: actions-rs/clippy-check@v1 + with: + token: ${{ secrets.GITHUB_TOKEN }} + args: --workspace --all-targets + + test: + runs-on: ${{ matrix.os }} + strategy: + matrix: + toolchain: + - stable + - 1.39.0 + os: + - ubuntu-latest + - macos-latest + - windows-latest + steps: + - name: checkout + uses: actions/checkout@v2 + - name: install toolchain + uses: actions-rs/toolchain@v1 + with: + toolchain: ${{ matrix.toolchain }} + override: true + components: rustfmt + - name: install ninja + uses: seanmiddleditch/gha-setup-ninja@v1 + - name: cache target directory + uses: actions/cache@v1 + with: + path: target + key: ${{ runner.os }}-${{ matrix.toolchain }}-target-${{ hashFiles('**/Cargo.toml') }}-${{ hashFiles('**/*.rs') }} + restore_keys: | + ${{ runner.os }}-${{ matrix.toolchain }}-target-${{ hashFiles('**/Cargo.toml') }}- + ${{ runner.os }}-${{ matrix.toolchain }}-target- + - name: test + uses: actions-rs/cargo@v1 + with: + command: test + args: --workspace --all-targets diff --git a/.travis.yml b/.travis.yml deleted file mode 100644 index 57406dab7..000000000 --- a/.travis.yml +++ /dev/null @@ -1,29 +0,0 @@ -language: rust -dist: xenial - -cache: cargo - -os: - - linux - - osx - -rust: - - 1.39.0 - - nightly - -script: - - cargo build --verbose --all --exclude benchmarks - - cargo test --verbose --all --exclude benchmarks - - if [[ $TRAVIS_RUST_VERSION = nightly* ]]; then - cargo bench --verbose --no-run; - fi - -addons: - homebrew: - packages: - - cmake - - ninja - apt: - packages: - - cmake - - ninja-build diff --git a/benches/varint.rs b/benches/varint.rs index 4aa6ad9ad..045691e5f 100644 --- a/benches/varint.rs +++ b/benches/varint.rs @@ -48,11 +48,10 @@ fn benchmark_varint(criterion: &mut Criterion, name: &str, mut values: Vec) }) .throughput(Throughput::Bytes(decoded_len)); - let encoded_len_values = values.clone(); let encoded_len = Benchmark::new("encoded_len", move |b| { b.iter(|| { let mut sum = 0; - for &value in &encoded_len_values { + for &value in &values { sum += encoded_len_varint(value); } criterion::black_box(sum); diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 000000000..5988e12d8 --- /dev/null +++ b/clippy.toml @@ -0,0 +1 @@ +too-many-arguments-threshold=8 diff --git a/conformance/tests/conformance.rs b/conformance/tests/conformance.rs index aaf48ea96..39d87ce8a 100644 --- a/conformance/tests/conformance.rs +++ b/conformance/tests/conformance.rs @@ -1,3 +1,5 @@ +#![cfg(not(target_os = "windows"))] + use std::env; use std::process::Command; diff --git a/prost-build/src/ast.rs b/prost-build/src/ast.rs index 31a369a42..5fd2d591b 100644 --- a/prost-build/src/ast.rs +++ b/prost-build/src/ast.rs @@ -37,9 +37,9 @@ impl Comments { .as_ref() .map_or(Vec::new(), get_lines); Comments { - leading_detached: leading_detached, - leading: leading, - trailing: trailing, + leading_detached, + leading, + trailing, } } diff --git a/prost-build/src/code_generator.rs b/prost-build/src/code_generator.rs index eaa735cf6..31dc04a42 100644 --- a/prost-build/src/code_generator.rs +++ b/prost-build/src/code_generator.rs @@ -105,14 +105,9 @@ impl<'a> CodeGenerator<'a> { code_gen.path.pop(); } - let buf = code_gen.buf; - code_gen - .config - .service_generator - .as_mut() - .map(|service_generator| { - service_generator.finalize(buf); - }); + if let Some(service_generator) = code_gen.config.service_generator.as_mut() { + service_generator.finalize(code_gen.buf); + } code_gen.path.pop(); } @@ -584,7 +579,7 @@ impl<'a> CodeGenerator<'a> { self.depth += 1; self.path.push(2); - for (idx, value) in enum_values.into_iter().enumerate() { + for (idx, value) in enum_values.iter().enumerate() { // Skip duplicate enum values. Protobuf allows this when the // 'allow_alias' option is set. if !numbers.insert(value.number()) { @@ -677,11 +672,9 @@ impl<'a> CodeGenerator<'a> { options: service.options.unwrap_or_default(), }; - let buf = &mut self.buf; - self.config - .service_generator - .as_mut() - .map(move |service_generator| service_generator.generate(service, buf)); + if let Some(service_generator) = self.config.service_generator.as_mut() { + service_generator.generate(service, &mut self.buf) + } } fn push_indent(&mut self) { diff --git a/prost-build/src/ident.rs b/prost-build/src/ident.rs index af2d75785..046fb1efb 100644 --- a/prost-build/src/ident.rs +++ b/prost-build/src/ident.rs @@ -76,6 +76,9 @@ pub fn match_ident(matcher: &str, msg: &str, field: Option<&str>) -> bool { #[cfg(test)] mod tests { + + #![allow(clippy::cognitive_complexity)] + use super::*; #[test] diff --git a/prost-build/src/lib.rs b/prost-build/src/lib.rs index c0c572462..07cd0645a 100644 --- a/prost-build/src/lib.rs +++ b/prost-build/src/lib.rs @@ -1,4 +1,5 @@ #![doc(html_root_url = "https://docs.rs/prost-build/0.6.1")] +#![allow(clippy::option_as_ref_deref)] //! `prost-build` compiles `.proto` files into Rust. //! @@ -50,9 +51,10 @@ //! `build.rs` build-script: //! //! ```rust,no_run -//! fn main() { -//! prost_build::compile_protos(&["src/items.proto"], -//! &["src/"]).unwrap(); +//! # use std::io::Result; +//! fn main() -> Result<()> { +//! prost_build::compile_protos(&["src/items.proto"], &["src/"])?; +//! Ok(()) //! } //! ``` //! @@ -492,11 +494,12 @@ impl Config { /// # Example `build.rs` /// /// ```rust,no_run - /// fn main() { - /// let mut prost_build = prost_build::Config::new(); - /// prost_build.btree_map(&["."]); - /// prost_build.compile_protos(&["src/frontend.proto", "src/backend.proto"], - /// &["src"]).unwrap(); + /// # use std::io::Result; + /// fn main() -> Result<()> { + /// let mut prost_build = prost_build::Config::new(); + /// prost_build.btree_map(&["."]); + /// prost_build.compile_protos(&["src/frontend.proto", "src/backend.proto"], &["src"])?; + /// Ok(()) /// } /// ``` pub fn compile_protos

(&mut self, protos: &[P], includes: &[P]) -> Result<()> @@ -658,9 +661,10 @@ impl default::Default for Config { /// # Example `build.rs` /// /// ```rust,no_run -/// fn main() { -/// prost_build::compile_protos(&["src/frontend.proto", "src/backend.proto"], -/// &["src"]).unwrap(); +/// # use std::io::Result; +/// fn main() -> Result<()> { +/// prost_build::compile_protos(&["src/frontend.proto", "src/backend.proto"], &["src"])?; +/// Ok(()) /// } /// ``` /// @@ -741,7 +745,7 @@ mod tests { impl ServiceGenerator for MockServiceGenerator { fn generate(&mut self, service: Service, _buf: &mut String) { let mut state = self.state.borrow_mut(); - state.service_names.push(service.name.clone()); + state.service_names.push(service.name); } fn finalize(&mut self, _buf: &mut String) { diff --git a/prost-derive/src/field/group.rs b/prost-derive/src/field/group.rs index 11fc237db..8babca20f 100644 --- a/prost-derive/src/field/group.rs +++ b/prost-derive/src/field/group.rs @@ -51,7 +51,7 @@ impl Field { Ok(Some(Field { label: label.unwrap_or(Label::Optional), - tag: tag, + tag, })) } diff --git a/prost-derive/src/field/map.rs b/prost-derive/src/field/map.rs index 0bb4a56e4..23a31a505 100644 --- a/prost-derive/src/field/map.rs +++ b/prost-derive/src/field/map.rs @@ -58,10 +58,9 @@ impl Field { .get_ident() .and_then(|i| MapTy::from_str(&i.to_string())) { - let (k, v): (String, String) = match *attr { + let (k, v): (String, String) = match &*attr { Meta::NameValue(MetaNameValue { - lit: Lit::Str(ref lit), - .. + lit: Lit::Str(lit), .. }) => { let items = lit.value(); let mut items = items.split(',').map(ToString::to_string); @@ -75,19 +74,19 @@ impl Field { } (k, v) } - Meta::List(ref meta_list) => { + Meta::List(meta_list) => { // TODO(rustlang/rust#23121): slice pattern matching would make this much nicer. if meta_list.nested.len() != 2 { bail!("invalid map attribute: must contain key and value types"); } let k = match &meta_list.nested[0] { - &NestedMeta::Meta(Meta::Path(ref k)) if k.get_ident().is_some() => { + NestedMeta::Meta(Meta::Path(k)) if k.get_ident().is_some() => { k.get_ident().unwrap().to_string() } _ => bail!("invalid map attribute: key must be an identifier"), }; let v = match &meta_list.nested[1] { - &NestedMeta::Meta(Meta::Path(ref v)) if v.get_ident().is_some() => { + NestedMeta::Meta(Meta::Path(v)) if v.get_ident().is_some() => { v.get_ident().unwrap().to_string() } _ => bail!("invalid map attribute: value must be an identifier"), @@ -107,11 +106,11 @@ impl Field { } Ok(match (types, tag.or(inferred_tag)) { - (Some((map_ty, key_ty, val_ty)), Some(tag)) => Some(Field { - map_ty: map_ty, - key_ty: key_ty, - value_ty: val_ty, - tag: tag, + (Some((map_ty, key_ty, value_ty)), Some(tag)) => Some(Field { + map_ty, + key_ty, + value_ty, + tag, }), _ => None, }) @@ -128,8 +127,8 @@ impl Field { let ke = quote!(::prost::encoding::#key_mod::encode); let kl = quote!(::prost::encoding::#key_mod::encoded_len); let module = self.map_ty.module(); - match self.value_ty { - ValueTy::Scalar(scalar::Ty::Enumeration(ref ty)) => { + match &self.value_ty { + ValueTy::Scalar(scalar::Ty::Enumeration(ty)) => { let default = quote!(#ty::default() as i32); quote! { ::prost::encoding::#module::encode_with_default( @@ -144,7 +143,7 @@ impl Field { ); } } - ValueTy::Scalar(ref value_ty) => { + ValueTy::Scalar(value_ty) => { let val_mod = value_ty.module(); let ve = quote!(::prost::encoding::#val_mod::encode); let vl = quote!(::prost::encoding::#val_mod::encoded_len); @@ -180,8 +179,8 @@ impl Field { let key_mod = self.key_ty.module(); let km = quote!(::prost::encoding::#key_mod::merge); let module = self.map_ty.module(); - match self.value_ty { - ValueTy::Scalar(scalar::Ty::Enumeration(ref ty)) => { + match &self.value_ty { + ValueTy::Scalar(scalar::Ty::Enumeration(ty)) => { let default = quote!(#ty::default() as i32); quote! { ::prost::encoding::#module::merge_with_default( @@ -194,7 +193,7 @@ impl Field { ) } } - ValueTy::Scalar(ref value_ty) => { + ValueTy::Scalar(value_ty) => { let val_mod = value_ty.module(); let vm = quote!(::prost::encoding::#val_mod::merge); quote!(::prost::encoding::#module::merge(#km, #vm, &mut #ident, buf, ctx)) @@ -217,8 +216,8 @@ impl Field { let key_mod = self.key_ty.module(); let kl = quote!(::prost::encoding::#key_mod::encoded_len); let module = self.map_ty.module(); - match self.value_ty { - ValueTy::Scalar(scalar::Ty::Enumeration(ref ty)) => { + match &self.value_ty { + ValueTy::Scalar(scalar::Ty::Enumeration(ty)) => { let default = quote!(#ty::default() as i32); quote! { ::prost::encoding::#module::encoded_len_with_default( @@ -230,7 +229,7 @@ impl Field { ) } } - ValueTy::Scalar(ref value_ty) => { + ValueTy::Scalar(value_ty) => { let val_mod = value_ty.module(); let vl = quote!(::prost::encoding::#val_mod::encoded_len); quote!(::prost::encoding::#module::encoded_len(#kl, #vl, #tag, &#ident)) @@ -252,7 +251,7 @@ impl Field { /// Returns methods to embed in the message. pub fn methods(&self, ident: &Ident) -> Option { - if let ValueTy::Scalar(scalar::Ty::Enumeration(ref ty)) = self.value_ty { + if let ValueTy::Scalar(scalar::Ty::Enumeration(ty)) = &self.value_ty { let key_ty = self.key_ty.rust_type(); let key_ref_ty = self.key_ty.rust_ref_type(); @@ -309,8 +308,8 @@ impl Field { builder.finish() } }; - match self.value_ty { - ValueTy::Scalar(ref ty) => { + match &self.value_ty { + ValueTy::Scalar(ty) => { let value = ty.rust_type(); quote! { struct #wrapper_name<'a>(&'a ::std::collections::#type_name<#key, #value>); @@ -374,8 +373,8 @@ impl ValueTy { /// If the contained value is enumeration, it tries to convert it to the variant. If not, it /// just forwards the implementation. fn debug(&self) -> TokenStream { - match *self { - ValueTy::Scalar(ref ty) => fake_scalar(ty.clone()).debug(quote!(ValueWrapper)), + match self { + ValueTy::Scalar(ty) => fake_scalar(ty.clone()).debug(quote!(ValueWrapper)), ValueTy::Message => quote!( fn ValueWrapper(v: T) -> T { v diff --git a/prost-derive/src/field/message.rs b/prost-derive/src/field/message.rs index 26c034aef..cd63ef97e 100644 --- a/prost-derive/src/field/message.rs +++ b/prost-derive/src/field/message.rs @@ -54,7 +54,7 @@ impl Field { Ok(Some(Field { label: label.unwrap_or(Label::Optional), - tag: tag, + tag, })) } diff --git a/prost-derive/src/field/mod.rs b/prost-derive/src/field/mod.rs index 14b908ecf..a5a798777 100644 --- a/prost-derive/src/field/mod.rs +++ b/prost-derive/src/field/mod.rs @@ -184,8 +184,8 @@ pub enum Label { } impl Label { - fn as_str(&self) -> &'static str { - match *self { + fn as_str(self) -> &'static str { + match self { Label::Optional => "optional", Label::Required => "required", Label::Repeated => "repeated", @@ -193,7 +193,7 @@ impl Label { } fn variants() -> slice::Iter<'static, Label> { - const VARIANTS: &'static [Label] = &[Label::Optional, Label::Required, Label::Repeated]; + const VARIANTS: &[Label] = &[Label::Optional, Label::Required, Label::Repeated]; VARIANTS.iter() } @@ -350,7 +350,7 @@ fn tags_attr(attr: &Meta) -> Result>, Error> { bail!("invalid tag attribute: {:?}", attr); } } - return Ok(Some(tags)); + Ok(Some(tags)) } Meta::NameValue(MetaNameValue { lit: Lit::Str(ref lit), @@ -360,7 +360,7 @@ fn tags_attr(attr: &Meta) -> Result>, Error> { .split(',') .map(|s| s.trim().parse::().map_err(Error::from)) .collect::, _>>() - .map(|tags| Some(tags)), + .map(Some), _ => bail!("invalid tag attribute: {:?}", attr), } } diff --git a/prost-derive/src/field/oneof.rs b/prost-derive/src/field/oneof.rs index db4e19076..81ac44a2a 100644 --- a/prost-derive/src/field/oneof.rs +++ b/prost-derive/src/field/oneof.rs @@ -65,7 +65,7 @@ impl Field { None => bail!("oneof field is missing a tags attribute"), }; - Ok(Some(Field { ty: ty, tags: tags })) + Ok(Some(Field { ty, tags })) } /// Returns a statement which encodes the oneof field. diff --git a/prost-derive/src/field/scalar.rs b/prost-derive/src/field/scalar.rs index 905c060fd..720fdb555 100644 --- a/prost-derive/src/field/scalar.rs +++ b/prost-derive/src/field/scalar.rs @@ -82,17 +82,13 @@ impl Field { (None, _, _) => Kind::Plain(default), (Some(Label::Optional), _, _) => Kind::Optional(default), (Some(Label::Required), _, _) => Kind::Required(default), - (Some(Label::Repeated), packed, false) if packed.unwrap_or(ty.is_numeric()) => { + (Some(Label::Repeated), packed, false) if packed.unwrap_or_else(|| ty.is_numeric()) => { Kind::Packed } (Some(Label::Repeated), _, false) => Kind::Repeated, }; - Ok(Some(Field { - ty: ty, - kind: kind, - tag: tag, - })) + Ok(Some(Field { ty, kind, tag })) } pub fn new_oneof(attrs: &[Meta]) -> Result, Error> { @@ -584,7 +580,7 @@ pub enum DefaultValue { impl DefaultValue { pub fn from_attr(attr: &Meta) -> Result, Error> { if !attr.path().is_ident("default") { - return Ok(None); + Ok(None) } else if let Meta::NameValue(ref name_value) = *attr { Ok(Some(name_value.lit.clone())) } else { @@ -677,7 +673,7 @@ impl DefaultValue { } // Rust doesn't have a negative literals, so they have to be parsed specially. - if value.chars().next() == Some('-') { + if value.starts_with('-') { if let Ok(lit) = syn::parse_str::(&value[1..]) { match lit { Lit::Int(ref lit) if is_i32 && empty_or_is("i32", lit.suffix()) => { @@ -741,9 +737,7 @@ impl DefaultValue { Ty::Bool => DefaultValue::Bool(false), Ty::String => DefaultValue::String(String::new()), Ty::Bytes => DefaultValue::Bytes(Vec::new()), - Ty::Enumeration(ref path) => { - return DefaultValue::Enumeration(quote!(#path::default())) - } + Ty::Enumeration(ref path) => DefaultValue::Enumeration(quote!(#path::default())), } } diff --git a/protobuf/Cargo.toml b/protobuf/Cargo.toml index e08aa7507..b82ef734d 100644 --- a/protobuf/Cargo.toml +++ b/protobuf/Cargo.toml @@ -11,6 +11,7 @@ prost = { path = ".." } prost-types = { path = "../prost-types" } [build-dependencies] +anyhow = "1" cfg-if = "0.1" curl = "0.4" flate2 = "1.0" diff --git a/protobuf/benches/dataset.rs b/protobuf/benches/dataset.rs index e02071030..86a2d81fe 100644 --- a/protobuf/benches/dataset.rs +++ b/protobuf/benches/dataset.rs @@ -7,8 +7,8 @@ use criterion::{criterion_group, criterion_main, Criterion}; use prost::Message; use protobuf::benchmarks::{ - dataset, google_message3::GoogleMessage3, google_message4::GoogleMessage4, proto2, proto3, - BenchmarkDataset, + dataset, google_message3::GoogleMessage3, /*google_message4::GoogleMessage4,*/ proto2, + proto3, BenchmarkDataset, }; fn load_dataset(dataset: &Path) -> Result> { @@ -80,12 +80,12 @@ macro_rules! dataset { dataset!(google_message1_proto2, proto2::GoogleMessage1); dataset!(google_message1_proto3, proto3::GoogleMessage1); dataset!(google_message2, proto2::GoogleMessage2); -dataset!(google_message3_1, GoogleMessage3); +//dataset!(google_message3_1, GoogleMessage3); dataset!(google_message3_2, GoogleMessage3); dataset!(google_message3_3, GoogleMessage3); dataset!(google_message3_4, GoogleMessage3); -dataset!(google_message3_5, GoogleMessage3); -dataset!(google_message4, GoogleMessage4); +//dataset!(google_message3_5, GoogleMessage3); +//dataset!(google_message4, GoogleMessage4); criterion_group!( dataset, @@ -100,10 +100,14 @@ criterion_group! { targets = google_message3_2, google_message3_4, google_message3_3 } +// TODO: Criterion now requires a sample_size of 10; figure out a better way to +// get these tests to run in a reasonable time. +/* criterion_group! { name = extra_slow; - config = Criterion::default().sample_size(3); + config = Criterion::default().sample_size(10); targets = google_message3_1, google_message3_5, google_message4 } +*/ -criterion_main!(dataset, slow, extra_slow); +criterion_main!(dataset, slow /*, extra_slow*/); diff --git a/protobuf/build.rs b/protobuf/build.rs index 03f0380c1..4762ed823 100644 --- a/protobuf/build.rs +++ b/protobuf/build.rs @@ -8,11 +8,12 @@ use std::io::Cursor; use std::path::{Path, PathBuf}; use std::process::Command; +use anyhow::{ensure, Context, Result}; use curl::easy::Easy; use flate2::bufread::GzDecoder; use tar::Archive; -const VERSION: &'static str = "3.11.2"; +const VERSION: &str = "3.11.2"; static TEST_PROTOS: &[&str] = &[ "test_messages_proto2.proto", @@ -41,7 +42,7 @@ static DATASET_PROTOS: &[&str] = &[ "google_message4/benchmark_message4_3.proto", ]; -fn main() { +fn main() -> Result<()> { let out_dir = &PathBuf::from(env::var("OUT_DIR").expect("OUT_DIR environment variable not set")); let protobuf_dir = &out_dir.join(format!("protobuf-{}", VERSION)); @@ -52,13 +53,13 @@ fn main() { .tempdir_in(out_dir) .expect("failed to create temporary directory"); - let src_dir = &download_protobuf(tempdir.path()); + let src_dir = &download_protobuf(tempdir.path())?; let prefix_dir = &src_dir.join("prefix"); fs::create_dir(prefix_dir).expect("failed to create prefix directory"); - install_conformance_test_runner(src_dir, prefix_dir); - install_protos(src_dir, prefix_dir); - install_datasets(src_dir, prefix_dir); - fs::rename(prefix_dir, protobuf_dir).expect("failed to move protobuf dir"); + install_conformance_test_runner(src_dir, prefix_dir)?; + install_protos(src_dir, prefix_dir)?; + install_datasets(src_dir, prefix_dir)?; + fs::rename(prefix_dir, protobuf_dir).context("failed to move protobuf dir")?; } let include_dir = &protobuf_dir.join("include"); @@ -101,17 +102,18 @@ fn main() { // Emit an environment variable with the path to the build so that it can be located in the // main crate. println!("cargo:rustc-env=PROTOBUF={}", protobuf_dir.display()); + Ok(()) } -fn download_tarball(url: &str, out_dir: &Path) { +fn download_tarball(url: &str, out_dir: &Path) -> Result<()> { let mut data = Vec::new(); let mut handle = Easy::new(); // Download the tarball. - handle.url(url).expect("failed to configure tarball URL"); + handle.url(url).context("failed to configure tarball URL")?; handle .follow_location(true) - .expect("failed to configure follow location"); + .context("failed to configure follow location")?; { let mut transfer = handle.transfer(); transfer @@ -119,29 +121,29 @@ fn download_tarball(url: &str, out_dir: &Path) { data.extend_from_slice(new_data); Ok(new_data.len()) }) - .expect("failed to write download data"); - transfer.perform().expect("failed to download tarball"); + .context("failed to write download data")?; + transfer.perform().context("failed to download tarball")?; } // Unpack the tarball. Archive::new(GzDecoder::new(Cursor::new(data))) .unpack(out_dir) - .expect("failed to unpack tarball"); + .context("failed to unpack tarball") } /// Downloads and unpacks a Protobuf release tarball to the provided directory. -fn download_protobuf(out_dir: &Path) -> PathBuf { +fn download_protobuf(out_dir: &Path) -> Result { download_tarball( &format!( "https://github.com/google/protobuf/archive/v{}.tar.gz", VERSION ), out_dir, - ); + )?; let src_dir = out_dir.join(format!("protobuf-{}", VERSION)); // Apply patches. - let mut patch_src = env::current_dir().expect("failed to get current working directory"); + let mut patch_src = env::current_dir().context("failed to get current working directory")?; patch_src.push("src"); patch_src.push("fix-conformance_test_runner-cmake-build.patch"); @@ -151,49 +153,56 @@ fn download_protobuf(out_dir: &Path) -> PathBuf { .arg(patch_src) .current_dir(&src_dir) .status() - .expect("failed to apply patch"); - assert!(rc.success(), "protobuf patch failed"); + .context("failed to apply patch")?; + ensure!(rc.success(), "protobuf patch failed"); - src_dir + Ok(src_dir) } -fn install_conformance_test_runner(src_dir: &Path, prefix_dir: &Path) { - #[cfg(not(windows))] - { - // Build and install protoc, the protobuf libraries, and the conformance test runner. - let rc = Command::new("cmake") - .arg("-GNinja") - .arg("cmake/") - .arg("-DCMAKE_BUILD_TYPE=DEBUG") - .arg(&format!("-DCMAKE_INSTALL_PREFIX={}", prefix_dir.display())) - .arg("-Dprotobuf_BUILD_CONFORMANCE=ON") - .arg("-Dprotobuf_BUILD_TESTS=OFF") - .current_dir(&src_dir) - .status() - .expect("failed to execute CMake"); - assert!(rc.success(), "protobuf CMake failed"); - - let num_jobs = env::var("NUM_JOBS").expect("NUM_JOBS environment variable not set"); - - let rc = Command::new("ninja") - .arg("-j") - .arg(&num_jobs) - .arg("install") - .current_dir(&src_dir) - .status() - .expect("failed to execute ninja protobuf"); - assert!(rc.success(), "failed to make protobuf"); - - // Install the conformance-test-runner binary, since it isn't done automatically. - fs::rename( - src_dir.join("conformance_test_runner"), - prefix_dir.join("bin").join("conformance-test-runner"), - ) - .expect("failed to move conformance-test-runner"); - } +#[cfg(windows)] +fn install_conformance_test_runner(_: &Path, _: &Path) -> Result<()> { + // The conformance test runner does not support Windows [1]. + // [1]: https://github.com/protocolbuffers/protobuf/tree/master/conformance#portability + Ok(()) +} + +#[cfg(not(windows))] +fn install_conformance_test_runner(src_dir: &Path, prefix_dir: &Path) -> Result<()> { + // Build and install protoc, the protobuf libraries, and the conformance test runner. + let rc = Command::new("cmake") + .arg("-GNinja") + .arg("cmake/") + .arg("-DCMAKE_BUILD_TYPE=DEBUG") + .arg(&format!("-DCMAKE_INSTALL_PREFIX={}", prefix_dir.display())) + .arg("-Dprotobuf_BUILD_CONFORMANCE=ON") + .arg("-Dprotobuf_BUILD_TESTS=OFF") + .current_dir(&src_dir) + .status() + .context("failed to execute CMake")?; + assert!(rc.success(), "protobuf CMake failed"); + + let num_jobs = env::var("NUM_JOBS").context("NUM_JOBS environment variable not set")?; + + let rc = Command::new("ninja") + .arg("-j") + .arg(&num_jobs) + .arg("install") + .current_dir(&src_dir) + .status() + .context("failed to execute ninja protobuf")?; + ensure!(rc.success(), "failed to make protobuf"); + + // Install the conformance-test-runner binary, since it isn't done automatically. + fs::rename( + src_dir.join("conformance_test_runner"), + prefix_dir.join("bin").join("conformance-test-runner"), + ) + .context("failed to move conformance-test-runner")?; + + Ok(()) } -fn install_protos(src_dir: &Path, prefix_dir: &Path) { +fn install_protos(src_dir: &Path, prefix_dir: &Path) -> Result<()> { let include_dir = prefix_dir.join("include"); // Move test protos to the prefix directory. @@ -208,7 +217,7 @@ fn install_protos(src_dir: &Path, prefix_dir: &Path) { .join(proto), test_include_dir.join(proto), ) - .expect(&format!("failed to move {}", proto)); + .with_context(|| format!("failed to move {}", proto))?; } // Move conformance.proto to the install directory. @@ -234,16 +243,19 @@ fn install_protos(src_dir: &Path, prefix_dir: &Path) { .expect("failed to move benchmarks.proto"); for proto in DATASET_PROTOS.iter().map(Path::new) { let dir = &datasets_include_dir.join(proto.parent().unwrap()); - fs::create_dir_all(dir).expect(&format!("unable to create directory {}", dir.display())); + fs::create_dir_all(dir) + .with_context(|| format!("unable to create directory {}", dir.display()))?; fs::rename( datasets_src_dir.join(proto), datasets_include_dir.join(proto), ) - .expect(&format!("failed to move {}", proto.display())); + .with_context(|| format!("failed to move {}", proto.display()))?; } + + Ok(()) } -fn install_datasets(src_dir: &Path, prefix_dir: &Path) { +fn install_datasets(src_dir: &Path, prefix_dir: &Path) -> Result<()> { let share_dir = &prefix_dir.join("share"); fs::create_dir(share_dir).expect("failed to create share directory"); for dataset in &[ @@ -259,11 +271,11 @@ fn install_datasets(src_dir: &Path, prefix_dir: &Path) { src_dir.join("benchmarks").join("datasets").join(dataset), share_dir.join(dataset.file_name().unwrap()), ) - .expect(&format!("failed to move {}", dataset.display())); + .with_context(|| format!("failed to move {}", dataset.display()))?; } download_tarball( "https://storage.googleapis.com/protobuf_opensource_benchmark_data/datasets.tar.gz", share_dir, - ); + ) } diff --git a/protobuf/src/lib.rs b/protobuf/src/lib.rs index 9f8048fb7..db083e6ce 100644 --- a/protobuf/src/lib.rs +++ b/protobuf/src/lib.rs @@ -1,3 +1,5 @@ +#![allow(clippy::large_enum_variant, clippy::unreadable_literal)] + pub mod benchmarks { include!(concat!(env!("OUT_DIR"), "/benchmarks.rs")); diff --git a/src/encoding.rs b/src/encoding.rs index 4ed88882e..18902f134 100644 --- a/src/encoding.rs +++ b/src/encoding.rs @@ -2,6 +2,8 @@ //! //! Meant to be used only from `Message` implementations. +#![allow(clippy::implicit_hasher, clippy::ptr_arg)] + use std::cmp::min; use std::convert::TryFrom; use std::mem; @@ -381,7 +383,12 @@ where Ok(()) } -pub fn skip_field(wire_type: WireType, tag: u32, buf: &mut B, ctx: DecodeContext) -> Result<(), DecodeError> +pub fn skip_field( + wire_type: WireType, + tag: u32, + buf: &mut B, + ctx: DecodeContext, +) -> Result<(), DecodeError> where B: Buf, { @@ -1473,6 +1480,9 @@ mod test { #[test] fn varint() { fn check(value: u64, mut encoded: &[u8]) { + // TODO(rust-lang/rust-clippy#5494) + #![allow(clippy::clone_double_ref)] + // Small buffer. let mut buf = Vec::with_capacity(1); encode_varint(value, &mut buf); @@ -1488,7 +1498,6 @@ mod test { let roundtrip_value = decode_varint(&mut encoded.clone()).expect("decoding failed"); assert_eq!(value, roundtrip_value); - println!("encoding {:?}", encoded); let roundtrip_value = decode_varint_slow(&mut encoded).expect("slow decoding failed"); assert_eq!(value, roundtrip_value); } diff --git a/tests/src/bootstrap.rs b/tests/src/bootstrap.rs index e7d725f6d..6090e90d0 100644 --- a/tests/src/bootstrap.rs +++ b/tests/src/bootstrap.rs @@ -1,3 +1,6 @@ +// protoc on Windows outputs \r\n line endings. +#![cfg(not(target_os = "windows"))] + use std::fs; use std::io::Read; use std::io::Write; diff --git a/tests/src/build.rs b/tests/src/build.rs index f39169512..ce117333a 100644 --- a/tests/src/build.rs +++ b/tests/src/build.rs @@ -13,7 +13,7 @@ use std::fs; use std::path::PathBuf; fn main() { - let _ = env_logger::init(); + env_logger::init(); // The source directory. The indirection is necessary in order to support the tests-2015 crate, // which sets the current directory to tests-2015 during build script evaluation. diff --git a/tests/src/deprecated_field.rs b/tests/src/deprecated_field.rs index 3c3589b42..a07fc1ce2 100644 --- a/tests/src/deprecated_field.rs +++ b/tests/src/deprecated_field.rs @@ -6,7 +6,7 @@ mod deprecated_field { #[test] fn test_warns_when_using_fields_with_deprecated_field() { #[allow(deprecated)] - deprecated_field::Test { + let message = deprecated_field::Test { not_outdated: ".ogg".to_string(), outdated: ".wav".to_string(), }; @@ -23,6 +23,5 @@ fn test_warns_when_using_fields_with_deprecated_field() { // | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ // | // = note: `#[warn(deprecated)]` on by default - - assert!(true); + drop(message); } diff --git a/tests/src/lib.rs b/tests/src/lib.rs index 00854c611..1b85de1fe 100644 --- a/tests/src/lib.rs +++ b/tests/src/lib.rs @@ -1,3 +1,9 @@ +#![allow( + clippy::cognitive_complexity, + clippy::module_inception, + clippy::unreadable_literal +)] + #[macro_use] extern crate cfg_if; @@ -467,7 +473,7 @@ mod tests { #[test] fn test_267_regression() { // Checks that skip_field will error appropriately when given a big stack of StartGroup - // tags. + // tags. When the no-recursion-limit feature is enabled this results in stack overflow. // // https://github.com/danburkert/prost/issues/267 let buf = vec![b'C'; 1 << 20]; diff --git a/tests/src/unittest.rs b/tests/src/unittest.rs index 1b4c6cb4f..6a1a03af2 100644 --- a/tests/src/unittest.rs +++ b/tests/src/unittest.rs @@ -1,3 +1,5 @@ +#![allow(clippy::float_cmp)] + #[test] fn extreme_default_values() { use protobuf::test_messages::protobuf_unittest;