Switch inject-browser-sdk to submodule; consolidate deps#61
Merged
pawelchcki merged 5 commits intomainfrom Apr 21, 2026
Merged
Conversation
Contributor
Author
pawelchcki
force-pushed
the
pc/submodule-deps
branch
3 times, most recently
from
92f2ff9 to
52d97f8
Compare
pawelchcki
force-pushed
the
pc/submodule-deps
branch
from
52d97f8 to
c499813
Compare
pawelchcki
force-pushed
the
graphite-base/61
branch
from
223a6ca to
e27234e
Compare
pawelchcki
requested review from
Copilot and
zacharycmontoya
and removed request for
a team
pawelchcki
force-pushed
the
pc/submodule-deps
branch
from
c499813 to
6b73a7b
Compare
There was a problem hiding this comment.
Pull request overview
This PR migrates inject-browser-sdk from FetchContent to a git submodule to avoid configure-time cloning (and credential issues), and consolidates third-party dependency wiring under deps/CMakeLists.txt.
Changes:
- Add
deps/inject-browser-sdkas a git submodule and wire it into the build whenHTTPD_DATADOG_ENABLE_RUMis enabled. - Move fmt + RUM-related dependency setup out of the top-level
CMakeLists.txtintodeps/CMakeLists.txt. - Simplify
cmake/deps/fmt.cmaketo useFetchContent_MakeAvailable(and addEXCLUDE_FROM_ALL).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
deps/CMakeLists.txt |
Centralizes fmt + RUM dependency setup and adds submodule-based inject-browser-sdk integration. |
cmake/deps/fmt.cmake |
Switches fmt fetching to FetchContent_MakeAvailable and marks it EXCLUDE_FROM_ALL. |
CMakeLists.txt |
Removes top-level fmt and RUM FetchContent wiring (now handled under deps/). |
.gitmodules |
Adds the deps/inject-browser-sdk submodule entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Using FetchContent meant cmake had to clone inject-browser-sdk (a private repo) at configure time, which broke the devcontainer workflow because the container has no GitHub credentials. Converting it to a git submodule reuses the existing mechanism the repo already relies on for dd-trace-cpp and nginx-datadog — CI initializes it via GIT_SUBMODULE_STRATEGY: recursive, and local dev gets a checked-out tree with no extra auth. The submodule is pinned at 49245e1c6ed8, the previous FetchContent GIT_TAG. Since deps/CMakeLists.txt already handles dd-trace-cpp and is now handling inject-browser-sdk too, the top-level CMakeLists.txt's include(cmake/deps/fmt.cmake) and the HTTPD_DATADOG_ENABLE_RUM block move into deps/CMakeLists.txt so all third-party wiring lives in one place. fmt.cmake itself is simplified to FetchContent_MakeAvailable with EXCLUDE_FROM_ALL in Declare: both features require CMake 3.14 and 3.28 respectively, and dd-trace-cpp already forces 3.28 as the effective minimum, so the old manual FetchContent_GetProperties + FetchContent_Populate + add_subdirectory dance is no longer needed.
- cmake/deps/fmt.cmake: drop the FetchContent_MakeAvailable refactor and keep main's Populate + add_subdirectory pattern. The refactor used EXCLUDE_FROM_ALL as a FetchContent_Declare keyword, which requires CMake 3.28+, but the project advertises 3.12+ in both CMakeLists.txt and CONTRIBUTING.md. This revert keeps fmt.cmake identical to main and drops an ancillary change that wasn't load-bearing for the submodule work. - deps/CMakeLists.txt: quote the path in the EXISTS check so it survives spaces/semicolons (Windows paths, etc.).
pawelchcki
force-pushed
the
pc/submodule-deps
branch
from
48dc9fa to
2641675
Compare
checkout with submodules: true tried to clone inject-browser-sdk, which is a private repo that the default GITHUB_TOKEN can't access, so the build job failed at checkout before cmake ever ran. None of the GitHub Actions presets (ci-dev, ci-release) enable HTTPD_DATADOG_ENABLE_RUM, so inject-browser-sdk is never needed in these workflows. Drop submodules: true and explicitly init the two submodules the build actually consumes: deps/dd-trace-cpp and deps/nginx-datadog.
|
🎯 Code Coverage (details) 🔗 Commit SHA: 1330904 | Docs | Datadog PR Page | Give us feedback! |
Without access to DataDog/inject-browser-sdk, --init --recursive fails at clone time. Document the two-submodule command that covers standard builds and mention --recursive as the RUM-build opt-in.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Matches the PR description: dd-trace-cpp already forces CMake 3.28 as the effective minimum, so the manual FetchContent_GetProperties + FetchContent_Populate + add_subdirectory dance is no longer needed. EXCLUDE_FROM_ALL moves onto the Declare() call where MakeAvailable picks it up in 3.28+.
Contributor
Author
Merge activity
|
xlamorlette-datadog
approved these changes
Apr 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moving back to submodule for inject-browser-sdk, as this simplifies the setup for running integration tests within a container to satisfy permissions within the container.
FetchContent forced cmake to clone inject-browser-sdk (private repo) at configure time, which broke devcontainer builds with no GitHub credentials. Switch to a submodule (pinned at
49245e1c6ed8) so CI's existingGIT_SUBMODULE_STRATEGY: recursiveand local checkouts both work with no extra auth.While here, move fmt and the RUM wiring into
deps/CMakeLists.txtso all third-party setup lives in one place, and simplifyfmt.cmaketoFetchContent_MakeAvailablenow that dd-trace-cpp already forces CMake 3.28.