protocolbuffers / protobuf-javascript

BSD 3-Clause "New" or "Revised" License
330 stars 66 forks source link

Update 'syntax' and synthetic oneof to support protobuf 27.1 #208

Closed dibenede closed 1 month ago

dibenede commented 1 month ago

The protobuf descriptor API has evolved to remove access to the schema's syntax and direct access to a oneof's synthetic-ness.

Fixes #206

easternmotors commented 4 weeks ago

This commit seems to have broken my build pipeline. I created a minimal repro (code was inspired by this comment inside a Docker image to avoid any external variables which could cause issues:

FROM node:20-bullseye

ARG BAZEL_VERSION="7.2.0"
ARG COMMIT_SHA="45fbfcb"

# Install necessary Debian packages
RUN apt-get update
RUN apt-get install curl git -y

# Download Bazel
RUN ARCHITECTURE=$(arch | sed s/aarch64/arm64/) && \
    curl --location https://github.com/bazelbuild/bazel/releases/download/$BAZEL_VERSION/bazel-$BAZEL_VERSION-linux-$ARCHITECTURE --output /usr/local/bin/bazel
RUN chmod +x /usr/local/bin/bazel

# Clone protobuf-javascript repo
RUN mkdir /tmp/protobuf-javascript
RUN git clone https://github.com/protocolbuffers/protobuf-javascript.git /tmp/protobuf-javascript

# Build protoc-gen-js
RUN cd /tmp/protobuf-javascript && git checkout $COMMIT_SHA && bazel build //generator:protoc-gen-js

If you run docker build --no-cache - < Dockerfile, the Docker image successfully builds (using the commit SHA 45fbfcb which is the commit before this was merged in). If you run docker build --build-arg COMMIT_SHA="893b51d" --no-cache - < Dockerfile (one commit after -- when this was merged into main), the Docker image now fails to build with the following error:

...
30.96 INFO: From Compiling src/google/protobuf/compiler/rust/relative_path.cc:
30.97 external/protobuf~/src/google/protobuf/compiler/rust/relative_path.cc: In member function 'std::string google::protobuf::compiler::rust::RelativePath::Relative(const google::protobuf::compiler::rust::RelativePath&) const':
30.97 external/protobuf~/src/google/protobuf/compiler/rust/relative_path.cc:66:21: warning: comparison of integer expressions of different signedness: 'int' and 'std::vector<absl::lts_20230802::string_view>::size_type' {aka 'long unsigned int'} [-Wsign-compare]
30.97    66 |   for (int i = 0; i < current_segments.size(); ++i) {
30.97       |                   ~~^~~~~~~~~~~~~~~~~~~~~~~~~
31.07 [733 / 1,195] Compiling absl/strings/internal/cord_rep_ring.cc [for tool]; 0s processwrapper-sandbox ... (8 actions, 7 running)
32.15 [743 / 1,195] Compiling absl/status/statusor.cc; 0s processwrapper-sandbox ... (9 actions, 7 running)
33.44 [751 / 1,195] Compiling absl/strings/cord.cc; 1s processwrapper-sandbox ... (9 actions, 7 running)
34.66 [760 / 1,195] Compiling absl/status/status.cc [for tool]; 1s processwrapper-sandbox ... (9 actions, 8 running)
35.67 [769 / 1,195] Compiling absl/flags/reflection.cc; 1s processwrapper-sandbox ... (9 actions, 8 running)
35.77 INFO: From Compiling src/google/protobuf/arena.cc:
35.77 In file included from external/protobuf~/src/google/protobuf/arena.cc:8:
35.77 bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_includes/arena/google/protobuf/arena.h: In static member function 'static T* google::protobuf::Arena::CreateMessage(google::protobuf::Arena*, Args&& ...)':
35.77 bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_includes/arena/google/protobuf/arena.h:183:32: warning: 'static T* google::protobuf::Arena::CreateMessage(google::protobuf::Arena*, Args&& ...)' is deprecated: Use Create [-Wdeprecated-declarations]
35.77   183 |         is_arena_constructable<Type>::value,
35.77       |                                ^~~~
35.77 bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_includes/arena/google/protobuf/arena.h:180:13: note: declared here
35.77   180 |   static T* CreateMessage(Arena* arena, Args&&... args) {
35.77       |             ^~~~~~~~~~~~~
35.77 bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_includes/arena/google/protobuf/arena.h:185:19: warning: 'static T* google::protobuf::Arena::CreateMessage(google::protobuf::Arena*, Args&& ...)' is deprecated: Use Create [-Wdeprecated-declarations]
35.77   185 |     return Create<Type>(arena, std::forward<Args>(args)...);
35.77       |                   ^~~~
35.77 bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_includes/arena/google/protobuf/arena.h:180:13: note: declared here
35.77   180 |   static T* CreateMessage(Arena* arena, Args&&... args) {
35.77       |             ^~~~~~~~~~~~~
36.68 [773 / 1,195] Compiling src/google/protobuf/json/internal/zero_copy_buffered_stream.cc; 1s processwrapper-sandbox ... (9 actions, 8 running)
37.10 INFO: From Compiling src/google/protobuf/any_lite.cc:
37.10 In file included from bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_includes/protobuf_lite/google/protobuf/arenastring.h:19,
37.10                  from bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_includes/protobuf_lite/google/protobuf/any.h:14,
37.10                  from external/protobuf~/src/google/protobuf/any_lite.cc:10:
37.10 bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_includes/protobuf_lite/google/protobuf/arena.h: In static member function 'static T* google::protobuf::Arena::CreateMessage(google::protobuf::Arena*, Args&& ...)':
37.10 bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_includes/protobuf_lite/google/protobuf/arena.h:183:32: warning: 'static T* google::protobuf::Arena::CreateMessage(google::protobuf::Arena*, Args&& ...)' is deprecated: Use Create [-Wdeprecated-declarations]
37.10   183 |         is_arena_constructable<Type>::value,
37.10       |                                ^~~~
37.10 bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_includes/protobuf_lite/google/protobuf/arena.h:180:13: note: declared here
37.10   180 |   static T* CreateMessage(Arena* arena, Args&&... args) {
37.10       |             ^~~~~~~~~~~~~
37.10 bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_includes/protobuf_lite/google/protobuf/arena.h:185:19: warning: 'static T* google::protobuf::Arena::CreateMessage(google::protobuf::Arena*, Args&& ...)' is deprecated: Use Create [-Wdeprecated-declarations]
37.10   185 |     return Create<Type>(arena, std::forward<Args>(args)...);
37.10       |                   ^~~~
37.10 bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_includes/protobuf_lite/google/protobuf/arena.h:180:13: note: declared here
37.10   180 |   static T* CreateMessage(Arena* arena, Args&&... args) {
37.10       |             ^~~~~~~~~~~~~
37.68 [782 / 1,195] Compiling absl/strings/cord.cc [for tool]; 0s processwrapper-sandbox ... (9 actions, 8 running)
38.69 [789 / 1,195] Compiling absl/strings/cord.cc [for tool]; 1s processwrapper-sandbox ... (9 actions, 8 running)
39.55 ERROR: /root/.cache/bazel/_bazel_root/e57db0c8e3cb6ec0b4f0428fb2bd1f06/external/protobuf~/src/google/protobuf/io/BUILD.bazel:11:11: Compiling src/google/protobuf/io/coded_stream.cc failed: (Exit 1): gcc failed: error executing CppCompile command (from target @@protobuf~//src/google/protobuf/io:io) /usr/bin/gcc -U_FORTIFY_SOURCE -fstack-protector -Wall -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer '-std=c++14' -MD -MF ... (remaining 34 arguments skipped)
39.55
39.55 Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
39.55 In file included from external/protobuf~/src/google/protobuf/io/coded_stream.cc:33:
39.55 bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_includes/arena/google/protobuf/arena.h: In static member function 'static T* google::protobuf::Arena::CreateMessage(google::protobuf::Arena*, Args&& ...)':
39.55 bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_includes/arena/google/protobuf/arena.h:183:32: error: 'static T* google::protobuf::Arena::CreateMessage(google::protobuf::Arena*, Args&& ...)' is deprecated: Use Create [-Werror=deprecated-declarations]
39.55   183 |         is_arena_constructable<Type>::value,
39.55       |                                ^~~~
39.55 bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_includes/arena/google/protobuf/arena.h:180:13: note: declared here
39.55   180 |   static T* CreateMessage(Arena* arena, Args&&... args) {
39.55       |             ^~~~~~~~~~~~~
39.55 bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_includes/arena/google/protobuf/arena.h:185:19: error: 'static T* google::protobuf::Arena::CreateMessage(google::protobuf::Arena*, Args&& ...)' is deprecated: Use Create [-Werror=deprecated-declarations]
39.55   185 |     return Create<Type>(arena, std::forward<Args>(args)...);
39.55       |                   ^~~~
39.55 bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_includes/arena/google/protobuf/arena.h:180:13: note: declared here
39.55   180 |   static T* CreateMessage(Arena* arena, Args&&... args) {
39.55       |             ^~~~~~~~~~~~~
39.55 cc1plus: all warnings being treated as errors
39.68 [801 / 1,195] Compiling absl/synchronization/internal/futex_waiter.cc [for tool]; 0s processwrapper-sandbox
40.07 Target //generator:protoc-gen-js failed to build
40.07 Use --verbose_failures to see the command lines of failed build steps.
40.13 INFO: Elapsed time: 39.610s, Critical Path: 3.00s
40.13 INFO: 802 processes: 518 internal, 284 processwrapper-sandbox.
40.13 ERROR: Build did NOT complete successfully

I can also verify this is still happening on the latest main branch (you can run docker build --build-arg COMMIT_SHA="main" --no-cache - < Dockerfile to verify).

I took a look at the commit and nothing stood out to me for reasons why it would break -- @dibenede @anna-lawson any thoughts/advice/ideas would be greatly appreciated!

dibenede commented 4 weeks ago

I think it's just all warnings, but you have cc1plus: all warnings being treated as errors that is maybe causing the actual failure.

The int in for loop warning has been around for awhile, so my best guess it might be the deprecation of CreateMessage in favor of Create.

dibenede commented 4 weeks ago

Did some poking around. So the deprecations around CreateMessage and ultimate failure to build come from protobuf 27.1 itself:

(I don't use docker much, so please bear with me. Tweaking the Dockerfile a bit)

FROM node:20-bullseye

ARG BAZEL_VERSION="7.2.0"
ARG PROTO_VERSION="v27.1"

# Install necessary Debian packages
RUN apt-get update
RUN apt-get install curl git -y

# Download Bazel
RUN ARCHITECTURE=$(arch | sed s/aarch64/arm64/) && \
    curl --location https://github.com/bazelbuild/bazel/releases/download/$BAZEL_VERSION/bazel-$BAZEL_VERSION-linux-$ARCHITECTURE --output /usr/local/bin/bazel
RUN chmod +x /usr/local/bin/bazel

WORKDIR /tmp/protobuf
RUN git clone https://github.com/protocolbuffers/protobuf.git .

# Build protobuf
RUN git submodule update --init --recursive && git checkout $PROTO_VERSION
RUN bazel build :protobuf

Naturally, you get the error against the commit you referenced because we're updating to a newer version. I think -Werror is also enabled in protobuf itself.

I'm not sure why the container leads to failure though. We're otherwise able to build 27.1 reliably.

easternmotors commented 2 weeks ago

@dibenede thanks so much for looking into this and getting back to me so quickly, sorry for my delayed response. I will be sure to post in here if I make any progress on the issue. I just tried testing against the newly release Protocol Buffers v27.2 and I'm still seeing the same issue