Skip to content

[C++ Client] Warn, don't fail, on unknown compiler options#14752

Closed
michaeljmarshall wants to merge 1 commit intoapache:masterfrom
michaeljmarshall:only-warn-for-unknown-options
Closed

[C++ Client] Warn, don't fail, on unknown compiler options#14752
michaeljmarshall wants to merge 1 commit intoapache:masterfrom
michaeljmarshall:only-warn-for-unknown-options

Conversation

@michaeljmarshall
Copy link
Member

@michaeljmarshall michaeljmarshall commented Mar 18, 2022

Fix: #13429

Motivation

As discussed in #13429, it's helpful to make warnings errors. As such, I (naively) propose that instead of turning off this option, we should just turn it off for unknown warning options.

An alternative option is to remove the -Wno-error=cpp option. However, based on the comment in the code just above it, it looks like that class of warnings is generally okay to ignore. Although, this change might be problematic in the event that certain warnings will break the build for one compiler and not for another, which will only become evident at release time.

Modifications

  • add -Wno-error=unknown-warning-option to c++ client options

Verifying this change

I will rely on those more familiar with C++ to verify this change. I am proposing it simply to try to fix the current issues.

Note that after this change, building the Python Client has these warning logs (that used to be errors):

    default: [  5%] Building CXX object lib/CMakeFiles/PULSAR_OBJECT_LIB.dir/Authentication.cc.o
    default: warning: unknown warning option '-Werror=cpp' [-Wunknown-warning-option]
    default: warning: unknown warning option '-Werror=cpp' [-Wunknown-warning-option]
    default: warning: unknown warning option '-Werror=cpp' [-Wunknown-warning-option]

Does this pull request potentially affect one of the following parts:

It just affects the build.

@github-actions
Copy link

@michaeljmarshall:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

1 similar comment
@github-actions
Copy link

@michaeljmarshall:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@michaeljmarshall michaeljmarshall added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Mar 18, 2022
@michaeljmarshall
Copy link
Member Author

@oversearch @merlimat @BewareMyPower - let me know if this option makes sense to add, or if we should actually just remove the -Wno-error=cpp option.

@michaeljmarshall
Copy link
Member Author

michaeljmarshall commented Mar 18, 2022

Ironically, this option isn't known to the compiler for our CI builds.

An additional data point: the build is not working for me on 2.10 because of the same error on 2.8.3.

mmarshall-rmbp16:osx michaelmarshall$ ./generate-all-wheel.sh v2.10.0-candidate-1
GIT_TAG: 'v2.10.0-candidate-1'
GIT_REPO: 'https://github.com/apache/pulsar.git'

------------- BUILDING PYTHON WHEELS FOR osx-10.12 ---------------------
~/dev/apache/pulsar/pulsar-client-cpp/python/pkg/osx/osx-10.12 ~/dev/apache/pulsar/pulsar-client-cpp/python/pkg/osx
Bringing machine 'default' up with 'virtualbox' provider...
==> default: Checking if box 'apachepulsar/osx-10.12-python-dev' version '0.1' is up to date...
==> default: Running provisioner: shell...
    default: Running: /var/folders/yl/07dpwzpn5wldvrvpf_rk1pzm0000gn/T/vagrant-shell20220318-37624-1ks9wth.sh
    default: + rm -rf pulsar
    default: + git clone -q --depth 1 --branch v2.10.0-candidate-1 https://github.com/apache/pulsar.git
    default: Note: checking out 'c58e6e8b33a487b6d9dd10410b7620d33ecb994f'.
    default:
    default: You are in 'detached HEAD' state. You can look around, make experimental
    default: changes and commit them, and you can discard any commits you make in this
    default: state without impacting any branches by performing another checkout.
    default:
    default: If you want to create a new branch to retain commits you create, you may
    default: do so (now or later) by using -b with the checkout command again. Example:
    default:
    default:   git checkout -b <new-branch-name>
    default:
    default: + cd pulsar/pulsar-client-cpp
    default: + brew link --force boost
    default: Warning: Already linked: /usr/local/Cellar/boost/1.68.0_1
    default: To relink: brew unlink boost && brew link boost
    default: + brew link --force protobuf260
    default: Warning: Already linked: /usr/local/Cellar/protobuf@2.6/2.6.1
    default: To relink: brew unlink protobuf@2.6 && brew link --force protobuf@2.6
    default: + brew unlink python
    default: Unlinking /usr/local/Cellar/python/3.7.2_1... 0 symlinks removed
    default: + brew unlink boost-python3
    default: Unlinking /usr/local/Cellar/boost-python3/1.68.0... 0 symlinks removed
    default: + brew link --force python@2
    default: Warning: Already linked: /usr/local/Cellar/python@2/2.7.15_2
    default: To relink: brew unlink python@2 && brew link python@2
    default: + brew link --force boost-python
    default: Warning: Already linked: /usr/local/Cellar/boost-python/1.68.0
    default: To relink: brew unlink boost-python && brew link boost-python
    default: + cmake . -DBUILD_TESTS=OFF -DLINK_STATIC=ON -DPYTHON_LIBRARY=/usr/local/Frameworks/Python.framework/Versions/2.7/lib/libpython2.7.dylib
    default: -- The C compiler identification is AppleClang 9.0.0.9000039
    default: -- The CXX compiler identification is AppleClang 9.0.0.9000039
    default: -- Check for working C compiler: /Library/Developer/CommandLineTools/usr/bin/cc
    default: -- Check for working C compiler: /Library/Developer/CommandLineTools/usr/bin/cc -- works
    default: -- Detecting C compiler ABI info
    default: -- Detecting C compiler ABI info - done
    default: -- Detecting C compile features
    default: -- Detecting C compile features - done
    default: -- Check for working CXX compiler: /Library/Developer/CommandLineTools/usr/bin/c++
    default: -- Check for working CXX compiler: /Library/Developer/CommandLineTools/usr/bin/c++ -- works
    default: -- Detecting CXX compiler ABI info
    default: -- Detecting CXX compiler ABI info - done
    default: -- Detecting CXX compile features
    default: -- Detecting CXX compile features - done
    default: -- ARCHITECTURE: x86_64
    default: -- BUILD_DYNAMIC_LIB:  ON
    default: -- BUILD_STATIC_LIB:  ON
    default: -- BUILD_TESTS:  OFF
    default: -- BUILD_PYTHON_WRAPPER:  ON
    default: -- BUILD_WIRESHARK:  OFF
    default: -- BUILD_PERF_TOOLS:  OFF
    default: -- LINK_STATIC:  ON
    default: -- USE_LOG4CXX:  OFF
    default: -- CMAKE_BUILD_TYPE:  RelWithDebInfo
    default: -- Looking for pthread.h
    default: -- Looking for pthread.h - found
    default: -- Looking for pthread_create
    default: -- Looking for pthread_create - found
    default: -- Found Threads: TRUE
    default: -- Threads library:
    default: -- Found OpenSSL: /usr/local/opt/openssl/lib/libcrypto.dylib
    default: -- Protobuf_LIBRARIES: /usr/local/lib/libprotobuf.a
    default: -- Boost version: 1.68.0
    default: -- Linking with Boost:System
    default: -- Using std::regex
    default: -- Boost version: 1.68.0
    default: -- Found the following Boost libraries:
    default: --   system
    default: -- Found PythonLibs: /usr/local/Frameworks/Python.framework/Versions/2.7/lib/libpython2.7.dylib (found version "2.7.10")
    default: -- PYTHON: 2.7.10
    default: CMake Warning at /usr/local/Cellar/cmake/3.13.3/share/cmake/Modules/FindBoost.cmake:1775 (message):
    default:   No header defined for python-mt; skipping header check
    default: Call Stack (most recent call first):
    default:   CMakeLists.txt:281 (find_package)
    default:
    default:
    default: CMake Warning at /usr/local/Cellar/cmake/3.13.3/share/cmake/Modules/FindBoost.cmake:1775 (message):
    default:   No header defined for python-py27; skipping header check
    default: Call Stack (most recent call first):
    default:   CMakeLists.txt:281 (find_package)
    default:
    default:
    default: CMake Warning at /usr/local/Cellar/cmake/3.13.3/share/cmake/Modules/FindBoost.cmake:1775 (message):
    default:   No header defined for python27-mt; skipping header check
    default: Call Stack (most recent call first):
    default:   CMakeLists.txt:281 (find_package)
    default:
    default:
    default: CMake Warning at /usr/local/Cellar/cmake/3.13.3/share/cmake/Modules/FindBoost.cmake:1775 (message):
    default:   No header defined for python27-mt; skipping header check
    default: Call Stack (most recent call first):
    default:   CMakeLists.txt:292 (find_package)
    default:
    default:
    default: -- Boost version: 1.68.0
    default: -- Found the following Boost libraries:
    default: --   python27-mt
    default: -- Found OpenSSL: /usr/local/opt/openssl/lib/libcrypto.a
    default: -- HAS_ZSTD: 1
    default: -- HAS_SNAPPY: 0
    default: -- Looking for getauxval
    default: -- Looking for getauxval - not found
    default: -- Using Boost Python libs: /usr/local/lib/libboost_python27-mt.a
    default: clang-tidy not found
    default: clang-format not found
    default: -- Configuring done
    default: -- Generating done
    default: -- Build files have been written to: /Users/vagrant/pulsar/pulsar-client-cpp
    default: + make _pulsar -j8
    default: [  0%] Generating ../generated/lib/PulsarApi.pb.cc, ../generated/lib/PulsarApi.pb.h
    default: Scanning dependencies of target PULSAR_OBJECT_LIB
    default: [  1%] Building CXX object lib/CMakeFiles/PULSAR_OBJECT_LIB.dir/AckGroupingTracker.cc.o
    default: [  2%] Building CXX object lib/CMakeFiles/PULSAR_OBJECT_LIB.dir/AckGroupingTrackerDisabled.cc.o
    default: error: unknown warning option '-Werror=cpp' [-Werror,-Wunknown-warning-option]
    default: [  2%] Building CXX object lib/CMakeFiles/PULSAR_OBJECT_LIB.dir/AckGroupingTrackerEnabled.cc.o
    default: error: unknown warning option '-Werror=cpp' [-Werror,-Wunknown-warning-option]
    default: make[3]: *** [lib/CMakeFiles/PULSAR_OBJECT_LIB.dir/AckGroupingTracker.cc.o] Error 1
    default: make[3]: *** Waiting for unfinished jobs....
    default: make[3]: *** [lib/CMakeFiles/PULSAR_OBJECT_LIB.dir/AckGroupingTrackerDisabled.cc.o] Error 1
    default: [  3%] Building CXX object lib/CMakeFiles/PULSAR_OBJECT_LIB.dir/Authentication.cc.o
    default: error: unknown warning option '-Werror=cpp' [-Werror,-Wunknown-warning-option]
    default: make[3]: *** [lib/CMakeFiles/PULSAR_OBJECT_LIB.dir/AckGroupingTrackerEnabled.cc.o] Error 1
    default: error: unknown warning option '-Werror=cpp' [-Werror,-Wunknown-warning-option]
    default: make[3]: *** [lib/CMakeFiles/PULSAR_OBJECT_LIB.dir/Authentication.cc.o] Error 1
    default: make[2]: *** [lib/CMakeFiles/PULSAR_OBJECT_LIB.dir/all] Error 2
    default: make[1]: *** [python/CMakeFiles/_pulsar.dir/rule] Error 2
    default: make: *** [_pulsar] Error 2
The SSH command responded with a non-zero exit status. Vagrant
assumes that this means the command failed. The output for this command
should be in the log above. Please read the output to determine what
went wrong.

@BewareMyPower
Copy link
Contributor

BewareMyPower commented Mar 19, 2022

I think you can use https://godbolt.org/ to test if a compiler option works for a specific compiler.

@michaeljmarshall michaeljmarshall marked this pull request as draft March 24, 2022 04:48
@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@michaeljmarshall
Copy link
Member Author

This work is no longer necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs lifecycle/stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Release procedure: Building OSX packages is broken, at least in 2.9.1

2 participants