Skip to content

Conversation

@xmkg
Copy link
Member

@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

@xmkg
Copy link
Member 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.

@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
Member 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
Member 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
@codecov
Copy link

codecov bot commented Feb 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.53%. Comparing base (896fcb7) to head (47e0fa0).
Report is 3442 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3964   +/-   ##
=======================================
  Coverage   89.53%   89.53%           
=======================================
  Files         260      260           
  Lines       14741    14741           
=======================================
  Hits        13198    13198           
  Misses       1543     1543           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@xmkg xmkg force-pushed the enhancement/migrate-fmt-to-vcpkg branch from 516627d to 4ad6d56 Compare February 28, 2025 18:59
@xmkg
Copy link
Member 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
Member 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.

@xmkg
Copy link
Member Author

xmkg commented Mar 3, 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).

Changes passed the public CI. The private one is failing due to a merge conflict. I'll try to see if I can steer around it.

@xmkg
Copy link
Member Author

xmkg commented Mar 4, 2025

Changes passed the public CI. The private one is failing due to a merge conflict. I'll try to see if I can steer around it.

A separate MP for the private side is proposed to fix the merge conflicts.

@xmkg xmkg requested a review from georgeliao March 7, 2025 07:23
@xmkg xmkg force-pushed the enhancement/migrate-fmt-to-vcpkg branch from 9aa2b26 to ed2bf64 Compare March 11, 2025 13:03
@xmkg xmkg force-pushed the enhancement/migrate-fmt-to-vcpkg branch from ed2bf64 to eea0686 Compare April 3, 2025 11:15
@xmkg
Copy link
Member Author

xmkg commented Apr 3, 2025

@georgeliao rebased this & private side. should be good to go.

@xmkg xmkg force-pushed the enhancement/migrate-fmt-to-vcpkg branch 4 times, most recently from bccc772 to 2c94431 Compare April 3, 2025 13:33
@xmkg xmkg force-pushed the enhancement/migrate-fmt-to-vcpkg branch from 2c94431 to 8d2ac87 Compare April 3, 2025 14:21
@ricab ricab requested a review from sharder996 April 4, 2025 15:06
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 <mustafa.gilor@canonical.com>
@xmkg xmkg force-pushed the enhancement/migrate-fmt-to-vcpkg branch from 8d2ac87 to 08214db Compare April 15, 2025 11:05
sometimes, the dependency port files may refer to a revision that
is different than the HEAD, which causes vcpkg to fail since the
revision needed to satisfy the dependency is not available.

the "Check out submodules" step now unshallows the vcpkg to avoid
that.

Signed-off-by: Mustafa Kemal Gilor <mustafa.gilor@canonical.com>
Copy link
Contributor

@georgeliao georgeliao left a comment

Choose a reason for hiding this comment

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

It looks good to me. Thanks for the good work, especially the fix on github action.

@sharder996 sharder996 added this pull request to the merge queue Apr 23, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 23, 2025
@xmkg xmkg added this pull request to the merge queue Apr 23, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 23, 2025
@xmkg xmkg added this pull request to the merge queue Apr 23, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 23, 2025
@xmkg xmkg added this pull request to the merge queue Apr 24, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 24, 2025
@ricab ricab merged commit e9806b8 into main Apr 24, 2025
15 checks passed
@ricab ricab deleted the enhancement/migrate-fmt-to-vcpkg branch April 24, 2025 09:06
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.

4 participants