Skip to content

Replace fmt::localtime with localtime_r #2457

Open
tchaikov wants to merge 1 commit intoosm2pgsql-dev:masterfrom
tchaikov:fix-fmt12-localtime
Open

Replace fmt::localtime with localtime_r #2457
tchaikov wants to merge 1 commit intoosm2pgsql-dev:masterfrom
tchaikov:fix-fmt12-localtime

Conversation

@tchaikov
Copy link
Copy Markdown

@tchaikov tchaikov commented Apr 3, 2026

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
Copy link
Copy Markdown
Collaborator

lonvia commented Apr 3, 2026

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

@joto
Copy link
Copy Markdown
Collaborator

joto commented Apr 3, 2026

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.

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

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
@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
Copy link
Copy Markdown
Author

tchaikov commented Apr 3, 2026

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
Copy link
Copy Markdown
Collaborator

joto commented Apr 4, 2026

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
Copy link
Copy Markdown
Collaborator

joto commented Apr 4, 2026

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.

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