Skip to content

Add Google Cloud SDK dependency and build scripts. - #178

Merged
jantonguirao merged 10 commits into
NVIDIA:mainfrom
mzient:gcs
Sep 18, 2026
Merged

jantonguirao merged 10 commits into
NVIDIA:mainfrom
mzient:gcs

Conversation

@mzient

@mzient mzient commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

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 the storage component is requested
    (GOOGLE_CLOUD_CPP_ENABLE=storage), but that transitively pulls in monitoring, trace,
    opentelemetry and universe_domain regardless (verified: trying to drop them fails to build,
    not just configure - see build_google-cloud-cpp.sh). That's where gRPC and
    opentelemetry-cpp come 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) since
    nothing 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 by
    build_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 shared
    install prefix and reused by both aws-sdk-cpp and google-cloud-cpp, instead of
    build_aws-sdk-cpp.sh building its own private static copies of both (which just duplicated
    the work).

Key decisions

  • google-cloud-cpp is built shared, unlike everything else here. libdali.so and
    libdali_operators.so both use the GCS client; a gcs::Client constructed by one and used by
    the 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 .so would get its own private copy and
    passing 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=OFF drops googletest/benchmark from this dependency set
    entirely - 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_URL is forwarded from the environment to the CMake
    invocation. google-cloud-cpp is otherwise the only dependency in this repo that reaches the
    network 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=OFF and shared-library reasoning above were each
verified directly (see the inline review discussion for the specific build failures reproduced).

Comment thread build_scripts/build_deps.sh Outdated
Comment thread build_scripts/build_grpc.sh
Comment thread build_scripts/build_benchmark.sh Outdated
Comment thread build_scripts/build_google-cloud-cpp.sh Outdated
Comment thread build_scripts/build_google-cloud-cpp.sh Outdated
Comment thread build_scripts/build_googletest.sh Outdated
mzient and others added 5 commits September 15, 2026 18:18
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>
@jantonguirao
jantonguirao marked this pull request as ready for review September 17, 2026 08:42
Copilot AI lite review requested due to automatic review settings September 17, 2026 08:42

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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.

Comment thread build_scripts/generate_toolchain_file.sh Outdated
@greptile-apps

greptile-apps Bot commented Sep 17, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The PR appears safe to merge, with no new actionable findings and the prior compiler-detection thread manually resolved.

Summary

This PR adds the Google Cloud Storage C++ client and its required dependency chain to the DALI dependency build.

  • Adds pinned submodules and build scripts for google-cloud-cpp, gRPC, OpenTelemetry C++, and nlohmann_json.
  • Extracts reusable OpenSSL, curl, and CMake toolchain setup into shared build steps.
  • Updates AWS SDK builds to consume OpenSSL and curl from the common installation prefix.
  • Handles host tools during cross-compilation and installs the Google Cloud libraries as shared objects.
  • Documents dependency versions and non-obvious Google Cloud build constraints.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  Build[build_deps.sh] --> Protobuf[protobuf and Abseil]
  Build --> OpenSSL[OpenSSL]
  OpenSSL --> Curl[libcurl]
  Protobuf --> GRPC[gRPC]
  OpenSSL --> GRPC
  Build --> JSON[nlohmann_json]
  Build --> OTel[opentelemetry-cpp]
  Curl --> AWS[AWS SDK shared libraries]
  OpenSSL --> AWS
  Curl --> GCS[google-cloud-cpp storage shared libraries]
  OpenSSL --> GCS
  GRPC --> GCS
  JSON --> GCS
  OTel --> GCS
Loading

Reviews (5) · Last reviewed commit: "Address review: self-contained cross-com..."

Comment thread build_scripts/generate_toolchain_file.sh Outdated
Comment thread build_scripts/build_grpc.sh Outdated
Comment thread build_scripts/build_google-cloud-cpp.sh Outdated
…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 rostan-t left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread build_scripts/generate_toolchain_file.sh Outdated
Comment thread CLAUDE.md Outdated
…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.
@jantonguirao
jantonguirao merged commit c9c3799 into NVIDIA:main Sep 18, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants