From 5695a882bd93b8ae4803f220a448785dc868a876 Mon Sep 17 00:00:00 2001 From: Mike Kruskal Date: Fri, 23 Aug 2024 13:24:13 -0700 Subject: [PATCH] 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 --- .bazelrc | 2 ++ .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 ++-- conformance/conformance_test.cc | 4 +-- conformance/failure_list_trie_node.cc | 6 ++-- conformance/failure_list_trie_node.h | 6 ++-- ruby/ext/google/protobuf_c/convert.c | 3 +- rust/cpp_kernel/map.cc | 6 ++-- 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 +- 22 files changed, 47 insertions(+), 63 deletions(-) diff --git a/.bazelrc b/.bazelrc index 65cc8c491e52..ad5641162fff 100644 --- a/.bazelrc +++ b/.bazelrc @@ -1,5 +1,7 @@ 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" + build:dbg --compilation_mode=dbg build:opt --compilation_mode=opt diff --git a/.github/workflows/test_rust.yml b/.github/workflows/test_rust.yml index ea1f02491b13..4cc8be03935f 100644 --- a/.github/workflows/test_rust.yml +++ b/.github/workflows/test_rust.yml @@ -28,6 +28,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 e367cc365b94..1cb5b63347dc 100644 --- a/.github/workflows/test_upb.yml +++ b/.github/workflows/test_upb.yml @@ -72,7 +72,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..657dcdc5f53b 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" diff --git a/ci/macOS.bazelrc b/ci/macOS.bazelrc index 8e7eaf0fb381..21a44acf3392 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" 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 diff --git a/conformance/binary_json_conformance_suite.cc b/conformance/binary_json_conformance_suite.cc index 835204015393..d2d7a8871874 100644 --- a/conformance/binary_json_conformance_suite.cc +++ b/conformance/binary_json_conformance_suite.cc @@ -3501,12 +3501,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/conformance/conformance_test.cc b/conformance/conformance_test.cc index 1b626cd5f9ca..c3d299865e7e 100644 --- a/conformance/conformance_test.cc +++ b/conformance/conformance_test.cc @@ -531,8 +531,8 @@ bool ConformanceTestSuite::RunTest(const std::string& test_name, // In essence, find what wildcarded test names expand to or direct matches // (without wildcards). - if (auto result = failure_list_root_.WalkDownMatch(test_name); - result.has_value()) { + auto result = failure_list_root_.WalkDownMatch(test_name); + if (result.has_value()) { string matched_equivalent = result.value(); unmatched_.erase(matched_equivalent); TestStatus expansion; diff --git a/conformance/failure_list_trie_node.cc b/conformance/failure_list_trie_node.cc index 4cd464a0b727..95e27d6b5e6b 100644 --- a/conformance/failure_list_trie_node.cc +++ b/conformance/failure_list_trie_node.cc @@ -15,7 +15,8 @@ namespace google { namespace protobuf { absl::Status FailureListTrieNode::Insert(absl::string_view test_name) { - if (auto result = WalkDownMatch(test_name); result.has_value()) { + auto result = WalkDownMatch(test_name); + if (result.has_value()) { return absl::AlreadyExistsError( absl::StrFormat("Test name %s already exists in the trie FROM %s", test_name, result.value())); @@ -74,7 +75,8 @@ absl::optional FailureListTrieNode::WalkDownMatch( return std::string(appended); } } else { - if (auto result = child->WalkDownMatch(to_match); result.has_value()) { + auto result = child->WalkDownMatch(to_match); + if (result.has_value()) { return absl::StrCat(appended, ".", result.value()); } } diff --git a/conformance/failure_list_trie_node.h b/conformance/failure_list_trie_node.h index d6be45fe0116..2c7409d04e7d 100644 --- a/conformance/failure_list_trie_node.h +++ b/conformance/failure_list_trie_node.h @@ -24,11 +24,11 @@ namespace protobuf { // Example of what the trie might look like in practice: // // (root) -// / \ +// / | // "Recommended" "Required" -// / \ +// / | // "Proto2" "*" -// / \ \ +// / | | // "JsonInput" "ProtobufInput" "JsonInput" // // 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 60f53c7df00f..2610836a28dc 100644 --- a/rust/cpp_kernel/map.cc +++ b/rust/cpp_kernel/map.cc @@ -108,7 +108,7 @@ void IterGet(const internal::UntypedMapIterator* iter, internal::NodeBase* node = iter->node_; if constexpr (std::is_same::value) { const std::string* s = static_cast(node->GetVoidKey()); - *key = PtrAndLen(s->data(), s->size()); + *key = PtrAndLen{s->data(), s->size()}; } else { *key = *static_cast(node->GetVoidKey()); } @@ -219,10 +219,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 e568222edb98..42687930cf36 100644 --- a/rust/cpp_kernel/map.h +++ b/rust/cpp_kernel/map.h @@ -116,7 +116,7 @@ auto MakeCleanup(T value) { __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 2ff2248156cc..ff07992ed93f 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 05e14d1f33f6..91e71a2205e8 100644 --- a/src/google/protobuf/BUILD.bazel +++ b/src/google/protobuf/BUILD.bazel @@ -1070,6 +1070,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 42664748e3c9..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: -// // $ bazel 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());