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 'syntax' and synthetic oneof to support protobuf 27.1 #208

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

dibenede
Copy link
Contributor

@dibenede dibenede commented Jun 13, 2024

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

Copy link

@anna-lawson anna-lawson left a comment

Choose a reason for hiding this comment

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

Looks great, thanks so much!

@dibenede dibenede merged commit 893b51d into protocolbuffers:main Jun 14, 2024
4 checks passed
@easternmotors
Copy link

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

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 dibenede deleted the update-protobuf branch June 18, 2024 16:51
@dibenede
Copy link
Contributor Author

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
Copy link

@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

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.

Cannot build js generator with latest protobuf (v26.0+ not supported)
3 participants