ARROW-16734: [C++] Bump vendored version of protobuf #13581

raulcd · 2022-07-12T12:46:15Z

No description provided.

github-actions · 2022-07-12T12:46:39Z

https://issues.apache.org/jira/browse/ARROW-16733

raulcd · 2022-07-12T12:46:45Z

@github-actions crossbow submit -g cpp

github-actions · 2022-07-12T12:47:55Z

Revision: 3a7527b

Submitted crossbow builds: ursacomputing/crossbow @ actions-132e28e192

Task	Status
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-debian-10-cpp-amd64
test-debian-10-cpp-i386
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-35-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp

github-actions · 2022-07-12T16:01:28Z

https://issues.apache.org/jira/browse/ARROW-16734

…change

raulcd · 2022-07-13T10:50:14Z

@github-actions crossbow submit -g cpp

raulcd · 2022-07-13T10:56:03Z

@pitrou @lidavidm The latest version of protocol buffers is the following (there was a change of schema versioning between v3.20 and v21):

ARROW_PROTOBUF_BUILD_VERSION=v21.2
ARROW_PROTOBUF_BUILD_SHA256_CHECKSUM=9ae699200f3a80c735f9dc3b20e46d447584266f4601403e8fe5b97005f204dd

If I try to set the latest version all code and test works but there is a failure on the thread-sanitizer job building substrait.
See part of the stack trace for this build (https://github.com/ursacomputing/crossbow/runs/7302033023?check_suite_focus=true):

[86/902] Building CXX object CMakeFiles/substrait.dir/substrait_ep-generated/substrait/type.pb.cc.o
FAILED: CMakeFiles/substrait.dir/substrait_ep-generated/substrait/type.pb.cc.o 
/usr/bin/ccache /usr/lib/ccache/clang++-12  -DARROW_HAVE_RUNTIME_AVX2 -DARROW_HAVE_RUNTIME_AVX512 -DARROW_HAVE_RUNTIME_BMI2 -DARROW_HAVE_RUNTIME_SSE4_2 -DARROW_HAVE_SSE4_2 -DARROW_NO_DEPRECATED_API -DARROW_WITH_RE2 -DARROW_WITH_TIMING_TESTS -DARROW_WITH_UTF8PROC -DDYNAMIC_ANNOTATIONS_ENABLED -DTHREAD_SANITIZER -D_GLIBCXX_EXTERN_TEMPLATE=0 -Isubstrait_ep-generated -Iprotobuf_ep-install/include -Isrc -I/arrow/cpp/src -I/arrow/cpp/src/generated -Qunused-arguments -fcolor-diagnostics -ggdb -O0  -Wall -Wextra -Wdocumentation -Wshorten-64-to-32 -Wno-missing-braces -Wno-unused-parameter -Wno-constant-logical-operand -Wno-return-stack-address -Werror -Wno-unknown-warning-option -Wno-pass-failed -msse4.2  -fsanitize-blacklist=/arrow/cpp/build-support/sanitizer-disallowed-entries.txt -g -fPIC   -fsanitize=thread -std=c++11 -MD -MT CMakeFiles/substrait.dir/substrait_ep-generated/substrait/type.pb.cc.o -MF CMakeFiles/substrait.dir/substrait_ep-generated/substrait/type.pb.cc.o.d -o CMakeFiles/substrait.dir/substrait_ep-generated/substrait/type.pb.cc.o -c substrait_ep-generated/substrait/type.pb.cc
In file included from substrait_ep-generated/substrait/type.pb.cc:4:
In file included from substrait_ep-generated/substrait/type.pb.h:28:
In file included from protobuf_ep-install/include/google/protobuf/generated_message_reflection.h:47:
In file included from protobuf_ep-install/include/google/protobuf/unknown_field_set.h:53:
protobuf_ep-install/include/google/protobuf/parse_context.h:328:47: error: implicit conversion loses integer precision: 'long' to 'int' [-Werror,-Wshorten-64-to-32]
    int chunk_size = buffer_end_ + kSlopBytes - ptr;
        ~~~~~~~~~~   ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
protobuf_ep-install/include/google/protobuf/parse_context.h:342:45: error: implicit conversion loses integer precision: 'long' to 'int' [-Werror,-Wshorten-64-to-32]
      chunk_size = buffer_end_ + kSlopBytes - ptr;
                 ~ ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
protobuf_ep-install/include/google/protobuf/parse_context.h:786:41: error: implicit conversion loses integer precision: 'long' to 'int' [-Werror,-Wshorten-64-to-32]
  int nbytes = buffer_end_ + kSlopBytes - ptr;
      ~~~~~~   ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
protobuf_ep-install/include/google/protobuf/parse_context.h:804:39: error: implicit conversion loses integer precision: 'long' to 'int' [-Werror,-Wshorten-64-to-32]
    nbytes = buffer_end_ + kSlopBytes - ptr;
           ~ ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
protobuf_ep-install/include/google/protobuf/parse_context.h:836:32: error: implicit conversion loses integer precision: 'long' to 'int' [-Werror,-Wshorten-64-to-32]
  int chunk_size = buffer_end_ - ptr;
      ~~~~~~~~~~   ~~~~~~~~~~~~^~~~~
protobuf_ep-install/include/google/protobuf/parse_context.h:840:23: error: implicit conversion loses integer precision: 'long' to 'int' [-Werror,-Wshorten-64-to-32]
    int overrun = ptr - buffer_end_;
        ~~~~~~~   ~~~~^~~~~~~~~~~~~
protobuf_ep-install/include/google/protobuf/parse_context.h:861:30: error: implicit conversion loses integer precision: 'long' to 'int' [-Werror,-Wshorten-64-to-32]
    chunk_size = buffer_end_ - ptr;
               ~ ~~~~~~~~~~~~^~~~~
protobuf_ep-install/include/google/protobuf/parse_context.h:884:25: error: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
  uint32_t number = tag >> 3;
           ~~~~~~   ~~~~^~~~
8 errors generated.

This seems to be a substrait issue more than ARROW but I wanted to get your thoughts on this.
The current updated version on this PR is the latest before the change of schema versioning which works for the sanitizer job (I reproduced the issue locally).

github-actions · 2022-07-13T11:04:29Z

Revision: 9253949

Submitted crossbow builds: ursacomputing/crossbow @ actions-529e93f4b6

Task	Status
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-debian-10-cpp-amd64
test-debian-10-cpp-i386
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-35-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp

lidavidm · 2022-07-13T11:56:04Z

this is really a protobuf issue and we already suppress such warnings for MSVC which is stricter about these things

arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake

Lines 1733 to 1742 in 8608e19

    
           set(SUBSTRAIT_SUPPRESSED_WARNINGS) 
        
           if(MSVC) 
        
             # Protobuf generated files trigger some spurious warnings on MSVC. 
        
             # Implicit conversion from uint64_t to uint32_t: 
        
             list(APPEND SUBSTRAIT_SUPPRESSED_WARNINGS "/wd4244") 
        
             # Missing dll-interface: 
        
             list(APPEND SUBSTRAIT_SUPPRESSED_WARNINGS "/wd4251") 
        
           endif()

looks like we need to suppress them for all compilers now

…n generating substrait protobuf

raulcd · 2022-07-13T13:23:47Z

@github-actions crossbow submit test-ubuntu-20.04-cpp-thread-sanitizer

github-actions · 2022-07-13T13:43:06Z

Revision: e80016f

Submitted crossbow builds: ursacomputing/crossbow @ actions-33d6602cc3

Task	Status
test-ubuntu-20.04-cpp-thread-sanitizer

raulcd · 2022-07-13T14:26:53Z

It seems it is only on clang because we specifically build with: -Wshorten-64-to-32: https://github.com/apache/arrow/blob/master/cpp/cmake_modules/SetupCxxFlags.cmake#L285
Supressing the errors fixes the build.

raulcd · 2022-07-13T14:27:12Z

@github-actions crossbow submit -g cpp

github-actions · 2022-07-13T14:28:25Z

Revision: e80016f

Submitted crossbow builds: ursacomputing/crossbow @ actions-6d7f56138a

Task	Status
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-debian-10-cpp-amd64
test-debian-10-cpp-i386
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-35-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp

ursabot · 2022-07-15T00:32:07Z

Benchmark runs are scheduled for baseline = e766828 and contender = 5d86e9f. 5d86e9f is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.63% ⬆️0.59%] test-mac-arm
[Failed ⬇️0.63% ⬆️0.32%] ursa-i9-9960x
[Finished ⬇️0.18% ⬆️0.07%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 5d86e9fc ec2-t3-xlarge-us-east-2
[Failed] 5d86e9fc test-mac-arm
[Failed] 5d86e9fc ursa-i9-9960x
[Finished] 5d86e9fc ursa-thinkcentre-m75q
[Finished] e766828c ec2-t3-xlarge-us-east-2
[Failed] e766828c test-mac-arm
[Failed] e766828c ursa-i9-9960x
[Finished] e766828c ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added the Component: C++ label Jul 12, 2022

raulcd changed the title ~~ARROW-16733: [C++] Bump vendored version of protobuf~~ ARROW-16734: [C++] Bump vendored version of protobuf Jul 12, 2022

raulcd added 2 commits July 13, 2022 12:38

ARROW-16733: [C++] Bump vendored version of protobuf

00db9df

ARROW-16733: [C++] Use latest protobuf version before version schema …

9253949

…change

raulcd force-pushed the ARROW-16734 branch from 3a7527b to 9253949 Compare July 13, 2022 10:50

raulcd added 2 commits July 13, 2022 15:19

ARROW-16733: [C++] Use latest protobuf versiona and supress error whe…

1ccc205

…n generating substrait protobuf

Minor lint fix

e80016f

raulcd marked this pull request as ready for review July 13, 2022 14:27

lidavidm approved these changes Jul 13, 2022

View reviewed changes

lidavidm merged commit 5d86e9f into apache:master Jul 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-16734: [C++] Bump vendored version of protobuf #13581

ARROW-16734: [C++] Bump vendored version of protobuf #13581

raulcd commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

raulcd commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

raulcd commented Jul 13, 2022

raulcd commented Jul 13, 2022

github-actions bot commented Jul 13, 2022

lidavidm commented Jul 13, 2022

raulcd commented Jul 13, 2022

github-actions bot commented Jul 13, 2022

raulcd commented Jul 13, 2022

raulcd commented Jul 13, 2022

github-actions bot commented Jul 13, 2022

ursabot commented Jul 15, 2022

ARROW-16734: [C++] Bump vendored version of protobuf #13581

ARROW-16734: [C++] Bump vendored version of protobuf #13581

Conversation

raulcd commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

raulcd commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

raulcd commented Jul 13, 2022

raulcd commented Jul 13, 2022

github-actions bot commented Jul 13, 2022

lidavidm commented Jul 13, 2022

raulcd commented Jul 13, 2022

github-actions bot commented Jul 13, 2022

raulcd commented Jul 13, 2022

raulcd commented Jul 13, 2022

github-actions bot commented Jul 13, 2022

ursabot commented Jul 15, 2022