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
protocolbuffers · Aug 23, 2024 · b398481 · b398481
1 parent 0c9e14a
commit b398481
Show file tree

Hide file tree

Showing 19 changed files with 37 additions and 55 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
@@ -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
@@ -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
@@ -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
@@ -3470,12 +3470,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/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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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:
-// //  $ 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());