Breaking change: make protobuf comply with the C++ layering check#15212
Merged
1 commit merged intoJan 3, 2024
Merged
Conversation
2468643 to
5ae17bc
Compare
0b8e94b to
71b2233
Compare
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
71b2233 to
a7b0421
Compare
a7b0421
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Breaking change: make protobuf comply with the C++ layering check
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.