Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bazel/C++] Move core C++ implementation to //src/google/protobuf package. #9988

Merged
merged 13 commits into from
May 18, 2022

Conversation

dlj-NaN
Copy link
Contributor

@dlj-NaN dlj-NaN commented May 18, 2022

This is somewhat of a rough cut, since it doesn't split apart the lite and full targets, or unit tests.

@dlj-NaN dlj-NaN requested review from fowles and haberman May 18, 2022 03:31
@dlj-NaN dlj-NaN marked this pull request as ready for review May 18, 2022 03:31
# Built-in runtime types
# Source files: these are aliases to a filegroup, not a `proto_library`.
#
# (This is _probably_ not what you want.)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this in place just for compatibility? Do we have specific users that we know of? Can we remove this once we burn them down?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the old name lite_well_known_protos, it's compatibility and that name can probably go away soon-ish. But I think we will probably need to provide access to the sources for the foreseeable future (so well_known_type_protos probably ought to stick around), and I sort of think that the root package is the right place to put it. But that can be a future change.

BUILD.bazel Outdated
visibility = ["//visibility:public"],
)

# adapt_proto_library(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, thanks. Removed it.

Copy link
Contributor

@fowles fowles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo the commented out bit

@dlj-NaN dlj-NaN merged commit 171a6b1 into protocolbuffers:main May 18, 2022
@dlj-NaN dlj-NaN deleted the bazel5 branch May 18, 2022 20:45
@johanbrandhorst
Copy link
Contributor

I think this PR broke existing Bazel users depending on the well known types in this repository. The error I'm seeing in my repository is

no such target '@com_google_protobuf//:well_known_protos': target 'well_known_protos' not declared in package '' defined by /home/vscode/.cache/_grpc_gateway_bazel/external/com_google_protobuf/BUILD.bazel

. I fixed it by changing the reference to @com_google_protobuf//:well_known_type_protos. It would be nice if this change was added as a note to the release notes, so users can triage these issues themselves.

@dlj-NaN
Copy link
Contributor Author

dlj-NaN commented May 26, 2022

I think this PR broke existing Bazel users depending on the well known types in this repository. The error I'm seeing in my repository is

no such target '@com_google_protobuf//:well_known_protos': target 'well_known_protos' not declared in package '' defined by /home/vscode/.cache/_grpc_gateway_bazel/external/com_google_protobuf/BUILD.bazel

. I fixed it by changing the reference to @com_google_protobuf//:well_known_type_protos. It would be nice if this change was added as a note to the release notes, so users can triage these issues themselves.

Sorry about that... the "well_known_protos" target went away in #9915, to distinguish between the Well-Known Types and the built-in runtime protos. Your fix sounds correct, if the WKTs are what you want. Depending on how you use the WKTs, though, you might be able to use the finer-grained targets, too.

I'll go ahead and add this target back with a deprecation warning (see #10061).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants