Skip to content

Fix CFTimeZone crashes on Windows #5070

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 16, 2024

Conversation

jmschonfeld
Copy link
Contributor

CFTimeZone on Windows requires mappings between the Windows and Olson timezone identifiers. We unfortunately lost those mappings in the build for the re-core. This PR adds those mappings back which resolves crashes when calling into CFTimeZone. We add them back as literals in code rather than as plist resource files to make it easily compatible with both the CMake and SwiftPM builds. I've confirmed that when running tests on Windows the tests no longer crash with CFTimeZone issues

@jmschonfeld
Copy link
Contributor Author

@swift-ci please test

@jmschonfeld jmschonfeld force-pushed the windows/fix-cftimezone branch from 59269b1 to 48e7023 Compare August 15, 2024 18:35
@jmschonfeld
Copy link
Contributor Author

@swift-ci please test

@jmschonfeld
Copy link
Contributor Author

@swift-ci please test Windows platform

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with the approach, but I think that we should update the script that generates the mapping.

@jmschonfeld
Copy link
Contributor Author

I'm okay with the approach, but I think that we should update the script that generates the mapping.

Ah I didn't realize there was a script that generates the mapping - do you know where that lives?

@jmschonfeld
Copy link
Contributor Author

@swift-ci please test

@jmschonfeld
Copy link
Contributor Author

@swift-ci please test Windows platform

@compnerd
Copy link
Member

I'm okay with the approach, but I think that we should update the script that generates the mapping.

Ah I didn't realize there was a script that generates the mapping - do you know where that lives?

I could've sworn that I checked that in >.< ... maybe I didn't - it was some XSLT horror. I might've just shoved it into the commit message :(. For now, lets go ahead and get this merged.

}
__CFTimeZoneIdentifierPair *current = data;
while (current->source) {
if (strcmp(current->source, buffer) == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably do better than a linear search as a polish later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah ideally maybe we could pre-sort this and perform a binary search or perhaps even a fancier solution with some calculated hashes to create a dictionary-like lookup, but I'll do that as a followup on main and just cherry-pick this back to 6.0

@itingliu
Copy link
Contributor

Do we want this in release/6.0 too if it's crashing the tests?

@jmschonfeld
Copy link
Contributor Author

Do we want this in release/6.0 too if it's crashing the tests?

Yeah, this causes crashes in things that call down to some CFTimeZone functions (like DateFormatter) so I'll cherry-pick this back to release/6.0 too

@jmschonfeld jmschonfeld merged commit 2973067 into swiftlang:main Aug 16, 2024
2 of 3 checks passed
@jmschonfeld jmschonfeld deleted the windows/fix-cftimezone branch August 16, 2024 16:47
jmschonfeld added a commit to jmschonfeld/swift-corelibs-foundation that referenced this pull request Aug 16, 2024
* Fix CFTimeZone crashes on Windows

* Fix build failure
jmschonfeld added a commit that referenced this pull request Aug 16, 2024
* Fix CFTimeZone crashes on Windows

* Fix build failure
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.

3 participants