Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3rd-party] Migrate "fmt" dependency to vcpkg #3964

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

xmkg
Copy link
Contributor

@xmkg xmkg commented Feb 28, 2025

Right now, we’re using fmt as a submodule, which is not the most convenient way of consuming it considering the fact that we already have “vcpkg” available to our disposal.

This merge proposal removes the "fmt" submodule and introduces a "fmt" vcpkg dependency, updating the required CMakeLists references in the process.

MULTI-1846

the package catalogue for fetching fmt/11.0.2

Signed-off-by: Mustafa Kemal Gilor <[email protected]>
@xmkg
Copy link
Contributor Author

xmkg commented Feb 28, 2025

The vcpkg version is bumped to 2025.02.14, but the vcpkg baseline is intentionally not updated because gRPC 1.52.1 is not happy with the some changes that happened in "abseil" in the latest baseline.

removed "fmt" submodule, and added fmt/11.0.2 as vcpkg dependency.

added a find_package() call for fmt.

replaced all "fmt" CMakeLists references with fmt::fmt-header-only

Signed-off-by: Mustafa Kemal Gilor <[email protected]>
@xmkg xmkg force-pushed the enhancement/migrate-fmt-to-vcpkg branch from 84c5ea5 to fcfd598 Compare February 28, 2025 15:07
@georgeliao
Copy link
Contributor

This PR does the vcpkg and grpc update, it is just needed to reviewed and merged.

@georgeliao
Copy link
Contributor

georgeliao commented Feb 28, 2025

I think on CI, the vcpkg version and the baseline hash should match, maybe it is wiser to wait that update PR to merge or just rebase this PR on that.

@xmkg
Copy link
Contributor Author

xmkg commented Feb 28, 2025

I think on CI, the vcpkg version and the baseline hash should match.

I think that's not a hard requirement, and as a matter of fact, I'm working on a solution to fix that so we can have different vcpkg and baseline commits if we want to.

@xmkg
Copy link
Contributor Author

xmkg commented Feb 28, 2025

maybe it is wiser to wait that update PR to merge or just rebase this PR on that.

I'm just trying to avoid the obstacle to see whether anything else is blocking the road. The CI is a weird beast :)

@xmkg xmkg force-pushed the enhancement/migrate-fmt-to-vcpkg branch 3 times, most recently from 185f25e to 516627d Compare February 28, 2025 18:10
Copy link

codecov bot commented Feb 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.11%. Comparing base (c00e485) to head (9aa2b26).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3964   +/-   ##
=======================================
  Coverage   89.11%   89.11%           
=======================================
  Files         255      255           
  Lines       14603    14603           
=======================================
  Hits        13014    13014           
  Misses       1589     1589           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

The submodule cloning only fetches the top commit and the tags,
so a different builtin-baseline from the HEAD would cause pipeline
fail because it would be absent in the submodule's git tree.
This commit adds two steps to the checkout action, which reads the
vcpkg.json file and fetches the builtin-baseline commit.

Signed-off-by: Mustafa Kemal Gilor <[email protected]>
@xmkg xmkg force-pushed the enhancement/migrate-fmt-to-vcpkg branch from 516627d to 4ad6d56 Compare February 28, 2025 18:59
@xmkg
Copy link
Contributor Author

xmkg commented Feb 28, 2025

I think that's not a hard requirement, and as a matter of fact, I'm working on a solution to fix that so we can have different vcpkg and baseline commits if we want to.

We can do that now. The solution tries to be smart about what to fetch to help reduce the checkout time. The alternative is to use "git fetch --unshallow" but it fetches all the references we won't need, and it takes significantly longer (~3x).

@xmkg
Copy link
Contributor Author

xmkg commented Feb 28, 2025

This PR does the vcpkg and grpc update, it is just needed to reviewed and merged.

Great! I'll help review that branch next week.

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.

2 participants