Update vendored spdlog library to 1.17.0 (latest)#8530
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f0914bf06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
vandonr
left a comment
There was a problem hiding this comment.
I can compile on my mac with this
(ran ./tracer/build.sh Clean BuildTracerHome)
BenchmarksBenchmark execution time: 2026-04-28 16:13:57 Comparing candidate commit c3703cd in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 27 metrics, 0 unstable metrics, 58 known flaky benchmarks, 29 flaky benchmarks without significant changes.
|
dcd414a to
d6621cd
Compare
…ld, and we don't need it anyway
Summary of changes
/utf8Reason for change
We want to update spdlog, partly because newer versions of macos (Tahoe) require a newer version of
fmt, which spdlog uses.Implementation details
UpdateVendorstool to allow vendoring non-C# code #8529) to use 1.17.0 (current latest), which also bumps{fmt}library to 12.1.0.{fmt}requires narrow string literals to be UTF-8 encoded via astatic_assert:use_utf8is false on Windows unless/utf-8is passed (it tests whether"\u00A7"[1] == '\xA7', which only holds when narrow literals are UTF-8). Our Windows native projects compile with the default execution charset, so this fails.That gives us two options:
FMT_UNICODE=0to disable unicode support in{fmt}/utf-8to enable unicode support everywhereChose
FMT_UNICODE=0as it preserves the existing execution-charset behavior — adding/utf-8would change the encoding of every narrow string literal in our codebase, which seems higher risk. We log viastd::string(ASCII-only log messages), so disabling fmt's Unicode requirement should be safe.Test coverage
This is the test, if the build works and the tests pass, I'll check the log files for anything suspicious, but otherwise we should be good.
Other details
You may want to review the 3 commits separately:
UpdateVendorstool to update the vendored codeStacked on Update UpdateVendors tool to allow vendoring non-C# code#8529