Determine the Timezone directory to use at runtime on macOS. #1126
Conversation
|
/cc @ianpartridge @parkera Here is a possible solution to the TZDIR issue on macOS |
| } | ||
|
|
||
| // Timezone files moved in High Sierra(10.13) | ||
| if (major == 10 && minor >= 13) { |
There was a problem hiding this comment.
If major ever changes, this is going to provide the wrong behaviour here, surely? Wouldn't it be better to use major == 10 && minor < 13 as the fallback condition, and then everything else as the new condition?
There was a problem hiding this comment.
Yeah good catch, I will swap it round. Of course it will also break if the TZDIR is changed again - I was assuming any changes of that magnitude would be caught in the beta period :)
- The timezone directory changed on macOS 10.13 so get the os version at runtime and set the timezone directory dynamically. This allows the tests to be run on 10.12 and 10.13 - Added __InitTZStrings() for both Linux and macOS to work the same way as Windows.
spevans
force-pushed
the
pr_macos_tzdir_fix
branch
from
1ae6f34 to
80acf1c
Compare
|
@alblue I swapped the test around. |
|
I confirm this PR works for me, and the tests now run on Sierra. I don't have a High Sierra machine to test on, unfortunately. The patch touches a fair amount of |
|
@swift-ci please test |
|
is there anyway to get the swift-ci bot to also test on macOS? Am I correct in thinking only Linux is tested? |
|
AFAIK only Linux is tested by the CI. I agree it would be nice to run the tests on macOS too, I don't know what would be needed to make that happen. |
|
We're talking to @shahmishal about getting macOS tests running here. I think it's important to get that started, probably after we wrap up this release. |
|
Thanks @parkera. How does this PR look to you? |
|
Looks good to me. |
|
@swift-ci please test and merge |
|
Thanks for working on this @spevans 👍 |
|
@spevans can we re-apply this to master after the re-apply of the import? |
|
There was also a 2nd PR to fix an off-by-one error that this one introduced. I will create a new PR that covers both and test it on 10.12 then that should solve it |
|
@swift-ci please test |
The timezone directory changed on macOS 10.13 so get the os version
at runtime and set the timezone directory dynamically. This allows
the tests to be on 10.12 and 10.13
Added __InitTZStrings() for both Linux and macOS to work the same way as Windows.
Tested on Linux and 10.12 but not 10.13