[Windows] Fix incorrect TimeZone.current lookup logic
#975
Conversation
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
Contributor
Author
|
@swift-ci please test |
parkera
approved these changes
Oct 10, 2024
compnerd
approved these changes
Oct 11, 2024
| let result: String? = timeZoneIdentifier.withUnsafeBufferPointer { identifier in | ||
| return _withResizingUCharBuffer { buffer, size, status in | ||
| let len = ucal_getTimeZoneIDForWindowsID(identifier.baseAddress, Int32(identifier.count), nil, buffer, size, &status) | ||
| if status.isSuccess { |
Member
There was a problem hiding this comment.
return status.isSuccess ? len : nil feels like a better way to write this.
Contributor
Author
There was a problem hiding this comment.
Yeah that seems fine too, I was just copying the format of the above getCanonicalTimeZoneID function for consistency
jmschonfeld
added a commit
to jmschonfeld/swift-foundation
that referenced
this pull request
Oct 11, 2024
jmschonfeld
added a commit
to jmschonfeld/swift-foundation
that referenced
this pull request
Oct 11, 2024
jmschonfeld
added a commit
that referenced
this pull request
Oct 11, 2024
kateinoigakukun
added a commit
to kateinoigakukun/swift-foundation
that referenced
this pull request
Oct 16, 2024
swiftlang#975 started to restrict the fallback value for `TZDIR` and it revealed that WASI platform implicitly depends on TZDIR even though it won't have such directory. This patch explicitly handles the case for WASI platform for timezone operations.
kateinoigakukun
added a commit
that referenced
this pull request
Oct 18, 2024
#975 started to restrict the fallback value for `TZDIR` and it revealed that WASI platform implicitly depends on TZDIR even though it won't have such directory. This patch explicitly handles the case for WASI platform for timezone operations.
cthielen
pushed a commit
to cthielen/swift-foundation
that referenced
this pull request
Nov 8, 2024
cthielen
pushed a commit
to cthielen/swift-foundation
that referenced
this pull request
Nov 8, 2024
swiftlang#975 started to restrict the fallback value for `TZDIR` and it revealed that WASI platform implicitly depends on TZDIR even though it won't have such directory. This patch explicitly handles the case for WASI platform for timezone operations.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The current implementation of
TimeZone.currentattempts to read from/etc/localtimewhich is not a path that exists on Windows. This leads toTimeZone.currentalways being GMT on Windows. Instead, this updates the implementation to callGetTimeZoneInformation, convert the Windows name to an ICU system timezone name, and initialize a timezone with that name (this matches the behavior ofTimeZoneon Swift 5.10 when it was based onCFTimeZone). This also restricts the default values forTZDEFAULT/TZDIRto Darwin + Linux where the paths are reasonable unlike Windows where we know these constants should never exist to avoid accidental usage in the future.There's likely more work to be done here for Windows for a full solution (to allow in some way for developers to interface with Windows time zone identifiers so that
TimeZonecan interop with Windows system APIs) but this change at least fixes the critical issue and restores the Swift 5.10 behavior, we can look into adding additional API for working with Windows time zone identifiers in the future.Resolves #966, rdar://137451317