Skip to content

Commit

Permalink
Breaking change: make protobuf comply with the C++ layering check
Browse files Browse the repository at this point in the history
This check enforces that each C++ build target has the correct dependencies for
all headers that it includes. We have many targets that were not correct with
respect to this check, so I fixed them up.

I also cleaned up the C++ targets related to the well-known types. I created a
cc_proto_library() target for each one and removed the :wkt_cc_protos target,
since this was necessary to satisfy the layering check. I deleted the
//src/google/protobuf:protobuf_nowkt target and deprecated :protobuf_nowkt,
because the distinction between the :protobuf and :protobuf_nowkt targets was
not really correct. Neither one exposed the headers for the well-known types in
a way that was valid with respect to the layering check, and the idea of
bundling all the well-known types together is not idiomatic in Bazel anyway.
This is a breaking change, because the //:protobuf target no longer bundles the
well-known types. From now on they should be accessed through the new
//:*_cc_proto aliases in our top-level package.

I renamed the :port_def target to :port, which simplifies things a bit by
matching our internal name.

The original motivation for this change was that to move utf8_range onto our CI
infrastructure, we needed to make its dependency rules_fuzzing compatible with
Bazel 6. The rules_fuzzing project builds with the layering check, and I found
that the process of upgrading it to Bazel 6 made it take a dependency on
protobuf, which caused it to break due to layering violations. I was able to
work around this, but it would still be nice to comply with the layering check
so that we don't have to worry about this kind of thing in the future.

PiperOrigin-RevId: 595516736
  • Loading branch information
acozzette authored and Copybara-Service committed Jan 3, 2024
1 parent b660646 commit a7b0421
Show file tree
Hide file tree
Showing 30 changed files with 745 additions and 124 deletions.
4 changes: 4 additions & 0 deletions .bazelrc
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,7 @@ build:ubsan --copt=-fno-sanitize=function --copt=-fno-sanitize=vptr
# TODO: migrate all dependencies from WORKSPACE to MODULE.bazel
# https://github.com/protocolbuffers/protobuf/issues/14313
common --noenable_bzlmod

# Important: this flag ensures that we remain compliant with the C++ layering
# check.
build --features=layering_check
8 changes: 6 additions & 2 deletions .github/workflows/test_java.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ jobs:
- name: OpenJDK 8
version: '8'
image: us-docker.pkg.dev/protobuf-build/containers/test/linux/java:8-1fdbb997433cb22c1e49ef75ad374a8d6bb88702
targets: //java/... //java/internal:java_version
# TODO: b/318555165 - enable the layering check. Currently it does
# not work correctly with the toolchain in this Docker image.
targets: //java/... //java/internal:java_version --features=-layering_check
- name: OpenJDK 11
version: '11'
image: us-docker.pkg.dev/protobuf-build/containers/test/linux/java:11-1fdbb997433cb22c1e49ef75ad374a8d6bb88702
Expand Down Expand Up @@ -63,7 +65,9 @@ jobs:
image: us-docker.pkg.dev/protobuf-build/containers/test/linux/java:8-1fdbb997433cb22c1e49ef75ad374a8d6bb88702
credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }}
bazel-cache: java_linux/8
bazel: test --test_output=all //java:linkage_monitor --spawn_strategy=standalone
# TODO: b/318555165 - enable the layering check. Currently it does
# not work correctly with the toolchain in this Docker image.
bazel: test --test_output=all //java:linkage_monitor --spawn_strategy=standalone --features=-layering_check

protobuf-bom:
name: Protobuf Maven BOM
Expand Down
88 changes: 70 additions & 18 deletions BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,68 @@ alias(
visibility = ["//visibility:public"],
)

# C++ targets for the well-known types

alias(
name = "any_cc_proto",
actual = "//src/google/protobuf:any_cc_proto",
visibility = ["//visibility:public"],
)

alias(
name = "api_cc_proto",
actual = "//src/google/protobuf:api_cc_proto",
visibility = ["//visibility:public"],
)

alias(
name = "duration_cc_proto",
actual = "//src/google/protobuf:duration_cc_proto",
visibility = ["//visibility:public"],
)

alias(
name = "empty_cc_proto",
actual = "//src/google/protobuf:empty_cc_proto",
visibility = ["//visibility:public"],
)

alias(
name = "field_mask_cc_proto",
actual = "//src/google/protobuf:field_mask_cc_proto",
visibility = ["//visibility:public"],
)

alias(
name = "source_context_cc_proto",
actual = "//src/google/protobuf:source_context_cc_proto",
visibility = ["//visibility:public"],
)

alias(
name = "struct_cc_proto",
actual = "//src/google/protobuf:struct_cc_proto",
visibility = ["//visibility:public"],
)

alias(
name = "timestamp_cc_proto",
actual = "//src/google/protobuf:timestamp_cc_proto",
visibility = ["//visibility:public"],
)

alias(
name = "type_cc_proto",
actual = "//src/google/protobuf:type_cc_proto",
visibility = ["//visibility:public"],
)

alias(
name = "wrappers_cc_proto",
actual = "//src/google/protobuf:wrappers_cc_proto",
visibility = ["//visibility:public"],
)

# Source files: these are aliases to a filegroup, not a `proto_library`.
#
# (This is _probably_ not what you want.)
Expand Down Expand Up @@ -189,9 +251,16 @@ cc_binary(

# Expose the runtime for the proto_lang_toolchain so that it can also be used in
# a user-defined proto_lang_toolchain.
alias(
name = "protobuf",
actual = "//src/google/protobuf",
visibility = ["//visibility:public"],
)

alias(
name = "protobuf_nowkt",
actual = "//src/google/protobuf:protobuf_nowkt",
deprecation = "Use //:protobuf instead",
visibility = ["//visibility:public"],
)

Expand All @@ -210,23 +279,6 @@ alias(
visibility = ["//visibility:public"],
)

cc_library(
name = "protobuf",
copts = COPTS,
linkopts = LINK_OPTS,
visibility = ["//visibility:public"],
deps = [
"//src/google/protobuf",
"//src/google/protobuf/compiler:importer",
"//src/google/protobuf/util:delimited_message_util",
"//src/google/protobuf/util:differencer",
"//src/google/protobuf/util:field_mask_util",
"//src/google/protobuf/util:json_util",
"//src/google/protobuf/util:time_util",
"//src/google/protobuf/util:type_resolver_util",
],
)

# This provides just the header files for use in projects that need to build
# shared libraries for dynamic loading. This target is available until Bazel
# adds native support for such use cases.
Expand Down Expand Up @@ -324,7 +376,7 @@ proto_lang_toolchain(
"//:descriptor_proto",
],
command_line = "--cpp_out=$(OUT)",
runtime = "//src/google/protobuf:protobuf_nowkt",
runtime = "//src/google/protobuf",
visibility = ["//visibility:public"],
)

Expand Down
6 changes: 5 additions & 1 deletion ci/common.bazelrc
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,8 @@ build --incompatible_use_host_features

# TODO: migrate all dependencies from WORKSPACE to MODULE.bazel
# https://github.com/protocolbuffers/protobuf/issues/14313
common --noenable_bzlmod
common --noenable_bzlmod

# Important: this flag ensures that we remain compliant with the C++ layering
# check.
build --features=layering_check
31 changes: 27 additions & 4 deletions conformance/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
# Conformance testing for Protobuf.

load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_library", "cc_proto_library", "objc_library")
load("@rules_ruby//ruby:defs.bzl", "ruby_binary")
load("//ruby:defs.bzl", "internal_ruby_proto_library")
load("//:protobuf.bzl", "internal_csharp_proto_library", "internal_objc_proto_library", "internal_php_proto_library", "internal_py_proto_library")
load("//build_defs:internal_shell.bzl", "inline_sh_binary")
load(
"@rules_pkg//:mappings.bzl",
"pkg_filegroup",
"pkg_files",
"strip_prefix",
)
load("@rules_ruby//ruby:defs.bzl", "ruby_binary")
load("//:protobuf.bzl", "internal_csharp_proto_library", "internal_objc_proto_library", "internal_php_proto_library", "internal_py_proto_library")
load("//build_defs:internal_shell.bzl", "inline_sh_binary")
load("//ruby:defs.bzl", "internal_ruby_proto_library")

exports_files([
"bazel_conformance_test_runner.sh",
Expand Down Expand Up @@ -144,9 +144,15 @@ cc_library(
includes = ["."],
deps = [
":conformance_cc_proto",
"//src/google/protobuf",
"//src/google/protobuf:protobuf_lite",
"//src/google/protobuf/util:differencer",
"//src/google/protobuf/util:json_util",
"//src/google/protobuf/util:type_resolver_util",
"@com_google_absl//absl/container:btree",
"@com_google_absl//absl/container:flat_hash_set",
"@com_google_absl//absl/log:absl_check",
"@com_google_absl//absl/log:absl_log",
"@com_google_absl//absl/strings",
"@com_google_absl//absl/strings:str_format",
],
Expand All @@ -158,13 +164,21 @@ cc_library(
srcs = ["binary_json_conformance_suite.cc"],
hdrs = ["binary_json_conformance_suite.h"],
deps = [
":conformance_cc_proto",
":conformance_test",
":test_messages_proto2_proto_cc",
":test_messages_proto3_proto_cc",
"//src/google/protobuf",
"//src/google/protobuf:protobuf_lite",
"//src/google/protobuf/editions:test_messages_proto2_editions_cc_proto",
"//src/google/protobuf/editions:test_messages_proto3_editions_cc_proto",
"//src/google/protobuf/json",
"//src/google/protobuf/util:type_resolver_util",
"@com_google_absl//absl/log:absl_check",
"@com_google_absl//absl/log:absl_log",
"@com_google_absl//absl/log:die_if_null",
"@com_google_absl//absl/status",
"@com_google_absl//absl/strings",
"@jsoncpp",
],
)
Expand All @@ -178,6 +192,7 @@ cc_library(
":conformance_test",
":test_messages_proto2_proto_cc",
":test_messages_proto3_proto_cc",
"//src/google/protobuf",
"//src/google/protobuf/editions:test_messages_proto2_editions_cc_proto",
"//src/google/protobuf/editions:test_messages_proto3_editions_cc_proto",
"@com_google_absl//absl/log:absl_log",
Expand Down Expand Up @@ -209,8 +224,16 @@ cc_binary(
"//:protobuf",
"//:test_messages_proto2_cc_proto",
"//:test_messages_proto3_cc_proto",
"//src/google/protobuf",
"//src/google/protobuf:port",
"//src/google/protobuf:protobuf_lite",
"//src/google/protobuf/editions:test_messages_proto2_editions_cc_proto",
"//src/google/protobuf/editions:test_messages_proto3_editions_cc_proto",
"//src/google/protobuf/stubs",
"//src/google/protobuf/util:json_util",
"//src/google/protobuf/util:type_resolver_util",
"@com_google_absl//absl/log:absl_check",
"@com_google_absl//absl/log:absl_log",
"@com_google_absl//absl/status",
"@com_google_absl//absl/status:statusor",
],
Expand Down
2 changes: 2 additions & 0 deletions examples/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ cc_binary(
deps = [
":addressbook_cc_proto",
"@com_google_protobuf//:protobuf",
"@com_google_protobuf//src/google/protobuf/util:time_util",
],
)

Expand All @@ -48,6 +49,7 @@ cc_binary(
deps = [
":addressbook_cc_proto",
"@com_google_protobuf//:protobuf",
"@com_google_protobuf//src/google/protobuf/util:time_util",
],
)

Expand Down
3 changes: 3 additions & 0 deletions lua/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ cc_library(
deps = [
"//upb:json",
"//upb:message",
"//upb:port",
"//upb:reflection",
"//upb:text",
"@lua//:liblua",
Expand All @@ -44,7 +45,9 @@ cc_binary(
copts = UPB_DEFAULT_CPPOPTS,
visibility = ["//visibility:public"],
deps = [
"//src/google/protobuf",
"//src/google/protobuf/compiler:code_generator",
"//src/google/protobuf/io:printer",
"@com_google_absl//absl/strings",
],
)
Expand Down
4 changes: 2 additions & 2 deletions pkg/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
load("@rules_pkg//:pkg.bzl", "pkg_zip")
load(
"@rules_pkg//:mappings.bzl",
"pkg_attributes",
"pkg_files",
)
load("@rules_pkg//:pkg.bzl", "pkg_zip")
load("//:protobuf_release.bzl", "package_naming")
load(":build_systems.bzl", "gen_file_lists")
load(":cc_dist_library.bzl", "cc_dist_library")
Expand Down Expand Up @@ -163,9 +163,9 @@ cc_dist_library(
}),
tags = ["manual"],
deps = [
"//src/google/protobuf",
"//src/google/protobuf:arena_align",
"//src/google/protobuf:cmake_wkt_cc_proto",
"//src/google/protobuf:protobuf_nowkt",
"//src/google/protobuf/compiler:importer",
"//src/google/protobuf/json",
"//src/google/protobuf/util:delimited_message_util",
Expand Down
2 changes: 2 additions & 0 deletions protos_generator/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ cc_library(
visibility = ["//visibility:private"],
deps = [
"//:protobuf",
"//src/google/protobuf/io",
"@com_google_absl//absl/log:absl_log",
"@com_google_absl//absl/strings",
],
Expand All @@ -96,6 +97,7 @@ cc_library(
visibility = ["//visibility:private"],
deps = [
":output",
"//src/google/protobuf",
"//upb_generator:keywords",
],
)
13 changes: 12 additions & 1 deletion python/build_targets.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,18 @@ def build_targets(name):
],
deps = [
":proto_api",
"//:protobuf",
"//src/google/protobuf",
"//src/google/protobuf:port",
"//src/google/protobuf:protobuf_lite",
"//src/google/protobuf/io",
"//src/google/protobuf/io:tokenizer",
"//src/google/protobuf/stubs:lite",
"//src/google/protobuf/util:differencer",
"@com_google_absl//absl/container:flat_hash_map",
"@com_google_absl//absl/log:absl_check",
"@com_google_absl//absl/log:absl_log",
"@com_google_absl//absl/status",
"@com_google_absl//absl/strings",
] + select({
"//conditions:default": [],
":use_fast_cpp_protos": ["//external:python_headers"],
Expand Down
3 changes: 2 additions & 1 deletion rust/cpp_kernel/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ cc_library(
],
deps = [
":rust_alloc_for_cpp_api", # buildcleaner: keep
"//:protobuf_nowkt",
"//:protobuf",
"//:protobuf_lite",
"@com_google_absl//absl/strings:string_view",
],
)
Expand Down
2 changes: 1 addition & 1 deletion src/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@

# Most rules are under google/protobuf. This package exists for convenience.
load("@rules_pkg//:mappings.bzl", "pkg_filegroup", "pkg_files", "strip_prefix")
load("//upb/cmake:build_defs.bzl", "staleness_test")
load("//conformance:defs.bzl", "conformance_test")
load("//upb/cmake:build_defs.bzl", "staleness_test")

pkg_files(
name = "dist_files",
Expand Down
Loading

0 comments on commit a7b0421

Please sign in to comment.