Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,7 @@ ExternalProject_Add(grpc-repo
TEST_COMMAND ""
CMAKE_CACHE_ARGS
-DCMAKE_CXX_STANDARD:STRING=${TRITON_MIN_CXX_STANDARD}
# TODO(nnoble): remove this patch when the fix for https://github.com/abseil/abseil-cpp/issues/1769 is integrated within our dependencies
PATCH_COMMAND python3 ${CMAKE_CURRENT_SOURCE_DIR}/tools/patch.py apply -i -d ${CMAKE_CURRENT_BINARY_DIR}/grpc-repo/src/grpc/third_party/abseil-cpp ${CMAKE_CURRENT_SOURCE_DIR}/tools/abseil_no_rebuild.patch
COMMAND python3 ${CMAKE_CURRENT_SOURCE_DIR}/tools/install_src.py --src <SOURCE_DIR> ${INSTALL_SRC_DEST_ARG} --dest-basename=grpc_1.54.3
PATCH_COMMAND python3 ${CMAKE_CURRENT_SOURCE_DIR}/tools/install_src.py --src <SOURCE_DIR> ${INSTALL_SRC_DEST_ARG} --dest-basename=grpc_1.81.1
)
#
# Build nlohmann/json
Expand Down Expand Up @@ -171,7 +169,7 @@ ExternalProject_Add(absl
#
ExternalProject_Add(protobuf
PREFIX protobuf
SOURCE_DIR "${CMAKE_CURRENT_BINARY_DIR}/grpc-repo/src/grpc/third_party/protobuf/cmake"
SOURCE_DIR "${CMAKE_CURRENT_BINARY_DIR}/grpc-repo/src/grpc/third_party/protobuf"
EXCLUDE_FROM_ALL ON
DOWNLOAD_COMMAND ""
CMAKE_CACHE_ARGS
Expand Down Expand Up @@ -230,8 +228,10 @@ ExternalProject_Add(googletest
# platform
if (WIN32)
set(_FINDPACKAGE_PROTOBUF_CONFIG_DIR "${TRITON_THIRD_PARTY_INSTALL_PREFIX}/protobuf/cmake")
set(_FINDPACKAGE_UTF8_RANGE_CONFIG_DIR "${TRITON_THIRD_PARTY_INSTALL_PREFIX}/protobuf/cmake/utf8_range")
else()
set(_FINDPACKAGE_PROTOBUF_CONFIG_DIR "${TRITON_THIRD_PARTY_INSTALL_PREFIX}/protobuf/${LIB_DIR}/cmake/protobuf")
set(_FINDPACKAGE_UTF8_RANGE_CONFIG_DIR "${TRITON_THIRD_PARTY_INSTALL_PREFIX}/protobuf/${LIB_DIR}/cmake/utf8_range")
endif()
#
# Build c-ares project from grpc-repo
Expand Down Expand Up @@ -282,6 +282,7 @@ ExternalProject_Add(grpc
-Dc-ares_DIR:PATH=${TRITON_THIRD_PARTY_INSTALL_PREFIX}/c-ares/${LIB_DIR}/cmake/c-ares
-Dre2_DIR:PATH=${TRITON_THIRD_PARTY_INSTALL_PREFIX}/re2/${LIB_DIR}/cmake/re2
-DProtobuf_DIR:PATH=${_FINDPACKAGE_PROTOBUF_CONFIG_DIR}
-Dutf8_range_DIR:PATH=${_FINDPACKAGE_UTF8_RANGE_CONFIG_DIR}
${_CMAKE_ARGS_OPENSSL_ROOT_DIR}
${_CMAKE_ARGS_CMAKE_TOOLCHAIN_FILE}
${_CMAKE_ARGS_VCPKG_TARGET_TRIPLET}
Expand Down Expand Up @@ -416,7 +417,8 @@ set(GCS_CMAKE_PREFIX_PATH ${CMAKE_PREFIX_PATH}
${TRITON_THIRD_PARTY_INSTALL_PREFIX}/crc32c/${LIB_DIR}/cmake/Crc32c
${TRITON_THIRD_PARTY_INSTALL_PREFIX}/grpc/lib/cmake/grpc
${TRITON_THIRD_PARTY_INSTALL_PREFIX}/nlohmann_json/share/cmake/nlohmann_json
${_FINDPACKAGE_PROTOBUF_CONFIG_DIR})
${_FINDPACKAGE_PROTOBUF_CONFIG_DIR}
${_FINDPACKAGE_UTF8_RANGE_CONFIG_DIR})
#
# Build google-cloud-cpp
#
Expand All @@ -431,13 +433,15 @@ ExternalProject_Add(google-cloud-cpp
${_CMAKE_ARGS_VCPKG_TARGET_TRIPLET}
-DCMAKE_CXX_STANDARD:STRING=${TRITON_MIN_CXX_STANDARD}
-DGOOGLE_CLOUD_CPP_DEPENDENCY_PROVIDER:STRING=package
-DGOOGLE_CLOUD_CPP_ENABLE:STRING=storage

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 GOOGLE_CLOUD_CPP_ENABLE=storage narrows previously built features

Previously this target built all default google-cloud-cpp components; restricting to storage means any Triton backend or downstream that links against google-cloud-cpp for BigTable, Pub/Sub, Spanner, or IAM will silently get a missing-target error at their own build time. Could you confirm that no Triton backends consume google-cloud-cpp features beyond storage?

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Are there any Triton backends or downstream consumers that use google-cloud-cpp features other than storage (e.g. bigtable, pubsub, spanner)?

-DBUILD_TESTING:BOOL=OFF
-DGOOGLE_CLOUD_CPP_WITH_MOCKS:BOOL=OFF
-DCMAKE_PREFIX_PATH:PATH=${GCS_CMAKE_PREFIX_PATH}
-DCMAKE_POSITION_INDEPENDENT_CODE:BOOL=ON
-DCMAKE_INSTALL_PREFIX:PATH=${TRITON_THIRD_PARTY_INSTALL_PREFIX}/google-cloud-cpp
-Dnlohmann_json_DIR:PATH=${TRITON_THIRD_PARTY_INSTALL_PREFIX}/nlohmann_json/share/cmake/nlohmann_json
-DProtobuf_DIR:PATH=${_FINDPACKAGE_PROTOBUF_CONFIG_DIR}
-Dutf8_range_DIR:PATH=${_FINDPACKAGE_UTF8_RANGE_CONFIG_DIR}
-DCrc32c_DIR:PATH=${TRITON_THIRD_PARTY_INSTALL_PREFIX}/crc32c/${LIB_DIR}/cmake/Crc32c
PATCH_COMMAND python3 ${CMAKE_CURRENT_SOURCE_DIR}/tools/install_src.py --src <SOURCE_DIR> ${INSTALL_SRC_DEST_ARG}
DEPENDS grpc c-ares protobuf crc32c absl nlohmann-json
Expand Down Expand Up @@ -556,9 +560,11 @@ if (NOT WIN32)
${_CMAKE_ARGS_VCPKG_TARGET_TRIPLET}
-DBUILD_SHARED_LIBS:STRING=OFF
-DBUILD_TESTING:BOOL=OFF
-DWITH_FUNC_TESTS:BOOL=OFF

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why change these flags? in the PR description i see:

opentelemetry-cpp: switch WITH_ABSEIL to ON against our absl install (adds absl_DIR and the absl dependency), and disable functional tests (WITH_FUNC_TESTS=OFF).

but why do we actually need to do this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

-DWITH_EXAMPLES:BOOL=OFF
-DWITH_BENCHMARK:BOOL=OFF
-DWITH_ABSEIL:BOOL=OFF
-DWITH_ABSEIL:BOOL=ON
-Dabsl_DIR:PATH=${TRITON_THIRD_PARTY_INSTALL_PREFIX}/absl/${LIB_DIR}/cmake/absl
Comment on lines +563 to +567

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 opentelemetry-cpp missing utf8_range_DIR

With WITH_ABSEIL=ON, opentelemetry-cpp v1.13.0's CMake will call find_package(absl CONFIG) using the newly provided absl_DIR. If any of its internal cmake modules (e.g. for WITH_OTLP_HTTP) also invoke find_package(Protobuf CONFIG) — rather than relying solely on the raw Protobuf_LIBRARIES / Protobuf_INCLUDE_DIR variables — protobuf v33's package config will fail to locate utf8_range targets without a corresponding -Dutf8_range_DIR. The grpc and google-cloud-cpp targets both received this argument; opentelemetry-cpp did not. If CI passes in its current form this is benign, but adding -Dutf8_range_DIR:PATH=${_FINDPACKAGE_UTF8_RANGE_CONFIG_DIR} here would make the three consumers consistent.

-DWITH_OTLP_GRPC:BOOL=OFF
-DWITH_OTLP_HTTP:BOOL=ON
-DOPENTELEMETRY_INSTALL:BOOL=ON
Expand All @@ -568,6 +574,6 @@ if (NOT WIN32)
-DCMAKE_BUILD_TYPE:STRING=${CMAKE_BUILD_TYPE}
-DCMAKE_INSTALL_PREFIX:PATH=${TRITON_THIRD_PARTY_INSTALL_PREFIX}/opentelemetry-cpp
PATCH_COMMAND python3 ${CMAKE_CURRENT_SOURCE_DIR}/tools/install_src.py --src <SOURCE_DIR> ${INSTALL_SRC_DEST_ARG}
DEPENDS grpc nlohmann-json curl protobuf
DEPENDS grpc nlohmann-json curl protobuf absl
)
endif()
29 changes: 0 additions & 29 deletions tools/abseil_no_rebuild.patch

This file was deleted.

Loading