Skip to content

Replace fmt::localtime with localtime_r - #2457

Merged
joto merged 1 commit into
osm2pgsql-dev:masterfrom
tchaikov:fix-fmt12-localtime
Apr 7, 2026
Merged

Replace fmt::localtime with localtime_r #2457
joto merged 1 commit into
osm2pgsql-dev:masterfrom
tchaikov:fix-fmt12-localtime

Conversation

@tchaikov

@tchaikov tchaikov commented Apr 3, 2026

Copy link
Copy Markdown
Contributor

fmt::localtime was deprecated in fmt 11.2.0 and removed in fmt 12.0.0.
Replace with localtime_r, which is what fmt::localtime was implemented
with internally. The new implementation maintains identical behavior by
throwing a fmt::format_error exception when localtime_r() fails,
matching the original fmt::localtime() behavior.

See: https://github.com/fmtlib/fmt/releases/tag/12.0.0

Fixes #2456

@lonvia

lonvia commented Apr 3, 2026

Copy link
Copy Markdown
Collaborator

Can you please show how you have tested locally that your proposed change fixes the problem?

@joto

joto commented Apr 3, 2026

Copy link
Copy Markdown
Collaborator

There is a reason fmt::localtime was used and not std::localtime:

Converts given time since epoch as std::time_t value into calendar time, expressed in local time. Unlike std::localtime, this function is thread-safe on most platforms.
(From the fmt docs)

This shows the mess that localtime is on various platforms in various versions. This needs to be figured out and tested on all platforms.

@tchaikov
tchaikov force-pushed the fix-fmt12-localtime branch from 4f8bdbd to a55ac19 Compare April 3, 2026 08:28
@tchaikov tchaikov changed the title Replace fmt::localtime with std::localtime Replace fmt::localtime with localtime_r Apr 3, 2026
@tchaikov

tchaikov commented Apr 3, 2026

Copy link
Copy Markdown
Contributor Author

Can you please show how you have tested locally that your proposed change fixes the problem?

@lonvia hi Sarah, locally i tested it just by compiling it with fmt 12. see https://copr.fedorainfracloud.org/coprs/tchaikov/fmt-12/build/10289349/ . I am a maintainer of fmt library for fedora, so i need to fix all dependent packages that fail to build when bumping the packaged fmt library in the distro i am targeting. osm2pgsql is one of them. unfortunately, i don't use osm2pgsql myself, and i don't have a pgsql db around to run the test suite against. but i also use COPR/Koji build environment runs the full test suite. see https://download.copr.fedorainfracloud.org/results/tchaikov/fmt-12/fedora-rawhide-aarch64/10289349-osm2pgsql/builder-live.log

@joto i concur with you. so i've rewritten it and repushed the change to replicate the implementation used by fmt::localtime -- including the exception raised by it. hopefully, this would help to build more confidence to that this change is safe.

@joto

joto commented Apr 4, 2026

Copy link
Copy Markdown
Collaborator

https://en.cppreference.com/w/c/chrono/localtime.html says localtime_r is C23, but the man page says its in POSIX.1-1196 and SUSv2, so that's plenty old enough for our use case. But it doesn't look like MSVC supports it. And, in fact, the code in fmt is much more complicated than in your commit. Looks like we need to use localtime_s for Windows.

@joto

joto commented Apr 4, 2026

Copy link
Copy Markdown
Collaborator

See also fmtlib/fmt#4458 and fmtlib/fmt#4464 and other issues. We can not use std::chrono::time_zone::to_local yet, because that's C++20 (which would be okay to switch to I guess), but it might be unsupported in clang-19 which is a deal-breaker.

fmt::localtime was removed in fmt 12. Replace it with an equivalent
that is thread-safe and portable:
- POSIX: localtime_r(&time, &tm)
- MSVC:  localtime_s(&tm, &time)  (reversed argument order)

Both are re-entrant and thread-safe, matching the guarantee that
fmt::localtime provided.

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
@tchaikov
tchaikov force-pushed the fix-fmt12-localtime branch from a55ac19 to dec2937 Compare April 5, 2026 00:09
@tchaikov

tchaikov commented Apr 5, 2026

Copy link
Copy Markdown
Contributor Author

@joto hi Jochen, thank you for your insights. indeed, i completely missed that we had to be compatible with windows platforms and removed the MSVC bits in fmt's implementation. just updated the change to use localtime_s() to address the windows portability, as you pointed out std::chrono::time_zone::to_local() is not ready for us yet.

@joto
joto merged commit 6c96199 into osm2pgsql-dev:master Apr 7, 2026
24 checks passed
@joto

joto commented Apr 7, 2026

Copy link
Copy Markdown
Collaborator

Thanks @tchaikov for the PR and sticking with it!

@tchaikov
tchaikov deleted the fix-fmt12-localtime branch April 7, 2026 07:33
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.

Build fails with newer fmt libraries version 12

3 participants