-
Notifications
You must be signed in to change notification settings - Fork 72
Update protobuf-javascript to support Protobuf 25.2 #196
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
Conversation
This could be required if you see errors like this after a protobuf upgrade:
|
@@ -980,7 +985,7 @@ bool DeclaredReturnTypeIsNullable(const GeneratorOptions& options, | |||
return false; | |||
} | |||
|
|||
if (field->file()->syntax() == FileDescriptor::SYNTAX_PROTO3 && | |||
if (FileDescriptorLegacy(field->file()).syntax() == FileDescriptorLegacy::Syntax::SYNTAX_PROTO3 && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the legacy descriptors needed for? I haven't seen them used before and the inclusion the descriptor_legacy.h seems to be causing a build issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that field->file()->syntax()
is deprecated and no longer available in the FileDescriptor class. Using FileDescriptorLegacy seemed like the easiest and lowest-risk fix since I'm not entirely sure what this is actually trying to check. Similar concerns showed up a couple other places.
It looks like the first version of this failed because I didn't actually update the bzlmod dependency, so CI was building against an old version of protobuf. This didn't come up in my local testing since our repo pulls in newer protobuf through other mechanisms. |
3b675fd
to
7effd01
Compare
@@ -1,4 +1,4 @@ | |||
module(name = "protobuf_javascript", version = "3.21.2") | |||
|
|||
bazel_dep(name = "protobuf", version = "21.7", repo_name = "com_google_protobuf") | |||
bazel_dep(name = "protobuf", version = "23.1", repo_name = "com_google_protobuf") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that 23.1 is the latest available through bzlmod, but this is tested to also work with 25.x on our internal system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use a specific version of the module even if it’s not yet published to BCR with git_override
, like this:
bazel_dep(name = "protobuf", version = "25.3", repo_name = "com_google_protobuf")
git_override(
module_name = "protobuf",
remote = "https://github.com/protocolbuffers/protobuf.git",
# https://github.com/protocolbuffers/protobuf/releases/tag/v25.3
commit = "4a2aef570deb2bfb8927426558701e8bfc26f2a4",
)
or with archive_override
, like this:
bazel_dep(name = "protobuf", version = "25.3", repo_name = "com_google_protobuf")
archive_override(
module_name = "protobuf",
urls = ["https://github.com/protocolbuffers/protobuf/releases/download/v25.3/protobuf-25.3.tar.gz"],
integrity = "sha256-0ZZD0mW5eDgzUrMUPwTAZB7qdadSNcERzAGhNQFzGA4=",
strip_prefix = "protobuf-25.3",
)
This fixes a very minor aesthetic regression associated with newer protobuf versions protocolbuffers#196 Apparently the behavior of printer->Print has changed subtly in how it strips leading spaces.
This fixes a very minor aesthetic regression associated with newer protobuf versions protocolbuffers#196 Apparently the behavior of printer->Print has changed subtly in how it strips leading spaces.
This PR updates the code in protobuf-javascript to compile against the latest protobuf versions.
This primarily entails:
GOOGLE_DISALLOW_EVIL_CONSTRUCTORS