Add Google Cloud SDK dependency and build scripts. - #178
Conversation
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
`build_google-cloud-cpp.sh` set BUILD_TESTING=OFF but not the separate GOOGLE_CLOUD_CPP_WITH_MOCKS option, which stays ON by default even then (CMakeLists.txt's cmake_dependent_option: default ON, forced ON when BUILD_TESTING is off). That option gates FindGMockWithTargets at CMakeLists.txt:255 (`if (BUILD_TESTING OR GOOGLE_CLOUD_CPP_WITH_MOCKS)`), which is the only thing in this build that needs GTest - google-cloud-cpp's own CMakeLists never references `benchmark` at all, and grpc/opentelemetry-cpp already skip it via gRPC_BUILD_TESTS=OFF and WITH_BENCHMARK=OFF, both already set here. Verified with a from-scratch build of this branch's dependency chain (zlib, protobuf, openssl, curl, nlohmann_json, grpc, opentelemetry-cpp, then google-cloud-cpp), with googletest and benchmark deliberately never checked out: - today's flags fail configure: "Could NOT find GTest (missing: GTEST_LIBRARY GTEST_INCLUDE_DIR GTEST_MAIN_LIBRARY)", traced through cmake/FindGMockWithTargets.cmake:107. - adding -DGOOGLE_CLOUD_CPP_WITH_MOCKS=OFF configures, builds and installs cleanly, with no libgtest/libgmock/libbenchmark anywhere in the result. - a program using google::cloud::storage::Client and google::cloud::Status (google-cloud-cpp/google/cloud/storage/client.h, matching how DALI's gcs_filesystem.cc uses this library) compiles, links and runs against that install. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
build_aws-sdk-cpp.sh built its own private copies of OpenSSL and curl from the same third_party/openssl and third_party/curl submodules this branch's build_openssl.sh/build_curl.sh already build into the shared prefix, wasting whatever they cost on every run. For OpenSSL it was worse than wasted time: both scripts run `./Configure` directly in third_party/openssl with no separate build directory, so the second Configure reconfigures the same in-place source tree the first one already built. PACKAGE_LIST now builds openssl/curl before aws-sdk-cpp, and build_aws-sdk-cpp.sh points its cmake invocation at INSTALL_PREFIX - the already-built shared copies - instead of building and installing its own into a private third_party/aws-sdk-cpp/deps prefix. The CMake flags aws-sdk-cpp passes for OpenSSL/curl are otherwise unchanged, just repointed; its own toolchain generation is untouched. Verified with a from-scratch build of the reordered chain (zlib, protobuf, openssl, curl once, then aws-sdk-cpp against that shared build): the shared libssl.a/libcurl.a mtimes are identical before and after building aws-sdk-cpp (no rebuild), no third_party/aws-sdk-cpp/deps prefix is created, libaws-cpp-sdk-core/libaws-cpp-sdk-s3 install into the shared prefix as before, and a program constructing Aws::S3::S3Client against that install still links and runs. Building OpenSSL and curl once instead of twice saves roughly 6 minutes per full dependency build (~5m40s for OpenSSL, ~40s for curl, measured on 12 cores). Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
The comment documented this as the way to build offline, but nothing actually passed it to cmake - google-cloud-cpp is the only dependency here that reaches the network at build time (everything else is a pinned submodule), and the escape hatch for that didn't work. Also forward GOOGLE_CLOUD_CPP_OVERRIDE_GOOGLEAPIS_URL_HASH, google-cloud-cpp's matching override for the expected SHA256, for the same reason. Verified with a from-scratch configure+download against the pinned tarball (b8486a2f44f15dc578a9dc1e17b144253079d5c1.tar.gz, sha256 12411ca5cb...83766, matching cmake/GoogleapisConfig.cmake) saved to a local file:// URL: passing it through GOOGLE_CLOUD_CPP_OVERRIDE_GOOGLEAPIS_URL configures cleanly and the googleapis_download step verifies the checksum and extracts real proto sources (google/storage/v1/storage.proto etc.) straight from the local file, with no network request made. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
Mirrors what the AWS SDK already does here: aws-sdk-cpp keeps its own
CMake default of BUILD_SHARED_LIBS=ON and links its C runtime libraries
statically, so DALI gets libaws-cpp-sdk-{core,s3}.so over static
libaws-c-*.a.
The reason it matters is the same on both sides. libdali.so owns the
FileStream dispatch for gs:// and libdali_operators.so owns the reader
that lists and opens objects, so both use the client, and
dali::gcs_filesystem takes a gcs::Client& across that boundary. That is
only sound while there is a single copy of the library in the process.
Built statically, each shared object gets its own private copy - with
separate statics and vtables - and a client constructed by one and used
by the other segfaults. Measured: DALI's GCS test suite crashes in
Pipeline.build() when the symbols are hidden, and passes with this
change.
It also fixes the ABI surface. Statically linked, google-cloud-cpp put
4225 symbols into libdali.so's dynamic table and turned qa/TL0_abi red
with 1695 violations; shared, that is 3 - the dali::gcs_filesystem
functions DALI means to export - with no --exclude-libs needed, the same
as S3.
gRPC, protobuf, abseil, opentelemetry, curl and OpenSSL stay static and
are linked into the three shared objects this produces:
libgoogle_cloud_cpp_{storage,rest_internal,common}.so. Verified that this
is the entire runtime closure - the proto, monitoring, trace and
opentelemetry libraries that the forced features build are not in it.
Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
There was a problem hiding this comment.
🟡 Changes recommended
Fix compiler detection in generate_toolchain_file.sh so absolute host compiler paths are not misclassified as cross-compilation.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds Google Cloud Storage C++ dependencies and build orchestration, including shared OpenSSL/cURL reuse with the AWS SDK.
Changes:
- Adds Google Cloud, gRPC, OpenTelemetry, and nlohmann_json dependencies.
- Adds dependency build scripts and shared toolchain generation.
- Refactors AWS builds to reuse shared OpenSSL/cURL installations.
File summaries
| File | Summary |
|---|---|
README.rst |
Documents new dependencies. |
CLAUDE.md |
Documents GCS build requirements. |
build_scripts/generate_toolchain_file.sh |
Centralizes CMake toolchain generation. |
build_scripts/build_opentelemetry-cpp.sh |
Builds OpenTelemetry. |
build_scripts/build_openssl.sh |
Builds shared-prefix OpenSSL. |
build_scripts/build_nlohmann_json.sh |
Installs nlohmann_json. |
build_scripts/build_grpc.sh |
Builds gRPC. |
build_scripts/build_google-cloud-cpp.sh |
Builds the Google Cloud C++ client. |
build_scripts/build_deps.sh |
Updates dependency ordering. |
build_scripts/build_curl.sh |
Builds shared-prefix cURL. |
build_scripts/build_aws-sdk-cpp.sh |
Reuses shared dependencies. |
.gitmodules |
Registers new submodules. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
…e hatch Verified against google-cloud-cpp 3.9.0 (the relevant code is identical at the pinned 3.8.0): storage,-monitoring,-trace,-opentelemetry,-universe_domain configures cleanly but fails to build - internal/opentelemetry_context.cc is an unconditional entry in google_cloud_cpp_common.cmake, but opentelemetry-cpp is only linked in when the opentelemetry feature is requested. Dropping monitoring/trace individually fails at generate time instead (missing target). gRPC is genuinely unavoidable here; the old wording implied a cheaper configuration was one flag away.
…P string
${CC_COMP} != gcc misclassifies a native build as cross-compiling whenever
CC_COMP is an absolute path (e.g. /usr/bin/gcc, as validate_toolchain_env.sh
allows) or a versioned name. Flagged independently by Copilot and Greptile
on generate_toolchain_file.sh; build_grpc.sh and build_google-cloud-cpp.sh
had the identical check deciding whether to do a host-tool pre-build pass.
Fixed by comparing basename() plus, when the compiler can be queried, its
-dumpmachine target architecture against the build host - mirroring the
detection build_protobuf.sh already uses. Note this still does not treat a
versioned name like gcc-12 as native (basename mismatch alone still trips
it) - that's a pre-existing gap in build_protobuf.sh's own pattern too, not
something this fixes; flagging in review in case that's worth a follow-up.
…ndant Confirmed in review: both README.rst's documented checkout and docker/Dockerfile.deps (NVIDIA/DALI) already do a full recursive submodule update, so this loop is a no-op on either documented path. Keeping it as cheap defensive insurance against a plain, non-recursive checkout rather than removing it.
Confirmed by pulling the built deps image and inspecting it directly:
google-cloud-cpp installs libgoogle_cloud_cpp_{storage,rest_internal,
common}.so.3 to /usr/local/lib64 on this manylinux (RHEL-derived)
image, while every other dependency here (aws-sdk-cpp, protobuf,
grpc, curl, ...) lands in /usr/local/lib without any of their build
scripts setting CMAKE_INSTALL_LIBDIR explicitly - google-cloud-cpp's
own CMakeLists.txt just applies GNUInstallDirs' lib64 default more
strictly than the others do.
dali/python/bundle-wheel.sh (DALI) only looks in .../lib, so the
three .so's were silently skipped ("Didn't find ..., skipping...")
and never made it into the wheel - caught by CI on the GCS PR
(NVIDIA/DALI#6467): three build-builder jobs failed wheel validation
with "should be bundled in whl or removed from the dynamic link
dependency" for exactly these three libraries (plus libzstd.so.1/
libz.so.1, downstream noise from the same missing dependency chain).
rostan-t
left a comment
There was a problem hiding this comment.
Many comments have a lot of useful explanations but some are detailed enough that they starts to obscure the actual build scripts. Could we keep the important why close to the corresponding options and shorten the investigation/history details?
Some things like the dependency chain are already included in the PR description and others like the upstream CMake line numbers can be added there if really necessary.
…DE.md GCS section - generate_toolchain_file.sh: drop the "(the original bug)" reference that left a reviewer unable to tell what bug was meant; spell out the misdetection directly instead. - CLAUDE.md: fold "Google Cloud Storage" under a new "Dependency-specific build notes" heading so it doesn't read as arbitrarily singling out one dependency, and note when future entries belong there.
Adds the Google Cloud Storage (GCS) C++ client and its transitive dependencies, mirroring the
existing AWS SDK dependency so DALI can add a GCS reader alongside the S3 one.
What's added
google-cloud-cpp(3.8.0) - only thestoragecomponent is requested(
GOOGLE_CLOUD_CPP_ENABLE=storage), but that transitively pulls inmonitoring,trace,opentelemetryanduniverse_domainregardless (verified: trying to drop them fails to build,not just configure - see
build_google-cloud-cpp.sh). That's wheregRPCandopentelemetry-cppcome from, even though the GCS client itself only talks REST.gRPC(1.83.1) - required transitively as above; reuses this repo's existing protobuf, Abseil,zlib and OpenSSL instead of building its own copies.
opentelemetry-cpp(1.24.0) - API + SDK only, no exporters (OTLP/Zipkin/Prometheus) sincenothing here uses them.
nlohmann_json(3.12.0) - header-only, used by the GCS client to parse JSON API responses.generate_toolchain_file.sh- the CMake toolchain-file generation shared bybuild_grpc.sh/build_google-cloud-cpp.sh/build_opentelemetry-cpp.sh/build_nlohmann_json.sh/build_curl.sh, factored out instead of duplicated per script.build_openssl.sh/build_curl.sh- OpenSSL and libcurl are now built once into the sharedinstall prefix and reused by both
aws-sdk-cppandgoogle-cloud-cpp, instead ofbuild_aws-sdk-cpp.shbuilding its own private static copies of both (which just duplicatedthe work).
Key decisions
google-cloud-cppis built shared, unlike everything else here.libdali.soandlibdali_operators.soboth use the GCS client; agcs::Clientconstructed by one and used bythe other has to be the same object in memory, which only holds if there is a single copy of
the library in the process. Built statically, each
.sowould get its own private copy andpassing a client across that boundary would crash. This mirrors how the AWS SDK is already
handled (
libaws-cpp-sdk-{core,s3}.so, static C runtime libs underneath). gRPC, protobuf,Abseil, opentelemetry-cpp, curl and OpenSSL stay static, linked into the three shared objects
this produces (
libgoogle_cloud_cpp_{storage,rest_internal,common}.so).-DGOOGLE_CLOUD_CPP_WITH_MOCKS=OFFdropsgoogletest/benchmarkfrom this dependency setentirely - they were only pulled in because mocks are on by default whenever
BUILD_TESTING=OFF(which is this build's configuration). Verified with a from-scratch build: default flags fail
configure with "Could NOT find GTest"; this flag configures and builds cleanly without it.
GOOGLE_CLOUD_CPP_OVERRIDE_GOOGLEAPIS_URLis forwarded from the environment to the CMakeinvocation.
google-cloud-cppis otherwise the only dependency in this repo that reaches thenetwork at build time (everything else is a pinned, checksummed submodule/tarball); this lets a
network-isolated builder point it at a local copy instead.
Testing
Built from scratch (x86_64) and linked into DALI to exercise the new GCS reader (DALI-side PR to
follow). The
-DGOOGLE_CLOUD_CPP_WITH_MOCKS=OFFand shared-library reasoning above were eachverified directly (see the inline review discussion for the specific build failures reproduced).