Fix Time Zones Adjustment Rules on Linux by tarekgh · Pull Request #49733 · dotnet/runtime

tarekgh · 2021-03-17T00:55:44Z

Fixes #20626 and #20624

When running on Linux based systems, we store the time zone adjustment rules differently than we store for Windows. When users ask for the adjustment rules for any zone, we try to convert the internal stored rules into the form that match Windows rules. We had some bugs in this conversion and this change is fixing that. The change in the code has some comment describing what we are doing during this conversion.

I tested this manually for some time zones, but I am going to add some test later to this PR.

Note, I am planning to export a new property in the adjustment rule to report the UTC offset for the rule. So, the list of the reported rules would be more useful even on Windows.

tarekgh · 2021-03-17T00:56:54Z

@eerhardt could you please have a look at this change? Thanks!

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs

lewing · 2021-03-17T01:48:31Z

does this also fix #49491 ?

lewing

Are there any tests for these paths?

tarekgh · 2021-03-17T03:15:18Z

@lewing

does this also fix #49491 ?

No, this is different issue talking about the serialization. We already have another issue tracking same issue, so I closed #49491. Also, this is old functionality, and we are not recommending it. In other word we should obsolete it.

Are there any tests for these paths?

What paths? we have many time zone tests in general under the System.Runtime. If you are talking about the serialization, it is broken functionality on Linux Systems and as I mentioned this should be obsoleted.

tarekgh · 2021-03-18T16:25:45Z

@safern could you look too?

@eerhardt did you have a chance to take a quick look at this one?

eerhardt · 2021-03-18T20:53:19Z

but I am going to add some test later to this PR.

Do you plan on adding tests before merging?

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs

tarekgh · 2021-03-18T23:34:39Z

Do you plan on adding tests before merging?

I started to write the test but found would be better to write the test after exposing the rule UTC offset. Is it ok we can postpone the test till we expose the UTC offset?

eerhardt · 2021-03-18T23:54:35Z

Is it ok we can postpone the test till we expose the UTC offset?

I would think we could at least write the test for an adjustment rule spanning multiple years, and verifying the data returned is correct. When the UTC offset API gets added, we can modify the test to verify the new UTC offset values are correct.

tarekgh · 2021-03-19T00:22:51Z

I would think we could at least write the test for an adjustment rule spanning multiple years, and verifying the data returned is correct. When the UTC offset API gets added, we can modify the test to verify the new UTC offset values are correct.

I was trying to avoid assuming any rule in any time zone, but I am ok to add a test for like for America/Los_Angeles looking at the reported rules matching the original rule spanning multiple years.

eerhardt · 2021-03-19T00:28:26Z

I was trying to avoid assuming any rule in any time zone

We already have tests like this, which assume the exact rules of the Africa/Casablanca zone.

runtime/src/libraries/System.Runtime/tests/System/TimeZoneInfoTests.cs

Lines 1798 to 1820 in 9f93bcb

    
           [Theory] 
        
           [PlatformSpecific(TestPlatforms.AnyUnix)]  // Linux will use local mean time for DateTimes before standard time came into effect. 
        
           [InlineData("1940-02-24T23:59:59.0000000Z", false, "0:00:00")] 
        
           [InlineData("1940-02-25T00:00:00.0000000Z", true, "1:00:00")] 
        
           [InlineData("1940-11-20T00:00:00.0000000Z", true, "1:00:00")] 
        
           [InlineData("1940-12-31T23:59:59.0000000Z", true, "1:00:00")] 
        
           [InlineData("1941-01-01T00:00:00.0000000Z", true, "1:00:00")] 
        
           [InlineData("1945-02-24T12:00:00.0000000Z", true, "1:00:00")] 
        
           [InlineData("1945-11-17T01:00:00.0000000Z", true, "1:00:00")] 
        
           [InlineData("1945-11-17T22:59:59.0000000Z", true, "1:00:00")] 
        
           [InlineData("1945-11-17T23:00:00.0000000Z", false, "0:00:00")] 
        
           public static void IsDaylightSavingTime_CasablancaMultiYearDaylightSavings(string dateTimeString, bool expectedDST, string expectedOffsetString) 
        
           { 
        
               // Africa/Casablanca had DST from 
        
               //     1940-02-25T00:00:00.0000000Z {+01:00:00 DST=True} 
        
               //     1945-11-17T23:00:00.0000000Z { 00:00:00 DST=False} 
        
               DateTime dt = DateTime.ParseExact(dateTimeString, "o", CultureInfo.InvariantCulture, DateTimeStyles.AdjustToUniversal); 
        
               VerifyDST(s_casablancaTz, dt, expectedDST); 
        
               TimeSpan offset = TimeSpan.Parse(expectedOffsetString, CultureInfo.InvariantCulture); 
        
               Assert.Equal(offset, s_casablancaTz.GetUtcOffset(dt)); 
        
           }

tarekgh · 2021-03-19T00:30:36Z

We already have tests like this, which assume the exact rules of the Africa/Casablanca zone.

That is exactly the point I want to avoid. These tests fail on the platforms which has Julian rules we don't support.

eerhardt

This looks good to me. Only a couple minor/nit comments.

Thanks @tarekgh!

src/libraries/System.Runtime/tests/System/TimeZoneInfoTests.cs

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs

src/libraries/System.Runtime/tests/System/TimeZoneInfoTests.cs

tarekgh added 2 commits March 16, 2021 17:42

Fix Time Zones Adjustment Rules on Linux

7113632

Comment

be75f6e

tarekgh added this to the 6.0.0 milestone Mar 17, 2021

tarekgh self-assigned this Mar 17, 2021

tarekgh added the area-System.Runtime label Mar 17, 2021

tarekgh requested a review from eerhardt March 17, 2021 00:57

stephentoub reviewed Mar 17, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs Outdated Show resolved Hide resolved

lewing reviewed Mar 17, 2021

View reviewed changes

Init list with initial capacity

b2cd601

This was referenced Mar 17, 2021

OSX deprovision jaredpar/runfo#41

Closed

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

tarekgh mentioned this pull request Mar 17, 2021

Developers can benefit from enhanced Date and Time types and Timezone support #45318

Closed

4 tasks

eerhardt reviewed Mar 18, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs Outdated Show resolved Hide resolved

eerhardt reviewed Mar 18, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs Show resolved Hide resolved

eerhardt reviewed Mar 18, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs Show resolved Hide resolved

safern reviewed Mar 18, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs Show resolved Hide resolved

Address the feedback

4d24261

eerhardt approved these changes Mar 19, 2021

View reviewed changes

src/libraries/System.Runtime/tests/System/TimeZoneInfoTests.cs Show resolved Hide resolved

src/libraries/System.Runtime/tests/System/TimeZoneInfoTests.cs Show resolved Hide resolved

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs Outdated Show resolved Hide resolved

More Feedback

738070c

safern reviewed Mar 19, 2021

View reviewed changes

src/libraries/System.Runtime/tests/System/TimeZoneInfoTests.cs Outdated Show resolved Hide resolved

safern reviewed Mar 19, 2021

View reviewed changes

src/libraries/System.Runtime/tests/System/TimeZoneInfoTests.cs Outdated Show resolved Hide resolved

safern approved these changes Mar 19, 2021

View reviewed changes

feedback

de307d7

tarekgh merged commit 8851b5d into dotnet:main Mar 20, 2021

tarekgh deleted the FixTimeZoneRulesOnLinux branch March 20, 2021 04:29

tarekgh mentioned this pull request Mar 20, 2021

Incorrect trailing TimeZoneInfo.AdjustmentRule on Unix #20624

Closed

ghost locked as resolved and limited conversation to collaborators Apr 19, 2021

jeffhandley added area-System.DateTime and removed area-System.Runtime labels Nov 22, 2022

Conversation

tarekgh commented Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tarekgh commented Mar 17, 2021

Uh oh!

Uh oh!

lewing commented Mar 17, 2021

Uh oh!

lewing left a comment

Choose a reason for hiding this comment

Uh oh!

tarekgh commented Mar 17, 2021

Uh oh!

tarekgh commented Mar 18, 2021

Uh oh!

eerhardt commented Mar 18, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tarekgh commented Mar 18, 2021

Uh oh!

eerhardt commented Mar 18, 2021

Uh oh!

tarekgh commented Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerhardt commented Mar 19, 2021

Uh oh!

tarekgh commented Mar 19, 2021

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tarekgh commented Mar 17, 2021 •

edited

Loading

tarekgh commented Mar 19, 2021 •

edited

Loading