From f5a1b178ad52c3e64da40caceaa4ca9e51045cb4 Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Fri, 23 Aug 2024 16:47:09 -0700 Subject: [PATCH] Move -Werror to our test/dev bazelrc files. (#17938) * Move -Werror to our test/dev bazelrc files. Putting it into BUILD files unintentionally forces it on all our downstream users. Instead, we just want to enable this during testing and let them choose for themselves in their builds. Note, that this expands the scope of -Werror to our entire repo for CI, so a bunch of fixes and opt-outs had to be applied to get this change passing. Closed #14714 PiperOrigin-RevId: 666903224 * Fix extra warnings on 28.x * Fix zlib issues on macos * Second try at zlib/macos issues * Only disable deprecated-non-prototype on macos-14 --- .bazelrc | 3 ++ .github/workflows/test_objectivec.yml | 3 +- .github/workflows/test_rust.yml | 3 +- .github/workflows/test_upb.yml | 5 ++- build_defs/cpp_opts.bzl | 1 - ci/Linux.bazelrc | 1 + ci/macOS.bazelrc | 3 +- conformance/binary_json_conformance_suite.cc | 7 ++-- ruby/ext/google/protobuf_c/convert.c | 3 +- rust/cpp_kernel/map.cc | 4 +-- rust/cpp_kernel/map.h | 2 +- rust/cpp_kernel/repeated.cc | 2 +- rust/cpp_kernel/strings.cc | 2 +- rust/cpp_kernel/strings.h | 2 -- src/google/protobuf/BUILD.bazel | 1 + .../protobuf/descriptor_database_unittest.cc | 8 ++--- src/google/protobuf/string_view_test.cc | 7 ++++ third_party/zlib.BUILD | 4 +++ upb/io/zero_copy_stream_test.cc | 35 ------------------- upb/wire/eps_copy_input_stream_test.cc | 2 +- 20 files changed, 41 insertions(+), 57 deletions(-) diff --git a/.bazelrc b/.bazelrc index 65cc8c491e52..8ca6e56dd812 100644 --- a/.bazelrc +++ b/.bazelrc @@ -1,5 +1,8 @@ build --cxxopt=-std=c++17 --host_cxxopt=-std=c++17 +build --copt="-Werror" --copt="-Wno-sign-compare" --copt="-Wno-sign-conversion" --copt="-Wno-error=sign-conversion" --copt="-Wno-deprecated-declarations" + + build:dbg --compilation_mode=dbg build:opt --compilation_mode=opt diff --git a/.github/workflows/test_objectivec.yml b/.github/workflows/test_objectivec.yml index 1db6c10c88ab..ab3a5af4b37c 100644 --- a/.github/workflows/test_objectivec.yml +++ b/.github/workflows/test_objectivec.yml @@ -76,6 +76,7 @@ jobs: - OS: macos-14 PLATFORM: "visionos" XCODE: "15.2" + EXTRA_FLAGS: --copt="-Wno-deprecated-non-prototype" name: CocoaPods ${{ matrix.PLATFORM }} ${{ matrix.CONFIGURATION }} runs-on: ${{ matrix.OS }} steps: @@ -91,7 +92,7 @@ jobs: credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }} bazel-cache: cocoapods/${{ matrix.XCODE }} bash: | - ./regenerate_stale_files.sh $BAZEL_FLAGS --xcode_version="${{ matrix.XCODE }}" + ./regenerate_stale_files.sh $BAZEL_FLAGS ${{ matrix.EXTRA_FLAGS }} --xcode_version="${{ matrix.XCODE }}" pod lib lint --verbose \ --configuration=${{ matrix.CONFIGURATION }} \ --platforms=${{ matrix.PLATFORM }} \ diff --git a/.github/workflows/test_rust.yml b/.github/workflows/test_rust.yml index 00c2e4b7bb45..2f7021c0b90a 100644 --- a/.github/workflows/test_rust.yml +++ b/.github/workflows/test_rust.yml @@ -27,6 +27,7 @@ jobs: credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }} bazel-cache: rust_linux bazel: >- - test //rust:protobuf_upb_test //rust:protobuf_cpp_test + test --cxxopt=-std=c++17 --host_cxxopt=-std=c++17 --copt="-Wno-return-type-c-linkage" + //rust:protobuf_upb_test //rust:protobuf_cpp_test //rust/test/rust_proto_library_unit_test:rust_upb_aspect_test //src/google/protobuf/compiler/rust/... diff --git a/.github/workflows/test_upb.yml b/.github/workflows/test_upb.yml index 945b7c01b4f4..7490e75f754d 100644 --- a/.github/workflows/test_upb.yml +++ b/.github/workflows/test_upb.yml @@ -59,7 +59,10 @@ jobs: image: "us-docker.pkg.dev/protobuf-build/containers/test/linux/gcc:12.2-6.3.0-63dd26c0c7a808d92673a3e52e848189d4ab0f17" credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }} bazel-cache: "upb-bazel-gcc" - bazel: test --cxxopt=-std=c++17 --host_cxxopt=-std=c++17 -c opt //bazel/... //benchmarks/... //lua/... //python/... //upb/... //upb_generator/... + bazel: >- + test --cxxopt=-std=c++17 --host_cxxopt=-std=c++17 -c opt + --copt="-Wno-error=maybe-uninitialized" --copt="-Wno-error=attributes" + //bazel/... //benchmarks/... //lua/... //python/... //upb/... //upb_generator/... windows: strategy: diff --git a/build_defs/cpp_opts.bzl b/build_defs/cpp_opts.bzl index 46b60252f62b..d8646d1681fe 100644 --- a/build_defs/cpp_opts.bzl +++ b/build_defs/cpp_opts.bzl @@ -21,7 +21,6 @@ COPTS = select({ "-Woverloaded-virtual", "-Wno-sign-compare", "-Wno-nonnull", - "-Werror", ], }) diff --git a/ci/Linux.bazelrc b/ci/Linux.bazelrc index d5dcf5de005d..b4ec98f8c796 100644 --- a/ci/Linux.bazelrc +++ b/ci/Linux.bazelrc @@ -1,3 +1,4 @@ import common.bazelrc build --cxxopt=-std=c++14 --host_cxxopt=-std=c++14 +build --copt="-Werror" --copt="-Wno-sign-compare" --copt="-Wno-sign-conversion" --copt="-Wno-error=sign-conversion" --copt="-Wno-deprecated-declarations" diff --git a/ci/macOS.bazelrc b/ci/macOS.bazelrc index 8e7eaf0fb381..e1336ca1cd23 100644 --- a/ci/macOS.bazelrc +++ b/ci/macOS.bazelrc @@ -1,5 +1,6 @@ import common.bazelrc build --cxxopt=-std=c++14 --host_cxxopt=-std=c++14 +build --copt="-Werror" --copt="-Wno-sign-compare" --copt="-Wno-sign-conversion" --copt="-Wno-error=sign-conversion" --copt="-Wno-deprecated-declarations" common --repo_env=BAZEL_NO_APPLE_CPP_TOOLCHAIN=1 -common --xcode_version_config=@com_google_protobuf//.github:host_xcodes \ No newline at end of file +common --xcode_version_config=@com_google_protobuf//.github:host_xcodes diff --git a/conformance/binary_json_conformance_suite.cc b/conformance/binary_json_conformance_suite.cc index 1d2c29800608..1d42c08e1468 100644 --- a/conformance/binary_json_conformance_suite.cc +++ b/conformance/binary_json_conformance_suite.cc @@ -3470,12 +3470,11 @@ BinaryAndJsonConformanceSuiteImpl::GetFieldForOneofType( template std::string BinaryAndJsonConformanceSuiteImpl::SyntaxIdentifier() const { - if constexpr (std::is_same::value) { + if (std::is_same::value) { return "Proto2"; - } else if constexpr (std::is_same::value) { + } else if (std::is_same::value) { return "Proto3"; - } else if constexpr (std::is_same::value) { + } else if (std::is_same::value) { return "Editions_Proto2"; } else { return "Editions_Proto3"; diff --git a/ruby/ext/google/protobuf_c/convert.c b/ruby/ext/google/protobuf_c/convert.c index 90db70ebbde8..2b2244b0dcca 100644 --- a/ruby/ext/google/protobuf_c/convert.c +++ b/ruby/ext/google/protobuf_c/convert.c @@ -228,7 +228,8 @@ upb_MessageValue Convert_RubyToUpb(VALUE value, const char* name, ret.uint64_val = NUM2ULL(value); break; default: - break; + rb_raise(cTypeError, "Convert_RubyToUpb(): Unexpected type %d", + (int)type_info.type); } break; default: diff --git a/rust/cpp_kernel/map.cc b/rust/cpp_kernel/map.cc index 2418c7274ccd..b1eca654688c 100644 --- a/rust/cpp_kernel/map.cc +++ b/rust/cpp_kernel/map.cc @@ -31,10 +31,10 @@ __PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE(int64_t, i64, int64_t, __PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE( std::string, ProtoBytes, google::protobuf::rust::PtrAndLen, std::string*, std::move(*value), - google::protobuf::rust::PtrAndLen(cpp_value.data(), cpp_value.size())); + (google::protobuf::rust::PtrAndLen{cpp_value.data(), cpp_value.size()})); __PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE( std::string, ProtoString, google::protobuf::rust::PtrAndLen, std::string*, std::move(*value), - google::protobuf::rust::PtrAndLen(cpp_value.data(), cpp_value.size())); + (google::protobuf::rust::PtrAndLen{cpp_value.data(), cpp_value.size()})); } // extern "C" diff --git a/rust/cpp_kernel/map.h b/rust/cpp_kernel/map.h index 2f0a8a82ab86..5e548949619e 100644 --- a/rust/cpp_kernel/map.h +++ b/rust/cpp_kernel/map.h @@ -85,7 +85,7 @@ __PB_RUST_EXPOSE_SCALAR_MAP_METHODS( \ std::string, ProtoString, google::protobuf::rust::PtrAndLen, \ std::string(key.ptr, key.len), \ - google::protobuf::rust::PtrAndLen(cpp_key.data(), cpp_key.size()), value_ty, \ + (google::protobuf::rust::PtrAndLen{cpp_key.data(), cpp_key.size()}), value_ty, \ rust_value_ty, ffi_view_ty, ffi_value_ty, to_cpp_value, to_ffi_value); #endif // GOOGLE_PROTOBUF_RUST_CPP_KERNEL_MAP_H__ diff --git a/rust/cpp_kernel/repeated.cc b/rust/cpp_kernel/repeated.cc index 79654fbad207..bb9ad004c791 100644 --- a/rust/cpp_kernel/repeated.cc +++ b/rust/cpp_kernel/repeated.cc @@ -77,7 +77,7 @@ expose_repeated_field_methods(int64_t, i64); google::protobuf::rust::PtrAndLen proto2_rust_RepeatedField_##ty##_get( \ google::protobuf::RepeatedPtrField* r, size_t index) { \ const std::string& s = r->Get(index); \ - return google::protobuf::rust::PtrAndLen(s.data(), s.size()); \ + return google::protobuf::rust::PtrAndLen{s.data(), s.size()}; \ } \ void proto2_rust_RepeatedField_##ty##_set( \ google::protobuf::RepeatedPtrField* r, size_t index, \ diff --git a/rust/cpp_kernel/strings.cc b/rust/cpp_kernel/strings.cc index c835a8a2219e..af29ddeaf430 100644 --- a/rust/cpp_kernel/strings.cc +++ b/rust/cpp_kernel/strings.cc @@ -33,6 +33,6 @@ std::string* proto2_rust_cpp_new_string(google::protobuf::rust::PtrAndLen src) { void proto2_rust_cpp_delete_string(std::string* str) { delete str; } google::protobuf::rust::PtrAndLen proto2_rust_cpp_string_to_view(std::string* str) { - return google::protobuf::rust::PtrAndLen(str->data(), str->length()); + return google::protobuf::rust::PtrAndLen{str->data(), str->length()}; } } diff --git a/rust/cpp_kernel/strings.h b/rust/cpp_kernel/strings.h index 8b51cdedb0a4..b3302a191972 100644 --- a/rust/cpp_kernel/strings.h +++ b/rust/cpp_kernel/strings.h @@ -22,8 +22,6 @@ struct PtrAndLen { /// Borrows the memory. const char* ptr; size_t len; - - PtrAndLen(const char* ptr, size_t len) : ptr(ptr), len(len) {} }; // Represents an owned string for FFI purposes. diff --git a/src/google/protobuf/BUILD.bazel b/src/google/protobuf/BUILD.bazel index ce1d5e8dbee3..e845839f40f2 100644 --- a/src/google/protobuf/BUILD.bazel +++ b/src/google/protobuf/BUILD.bazel @@ -1047,6 +1047,7 @@ cc_test( name = "string_view_test", srcs = ["string_view_test.cc"], deps = [ + ":port", ":protobuf", ":unittest_string_view_cc_proto", "@com_google_absl//absl/strings:string_view", diff --git a/src/google/protobuf/descriptor_database_unittest.cc b/src/google/protobuf/descriptor_database_unittest.cc index 80e907def90a..7b24257244e8 100644 --- a/src/google/protobuf/descriptor_database_unittest.cc +++ b/src/google/protobuf/descriptor_database_unittest.cc @@ -446,14 +446,14 @@ TEST_P(DescriptorDatabaseTest, ConflictingExtensionError) { " extendee: \".Foo\" }"); } -INSTANTIATE_TEST_CASE_P( +INSTANTIATE_TEST_SUITE_P( Simple, DescriptorDatabaseTest, testing::Values(&SimpleDescriptorDatabaseTestCase::New)); -INSTANTIATE_TEST_CASE_P( +INSTANTIATE_TEST_SUITE_P( MemoryConserving, DescriptorDatabaseTest, testing::Values(&EncodedDescriptorDatabaseTestCase::New)); -INSTANTIATE_TEST_CASE_P(Pool, DescriptorDatabaseTest, - testing::Values(&DescriptorPoolDatabaseTestCase::New)); +INSTANTIATE_TEST_SUITE_P(Pool, DescriptorDatabaseTest, + testing::Values(&DescriptorPoolDatabaseTestCase::New)); TEST(EncodedDescriptorDatabaseExtraTest, FindNameOfFileContainingSymbol) { // Create two files, one of which is in two parts. diff --git a/src/google/protobuf/string_view_test.cc b/src/google/protobuf/string_view_test.cc index 1cdb860d9441..d348f1fbee87 100644 --- a/src/google/protobuf/string_view_test.cc +++ b/src/google/protobuf/string_view_test.cc @@ -12,6 +12,9 @@ #include "google/protobuf/text_format.h" #include "google/protobuf/unittest_string_view.pb.h" +// Must be included last. +#include "google/protobuf/port_def.inc" + namespace google { namespace protobuf { namespace { @@ -284,10 +287,12 @@ TEST(StringViewFieldTest, RepeatedSetAndGetByReflection) { } // MutableRepeatedPtrField(). + PROTOBUF_IGNORE_DEPRECATION_START; for (auto& it : *reflection->MutableRepeatedPtrField(&message, field)) { it.append(it); } + PROTOBUF_IGNORE_DEPRECATION_STOP; { const auto& rep_str = reflection->GetRepeatedFieldRef(message, field); @@ -309,3 +314,5 @@ TEST(StringViewFieldTest, RepeatedSetAndGetByReflection) { } // namespace } // namespace protobuf } // namespace google + +#include "google/protobuf/port_undef.inc" diff --git a/third_party/zlib.BUILD b/third_party/zlib.BUILD index faab8f5e8ba0..85a97c5d6ab7 100644 --- a/third_party/zlib.BUILD +++ b/third_party/zlib.BUILD @@ -59,6 +59,10 @@ cc_library( hdrs = _ZLIB_PREFIXED_HEADERS, copts = select({ "@platforms//os:windows": [], + "@platforms//os:macos": [ + "-Wno-unused-variable", + "-Wno-implicit-function-declaration", + ], "//conditions:default": [ "-Wno-deprecated-non-prototype", "-Wno-unused-variable", diff --git a/upb/io/zero_copy_stream_test.cc b/upb/io/zero_copy_stream_test.cc index 392d924848b5..b7378d402b61 100644 --- a/upb/io/zero_copy_stream_test.cc +++ b/upb/io/zero_copy_stream_test.cc @@ -45,12 +45,6 @@ class IoTest : public testing::Test { // WriteStuff() writes. void ReadStuff(upb_ZeroCopyInputStream* input, bool read_eof = true); - // Similar to WriteStuff, but performs more sophisticated testing. - int WriteStuffLarge(upb_ZeroCopyOutputStream* output); - // Reads and tests a stream that should have been written to - // via WriteStuffLarge(). - void ReadStuffLarge(upb_ZeroCopyInputStream* input); - static const int kBlockSizes[]; static const int kBlockSizeCount; }; @@ -157,35 +151,6 @@ void IoTest::ReadStuff(upb_ZeroCopyInputStream* input, bool read_eof) { } } -int IoTest::WriteStuffLarge(upb_ZeroCopyOutputStream* output) { - WriteString(output, "Hello world!\n"); - WriteString(output, "Some te"); - WriteString(output, "xt. Blah blah."); - WriteString(output, std::string(100000, 'x')); // A very long string - WriteString(output, std::string(100000, 'y')); // A very long string - WriteString(output, "01234567890123456789"); - - const int result = upb_ZeroCopyOutputStream_ByteCount(output); - EXPECT_EQ(result, 200055); - return result; -} - -// Reads text from an input stream and expects it to match what WriteStuff() -// writes. -void IoTest::ReadStuffLarge(upb_ZeroCopyInputStream* input) { - ReadString(input, "Hello world!\nSome text. "); - EXPECT_TRUE(upb_ZeroCopyInputStream_Skip(input, 5)); - ReadString(input, "blah."); - EXPECT_TRUE(upb_ZeroCopyInputStream_Skip(input, 100000 - 10)); - ReadString(input, std::string(10, 'x') + std::string(100000 - 20000, 'y')); - EXPECT_TRUE(upb_ZeroCopyInputStream_Skip(input, 20000 - 10)); - ReadString(input, "yyyyyyyyyy01234567890123456789"); - EXPECT_EQ(upb_ZeroCopyInputStream_ByteCount(input), 200055); - - uint8_t byte; - EXPECT_EQ(ReadFromInput(input, &byte, 1), 0); -} - // =================================================================== TEST_F(IoTest, ArrayIo) { diff --git a/upb/wire/eps_copy_input_stream_test.cc b/upb/wire/eps_copy_input_stream_test.cc index c28d1457e474..c1c5dc76cc50 100644 --- a/upb/wire/eps_copy_input_stream_test.cc +++ b/upb/wire/eps_copy_input_stream_test.cc @@ -281,7 +281,7 @@ TEST(EpsCopyInputStreamTest, ZeroSize) { // } // // // Test with: -// // $ blaze run --config=fuzztest third_party/upb:eps_copy_input_stream_test \ +// // $ bazel run --config=fuzztest third_party/upb:eps_copy_input_stream_test // // -- --gunit_fuzz= // FUZZ_TEST(EpsCopyFuzzTest, TestAgainstFakeStream) // .WithDomains(ArbitraryEpsCopyTestScript());