Skip to content

Commit

Permalink
build: mark all protobuf headers as system headers (#132)
Browse files Browse the repository at this point in the history
This PR makes sure that all protobuf headers, either from the library or
the generated ones, are marked as `SYSTEM` headers. This causes most
compilers to include them via `-isystem` instead of `-I`, which, in
turn, suppresses warnings from those headers and, thus, enables
compiling the main project with `-Werror`.

This is achieved with the combination of several changes:
* Several unnecessary `include_directories` statements have been
removed. These are unnecessary because we already depend on the
`protobuf::libprotobuf` target, which includes header dependencies.
* We don't unnecessarily set the `target_include_directories` for the
protobuf library headers for the same reason.
* We mark the Protobuf library downloaded by `FetchContent` as `SYSTEM`,
which marks all targets in that library as `SYSTEM`.
* We manually mark our `substrait::proto` target as `SYSTEM`.

I am not 100% sure if the first point doesn't break some legacy versions
of Protobuf. My best guess is that those would be so old that they do
not work anymore anyways. I am leaving this message in case somebody
runs into problems, though. In that case, the `include_directories`
commands should only be set for the legacy case.

Signed-off-by: Ingo Müller <[email protected]>
Co-authored-by: David Sisson <[email protected]>
  • Loading branch information
ingomueller-net and EpsilonPrime authored Feb 27, 2025
1 parent 2f5524d commit 5d05a2b
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 5 deletions.
6 changes: 1 addition & 5 deletions src/substrait/proto/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
# SPDX-License-Identifier: Apache-2.0
find_package(Protobuf REQUIRED)
include_directories(${Protobuf_INCLUDE_DIRS})
include_directories(${CMAKE_CURRENT_BINARY_DIR})

find_package(Perl REQUIRED)
set(UPDATE_PROTO_PACKAGE_TOOL
Expand Down Expand Up @@ -94,10 +92,8 @@ target_link_libraries(substrait_proto protobuf::libprotobuf)

# Make sure we can see our own generated include files.
target_include_directories(
substrait_proto
substrait_proto SYSTEM
PUBLIC $<BUILD_INTERFACE:${PROTO_OUTPUT_TOPLEVEL_DIR}/src>
$<INSTALL_INTERFACE:include>
PUBLIC $<BUILD_INTERFACE:${protobuf_SOURCE_DIR}/src>
$<INSTALL_INTERFACE:include>)

install(
Expand Down
1 change: 1 addition & 0 deletions third_party/protobuf.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ FetchContent_Declare(GTest
FetchContent_Declare(Protobuf
GIT_REPOSITORY https://github.com/protocolbuffers/protobuf.git
GIT_TAG v28.2
SYSTEM
OVERRIDE_FIND_PACKAGE
)

Expand Down

0 comments on commit 5d05a2b

Please sign in to comment.