-
Notifications
You must be signed in to change notification settings - Fork 736
[3rd-party] Migrate "fmt" dependency to vcpkg #3964
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
Conversation
|
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. |
84c5ea5 to
fcfd598
Compare
|
This PR does the vcpkg and grpc update, it is just needed to reviewed and merged. |
|
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. |
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. |
I'm just trying to avoid the obstacle to see whether anything else is blocking the road. The CI is a weird beast :) |
185f25e to
516627d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. 🚀 New features to boost your workflow:
|
516627d to
4ad6d56
Compare
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). |
Great! I'll help review that branch next week. |
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. |
9aa2b26 to
ed2bf64
Compare
ed2bf64 to
eea0686
Compare
|
@georgeliao rebased this & private side. should be good to go. |
bccc772 to
2c94431
Compare
2c94431 to
8d2ac87
Compare
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>
8d2ac87 to
08214db
Compare
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>
There was a problem hiding this 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.
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