Skip to content

Commit

Permalink
Move -Werror to our test/dev bazelrc files. (#17938)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
mkruskal-google authored Aug 23, 2024
1 parent 0c9e14a commit f5a1b17
Show file tree
Hide file tree
Showing 20 changed files with 41 additions and 57 deletions.
3 changes: 3 additions & 0 deletions .bazelrc
Original file line number Diff line number Diff line change
@@ -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
Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/test_objectivec.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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 }} \
Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/test_rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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/...
5 changes: 4 additions & 1 deletion .github/workflows/test_upb.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
1 change: 0 additions & 1 deletion build_defs/cpp_opts.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ COPTS = select({
"-Woverloaded-virtual",
"-Wno-sign-compare",
"-Wno-nonnull",
"-Werror",
],
})

Expand Down
1 change: 1 addition & 0 deletions ci/Linux.bazelrc
Original file line number Diff line number Diff line change
@@ -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"
3 changes: 2 additions & 1 deletion ci/macOS.bazelrc
Original file line number Diff line number Diff line change
@@ -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
common --xcode_version_config=@com_google_protobuf//.github:host_xcodes
7 changes: 3 additions & 4 deletions conformance/binary_json_conformance_suite.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
3 changes: 2 additions & 1 deletion ruby/ext/google/protobuf_c/convert.c
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
4 changes: 2 additions & 2 deletions rust/cpp_kernel/map.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
2 changes: 1 addition & 1 deletion rust/cpp_kernel/map.h
Original file line number Diff line number Diff line change
Expand Up @@ -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__
2 changes: 1 addition & 1 deletion rust/cpp_kernel/repeated.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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, \
Expand Down
2 changes: 1 addition & 1 deletion rust/cpp_kernel/strings.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()};
}
}
2 changes: 0 additions & 2 deletions rust/cpp_kernel/strings.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions src/google/protobuf/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
8 changes: 4 additions & 4 deletions src/google/protobuf/descriptor_database_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 7 additions & 0 deletions src/google/protobuf/string_view_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand All @@ -309,3 +314,5 @@ TEST(StringViewFieldTest, RepeatedSetAndGetByReflection) {
} // namespace
} // namespace protobuf
} // namespace google

#include "google/protobuf/port_undef.inc"
4 changes: 4 additions & 0 deletions third_party/zlib.BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
35 changes: 0 additions & 35 deletions upb/io/zero_copy_stream_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion upb/wire/eps_copy_input_stream_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down

0 comments on commit f5a1b17

Please sign in to comment.