From 99fd54a6ad884565218ad2290bae611940e8108a Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Fri, 23 Aug 2024 16:14:47 -0700 Subject: [PATCH] Move -Werror to our test/dev bazelrc files. (#17939) * 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 27.x * Fix zlib issues on macos * Fix spurious upb warnings in 27.x * 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 | 1 + conformance/binary_json_conformance_suite.cc | 7 ++-- python/map.c | 10 ------ python/message.c | 1 - python/protobuf.c | 2 +- ruby/ext/google/protobuf_c/convert.c | 3 +- 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 +- 18 files changed, 36 insertions(+), 61 deletions(-) diff --git a/.bazelrc b/.bazelrc index 65cc8c491e..8ca6e56dd8 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 1db6c10c88..ab3a5af4b3 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 cab8c4d874..790e78df9d 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 3ae8705d53..cbe5a2c794 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/... //protos/... //protos_generator/... //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/... //protos/... //protos_generator/... //python/... //upb/... //upb_generator/... windows: strategy: diff --git a/build_defs/cpp_opts.bzl b/build_defs/cpp_opts.bzl index 46b60252f6..d8646d1681 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 d5dcf5de00..b4ec98f8c7 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 465bf0957f..284ae690d9 100644 --- a/ci/macOS.bazelrc +++ b/ci/macOS.bazelrc @@ -1,4 +1,5 @@ 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 diff --git a/conformance/binary_json_conformance_suite.cc b/conformance/binary_json_conformance_suite.cc index f8430bceea..85c8e24b0b 100644 --- a/conformance/binary_json_conformance_suite.cc +++ b/conformance/binary_json_conformance_suite.cc @@ -3449,12 +3449,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/python/map.c b/python/map.c index 4b6e97e193..1e2b3806b6 100644 --- a/python/map.c +++ b/python/map.c @@ -259,16 +259,6 @@ static Py_ssize_t PyUpb_MapContainer_Length(PyObject* _self) { return map ? upb_Map_Size(map) : 0; } -static PyUpb_MapContainer* PyUpb_MapContainer_Check(PyObject* _self) { - PyUpb_ModuleState* state = PyUpb_ModuleState_Get(); - if (!PyObject_TypeCheck(_self, state->message_map_container_type) && - !PyObject_TypeCheck(_self, state->scalar_map_container_type)) { - PyErr_Format(PyExc_TypeError, "Expected protobuf map, but got %R", _self); - return NULL; - } - return (PyUpb_MapContainer*)_self; -} - int PyUpb_Message_InitMapAttributes(PyObject* map, PyObject* value, const upb_FieldDef* f); diff --git a/python/message.c b/python/message.c index c0c0882d24..6ace35ca20 100644 --- a/python/message.c +++ b/python/message.c @@ -1593,7 +1593,6 @@ static PyObject* PyUpb_Message_WhichOneof(PyObject* _self, PyObject* name) { } PyObject* DeepCopy(PyObject* _self, PyObject* arg) { - PyUpb_Message* self = (void*)_self; const upb_MessageDef* def = PyUpb_Message_GetMsgdef(_self); const upb_MiniTable* mini_table = upb_MessageDef_MiniTable(def); upb_Message* msg = PyUpb_Message_GetIfReified(_self); diff --git a/python/protobuf.c b/python/protobuf.c index 316b1f6603..8cdce61a37 100644 --- a/python/protobuf.c +++ b/python/protobuf.c @@ -240,7 +240,7 @@ static void* upb_trim_allocfunc(upb_alloc* alloc, void* ptr, size_t oldsize, } } static upb_alloc trim_alloc = {&upb_trim_allocfunc}; -static const upb_alloc* global_alloc = &trim_alloc; +static upb_alloc* global_alloc = &trim_alloc; // end:github_only static upb_Arena* PyUpb_NewArena(void) { diff --git a/ruby/ext/google/protobuf_c/convert.c b/ruby/ext/google/protobuf_c/convert.c index 1fae52446f..8231c53c89 100644 --- a/ruby/ext/google/protobuf_c/convert.c +++ b/ruby/ext/google/protobuf_c/convert.c @@ -204,7 +204,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/src/google/protobuf/BUILD.bazel b/src/google/protobuf/BUILD.bazel index 6ac0549f5c..842075f392 100644 --- a/src/google/protobuf/BUILD.bazel +++ b/src/google/protobuf/BUILD.bazel @@ -1038,6 +1038,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 6211650e7e..bea6f12c16 100644 --- a/src/google/protobuf/descriptor_database_unittest.cc +++ b/src/google/protobuf/descriptor_database_unittest.cc @@ -447,14 +447,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)); #endif // GTEST_HAS_PARAM_TEST diff --git a/src/google/protobuf/string_view_test.cc b/src/google/protobuf/string_view_test.cc index 1cdb860d94..d348f1fbee 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 faab8f5e8b..85a97c5d6a 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 392d924848..b7378d402b 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 c28d1457e4..c1c5dc76cc 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());