Repository navigation
Add the Windows-to-IANA timezone converter (T9a) - #3
Merged
Merged
Conversation
The Game API takes an IANA timezone on a session start. A Windows game knows its timezone by a Windows name, so without a converter it either sends nothing and gets UTC server-side, or sends a name the API rejects. UTC puts local-day boundaries on the wrong day: daily logins, event windows and once_per_day buckets all land wrong. The table is generated, not hand-written. data/windows-to-iana.tsv is the source, tools/generate_timezone_table.py produces src/core/timezone_table.inc, and a CI job fails if the two drift. The data comes from the Unity SDK's LootLockerTimezoneConverter, which is the mature implementation, with one gap it has filled: Russian Standard Time appears only in its reverse table, so a Windows machine reporting it would fall through to UTC. The platform clock read is a seam, not a hard-coded OS call. Timezone and locale are both potentially user-configurable in a game, and every engine exposes them differently, so the OS value is frequently not what the player chose. TimezoneProvider is injected through Config::timezone_provider; SystemTimezoneProvider is the default and is documented as possibly wrong. Resolution order is Config::timezone, then the injected provider, then the system provider, then Etc/UTC. IANA's Etc/GMT signs are reversed - Etc/GMT+5 is UTC-5 - which the tests assert explicitly because it is the kind of thing that gets "fixed" wrongly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
readlink needs <unistd.h>, which the POSIX branch did not include. MSVC never saw it, so the Windows build was green while Linux and macOS failed. IsNumericIdentifierList was superseded by IsIdentifierList during T9 and left behind. GCC and Clang warn on it; MSVC does not, which is why it survived. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Active critical and moderate findings remain, including likely build failures on supported POSIX and unsupported non-Windows targets, unreliable numeric clamping, incomplete timezone recognition, and a contract conflict around implicit system detection.
6 open issues
Include direct declarations for snprintf and POSIX readlink · New Guard POSIX detection and use UTC on unsupported platforms · New Clamp the parsed offset before converting it to int · New Avoid implicit environment and file reads in timezone resolution · New Validate IANA zones independently of the reduced mapping table · New Add the documented Pago Pago and Niue timezone mappings · New
What changed in this PR
Adds the Windows-to-IANA timezone conversion layer, generated mapping data, injectable timezone providers, platform fallback resolution, tests, and generated-file CI validation for future session-start integration.
Changes:
- Adds the 137-entry Windows/IANA source table, generator, and committed generated C++ table.
- Adds public conversion, normalization, provider, and configuration APIs.
- Adds platform detection, resolution precedence, timezone tests, CMake integration, and CI drift checking.
The review identified unresolved compatibility, portability, compilation, normalization, and system-detection issues. This pull request requires changes.
| File | Summary | Review notes |
|---|---|---|
tools/generate_timezone_table.py |
Generates the C++ timezone table from TSV source data. | — |
tests/test_timezone.cpp |
Tests conversion, normalization, offsets, fallback, and provider resolution. | — |
tests/CMakeLists.txt |
Registers the timezone test executable and CTest entry. | — |
src/core/timezone.cpp |
Implements conversion, normalization, provider resolution, and platform detection. | Critical: add required direct headers (1 vote); guard POSIX APIs on unsupported platforms (2 votes). Moderate: clamp numeric values before casting (2 votes); preserve canonical IANA casing (1 vote); validate broader IANA names (3 votes); address implicit environment/file reads (4 votes); support leading-colon TZ values (1 vote). |
src/core/timezone_table.inc |
Contains the generated timezone mapping table. | — |
include/lootlocker/timezone.h |
Defines the public timezone conversion and provider APIs. | Nit: add or document the Pacific/Pago_Pago and Pacific/Niue reverse mappings (2 votes). |
include/lootlocker/config.h |
Adds timezone override and provider configuration fields. | Moderate: preserve positional aggregate-initialization compatibility when adding timezone_provider (1 vote). |
data/windows-to-iana.tsv |
Provides the source-of-truth Windows-to-IANA mappings. | — |
CMakeLists.txt |
Adds the timezone implementation to the core library. | — |
.github/workflows/build.yml |
Adds generated-file consistency validation to CI. | — |
Review effort: Lite
IsKnownIanaTimezone was a table lookup, which is wrong: the table is a Windows mapping, not the IANA database, so it holds one representative per Windows zone and misses real names. A system reporting America/Argentina/Buenos_Aires was rejected and silently fell through to UTC. It is now a shape check on a well-formed Area/Location path, and the server stays the authority on whether a zone exists. The numeric offset path cast strtol's long to int before clamping, which is implementation-defined for an out-of-range value. Clamping now happens while the value is still a long. This only bites where long is wider than int, so the Windows run alone would not have caught it. The POSIX branch was the #else of _WIN32, so any non-POSIX target compiled getenv and readlink and failed to build. It is now guarded on __unix__ or __APPLE__, with an explicit unsupported-platform branch that returns nothing and lets ResolveTimezone fall through to UTC, which is what the contract promises. Missing includes: <cstdio> for std::snprintf, and <unistd.h> guarded for non-Windows. The header documented Pacific/Pago_Pago and Pacific/Niue as reverse mappings, but neither is in the table. Replaced with an accurate statement of what the reverse conversion accepts. Config::timezone being empty used to mean "derive from the platform clock", and the spec promised an adopter could send no timezone at all. Empty now means "resolve it", so that promise needed its own representation: Config::send_timezone, defaulting to true, with ResolveTimezone returning an empty string when it is false. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Implements T9a: the Windows-to-IANA timezone converter, plus the
TimezoneProviderseam the owner asked for.Why
The Game API takes an IANA timezone on a session start. A Windows game knows its timezone by a Windows name, so without a converter it either sends nothing and gets UTC server-side, or sends a name the API rejects. UTC puts local-day boundaries on the wrong day: daily logins, event windows and
once_per_daybuckets all land wrong.The table is generated, not hand-written
data/windows-to-iana.tsv— 137 pairs, the source of truth.tools/generate_timezone_table.py— producessrc/core/timezone_table.inc.generated-filesCI job fails if the two drift.The data comes from the Unity SDK's
LootLockerTimezoneConverter, which is the mature implementation, with one gap it has filled: Russian Standard Time appears only in its reverse table, so a Windows machine reporting it would fall through to UTC.The platform clock is a seam
Timezone and locale are both potentially user-configurable in a game, and every engine exposes them differently, so the OS value is frequently not what the player chose. An SDK that only reads the OS is confidently wrong on a machine whose owner picked a different timezone in-game.
TimezoneProvider— public interface, one method, injected throughConfig::timezone_provider.SystemTimezoneProvider— the default:GetDynamicTimeZoneInformationon Windows,TZthen/etc/localtimeon POSIX, UTC on platforms we do not cover.Config::timezone, then the injected provider, then the system provider, thenEtc/UTC.Verification
FixedProvider.generated-filescheck was verified by hand-editing the table and watching it fail.LOOTLOCKER_MODULE_ONLY=authstill builds, so the module boundary holds.Not covered here
The task's verification also asks for "a session start from a Windows timezone name sends the IANA equivalent". That needs T14.
ResolveTimezoneis the function a session start will call, and it is covered directly.Spec
lootlocker/index#1878 records the seam as a revision to the timezone section.