Add more support for Julian time zones POSIX rules #50131

tarekgh · 2021-03-23T21:25:55Z

On Linux the time zone files can include the rules posix format https://man7.org/linux/man-pages/man3/tzset.3.html. We already parse such format successfully except the Julian n day format. After investigating different Linux distro zone files for all zones, I found only rhel based distros (e.g. RHEL, Centos, and Amazon Linux) are using this Julian n rules for the following zones:

Africa\Casablanca: <+00>0<+01>,0/0,J365/25
Africa\El_Aaiun:   <+00>0<+01>,0/0,J365/25

Also, it is used only for convenience to specify January 1st date. From the experience with the time zone, I cannot think in any case that need to use this rule to specify dates after February 28. Considering that, we can handle this rule if the specified date in the rule would be from Jan 1 to Feb 28.

So, this change is to allow parsing this rule and we'll only fail in case if getting dates past Feb 28 which is very uncommon and very unlikely be used in the future.

#25933

dotnet-issue-labeler · 2021-03-23T21:25:59Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

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

eerhardt · 2021-03-24T14:17:23Z

    /// Parses a string like Jn or n into month and day values.

(nit) can you update this comment while you are in here? This only supports Jn strings, not n like it indicates. The n support is being handled in your new code above.

Refers to: src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs:1363 in 0c7707a. [](commit_id = 0c7707a, deletion_comment = False)

tarekgh · 2021-03-24T18:15:52Z

can you update this comment while you are in here?

Yes, I did it. Thanks for catching that.

@stephentoub @eerhardt please let me know if you have any more comments or we good to go. Thanks.

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

eerhardt

Thanks @tarekgh. The code changes look good to me.

tarekgh · 2021-03-24T19:43:59Z

Thanks @tarekgh. The code changes look good to me.

Thanks for your review and feedback. it was helpful.

I'll not merge this before try adding some test per your suggestion.

eerhardt

Looks great. Thanks for adding the tests.

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

danmoseley · 2021-03-27T06:21:29Z

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

@@ -1296,8 +1296,7 @@ private static DateTime ParseTimeOfDay(ReadOnlySpan<char> time)
            {
                if (date[0] != 'J')
                {
-                    // should be n Julian day format which we don't support.
-                    //
+                    // should be n Julian day format.


The comment below says "February 29 is counted in leap years". but the man page for tzset says:

Jn This specifies the Julian day with n between 1 and 365. Leap days are not counted. In this format, February 29 can't be represented; February 28 is day 59, and March 1 is always day 60.

Am I looking at the wrong thing?

If leap years can't be counted, that would explain why it's not very useful past Feb 28...

I think you got confused, this is handling the n format not the Jn. The n format goes from 0-365 and counts leap years.

See my comment below, but the Jn format is handled after this if.

Oh..nuts☺️

danmoseley · 2021-03-27T06:32:04Z

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

+                    // numbers less than 59 (up to Feb 28). Otherwise we'll skip this POSIX rule.
+                    // We've never encountered any time zone file using this format for days beyond Feb 28.
+
+                    if (int.TryParse(date, out int julianDay) && julianDay < 59)


Per the tzset man page pasted above, the value seems to start at 1, not 0..?

I may just be confused though, as I don't think I understand why the J format exists, unless it's a way to express a day that ignores a leap day (which is the way I read the man page). Eg., your example Africa\Casablanca: <+00>0<+01>,0/0,J365/25 means from the first day of the year to the last day of the year, whether it's a leap year or not. Also, this example would not be handled by this code as 365 is larger than 58 anyway?

cc @tarekgh

I believe the Jn format was already supported, this section is adding the n format. Which based on the man page is:

n This specifies the zero-based Julian day with n between 0 and 365. February 29 is counted in leap years.

Which is a regular int number. The Jn format which you are referring to is handled after this if:

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

Line 1502 in 8e690a2

// Julian day

Which is when date[0] == 'J'.

What about the question about only supporting up to Feb 28, when the example given is 365 ? I may well be still confused haha

In this bizarre counting system, Feb 29 doesn't exist. Dec 31st is always day 365, even in a leap year.

@danmoseley we are fully support Jn rule and partially support n rule. Jn rule is not differentiating between leap year and non-leap years. So, Feb 29th cannot be expressed with the Jn rule. i.e. if using J60 it is always March 1st and never be Feb 29th.

In the other hand, n rule account for leap years. note as mentioned before this rule is zero based rule. if specify 59 it means March 1st when the year is not leap year and will mean Feb 29th in the leap years. anyway, I never encountered any time zone file use this rule to specify a day after Feb 28th.

Let me know if there is anything unclear here.

danmoseley · 2021-03-27T06:36:34Z

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

+        [InlineData("<+00>0<+01>,A/0,J365/25", 0, 0, false)]
+        public static void NJulianRuleTest(string posixRule, int dayNumber, int monthNumber, bool shouldSucceed)
+        {
+            string zoneFilePath = Path.GetTempPath() + "dotnet_tz";


Tiny nit, using Path.GetRandomFileName() or some other source of randomness (as FileCleanupTestBase.GenerateTestFileName(..) does for example) would allow this test to be run concurrently with itself, eg., if someone runs debug and release tests together.

that is a good idea.

Add more support for Julian time zones Posix rules

0c7707a

tarekgh requested review from eerhardt and safern March 23, 2021 21:26

tarekgh added this to the 6.0.0 milestone Mar 23, 2021

tarekgh self-assigned this Mar 23, 2021

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

stephentoub approved these changes Mar 24, 2021

View reviewed changes

This was referenced Mar 24, 2021

OSX deprovision jaredpar/runfo#41

Closed

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

Closed

Address the feedback

801fa1d

safern reviewed Mar 24, 2021

View reviewed changes

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

safern approved these changes Mar 24, 2021

View reviewed changes

eerhardt reviewed Mar 24, 2021

View reviewed changes

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

Feedback

cab2247

eerhardt approved these changes Mar 24, 2021

View reviewed changes

tarekgh mentioned this pull request Mar 25, 2021

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

Closed

4 tasks

Add test

7eb0f84

eerhardt approved these changes Mar 26, 2021

View reviewed changes

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

ensure the file deletion in the test

716b079

tarekgh merged commit bb25166 into dotnet:main Mar 27, 2021

tarekgh deleted the FixJulianTZPosixRuleSupport branch March 27, 2021 01:51

tarekgh mentioned this pull request Mar 27, 2021

Support time zone 'n' Julian day POSIX format #25933

Closed

danmoseley reviewed Mar 27, 2021

View reviewed changes

tarekgh mentioned this pull request Mar 28, 2021

Use random file names in the TZ tests #50339

Merged

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more support for Julian time zones POSIX rules #50131

Add more support for Julian time zones POSIX rules #50131

tarekgh commented Mar 23, 2021 •

edited

Loading

dotnet-issue-labeler bot commented Mar 23, 2021

eerhardt commented Mar 24, 2021

tarekgh commented Mar 24, 2021

eerhardt left a comment

tarekgh commented Mar 24, 2021

eerhardt left a comment

danmoseley Mar 27, 2021

safern Mar 27, 2021

danmoseley Mar 27, 2021

danmoseley Mar 27, 2021

safern Mar 27, 2021

danmoseley Mar 27, 2021

mattjohnsonpint Mar 27, 2021

tarekgh Mar 28, 2021 •

edited

Loading

danmoseley Mar 27, 2021

tarekgh Mar 28, 2021

Add more support for Julian time zones POSIX rules #50131

Add more support for Julian time zones POSIX rules #50131

Conversation

tarekgh commented Mar 23, 2021 • edited Loading

dotnet-issue-labeler bot commented Mar 23, 2021

eerhardt commented Mar 24, 2021

tarekgh commented Mar 24, 2021

eerhardt left a comment

Choose a reason for hiding this comment

tarekgh commented Mar 24, 2021

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh Mar 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh commented Mar 23, 2021 •

edited

Loading

tarekgh Mar 28, 2021 •

edited

Loading