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

Update protobuf-javascript to support Protobuf 25.2 #196

Merged
merged 2 commits into from
Apr 18, 2024

Conversation

eric-skydio
Copy link
Contributor

This PR updates the code in protobuf-javascript to compile against the latest protobuf versions.

This primarily entails:

  • Using absl:: methods instead of the previous vendored ones in Protobuf
  • Explicitly using legacy descriptors when important properties are missing
  • Explicitly deleting constructors rather than relying on the deleted GOOGLE_DISALLOW_EVIL_CONSTRUCTORS

@eric-skydio
Copy link
Contributor Author

This could be required if you see errors like this after a protobuf upgrade:

In file included from external/protobuf_javascript/generator/protoc-gen-js.cc:33:0:
external/protobuf_javascript/generator/js_generator.h:39:10: fatal error: google/protobuf/stubs/logging.h: No such file or directory
 #include <google/protobuf/stubs/logging.h>
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

@@ -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 &&
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@eric-skydio
Copy link
Contributor Author

eric-skydio commented Apr 5, 2024

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.

@@ -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")
Copy link
Contributor Author

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.

Copy link
Contributor

@luangong luangong Apr 7, 2024

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",
)

generator/js_generator.cc Outdated Show resolved Hide resolved
@dibenede dibenede merged commit e66d4eb into protocolbuffers:main Apr 18, 2024
4 checks passed
eric-skydio added a commit to eric-skydio/protobuf-javascript that referenced this pull request Apr 18, 2024
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.
eric-skydio added a commit to eric-skydio/protobuf-javascript that referenced this pull request Apr 19, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants