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: 582400675
protocolbuffers · Aug 23, 2024 · d973446 · d973446
1 parent 79781ed
commit d973446
Show file tree

Hide file tree

Showing 22 changed files with 47 additions and 63 deletions.
diff --git 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
@@ -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
@@ -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
@@ -21,7 +21,6 @@ COPTS = select({
         "-Woverloaded-virtual",
         "-Wno-sign-compare",
         "-Wno-nonnull",
-        "-Werror",
     ],
 })
 

diff --git 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
@@ -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
diff --git a/conformance/binary_json_conformance_suite.cc b/conformance/binary_json_conformance_suite.cc
@@ -3501,12 +3501,11 @@ BinaryAndJsonConformanceSuiteImpl<MessageType>::GetFieldForOneofType(
 template <typename MessageType>
 std::string BinaryAndJsonConformanceSuiteImpl<MessageType>::SyntaxIdentifier()
     const {
-  if constexpr (std::is_same<MessageType, TestAllTypesProto2>::value) {
+  if (std::is_same<MessageType, TestAllTypesProto2>::value) {
     return "Proto2";
-  } else if constexpr (std::is_same<MessageType, TestAllTypesProto3>::value) {
+  } else if (std::is_same<MessageType, TestAllTypesProto3>::value) {
     return "Proto3";
-  } else if constexpr (std::is_same<MessageType,
-                                    TestAllTypesProto2Editions>::value) {
+  } else if (std::is_same<MessageType, TestAllTypesProto2Editions>::value) {
     return "Editions_Proto2";
   } else {
     return "Editions_Proto3";

diff --git 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
@@ -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<std::string> 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
@@ -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
@@ -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
@@ -108,7 +108,7 @@ void IterGet(const internal::UntypedMapIterator* iter,
   internal::NodeBase* node = iter->node_;
   if constexpr (std::is_same<Key, PtrAndLen>::value) {
     const std::string* s = static_cast<const std::string*>(node->GetVoidKey());
-    *key = PtrAndLen(s->data(), s->size());
+    *key = PtrAndLen{s->data(), s->size()};
   } else {
     *key = *static_cast<const Key*>(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
@@ -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
@@ -77,7 +77,7 @@ expose_repeated_field_methods(int64_t, i64);
   google::protobuf::rust::PtrAndLen proto2_rust_RepeatedField_##ty##_get(      \
       google::protobuf::RepeatedPtrField<std::string>* 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<std::string>* r, size_t index,        \

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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<std::string>(&message, field)) {
     it.append(it);
   }
+  PROTOBUF_IGNORE_DEPRECATION_STOP;
   {
     const auto& rep_str =
         reflection->GetRepeatedFieldRef<std::string>(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
@@ -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
@@ -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
@@ -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());