Skip to content

Add more support for Julian time zones POSIX rules #50131

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.

Sign up for GitHub

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
tarekgh merged 5 commits into from
Mar 27, 2021
Merged

Add more support for Julian time zones POSIX rules #50131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0c7707a
Add more support for Julian time zones Posix rules
tarekgh Mar 23, 2021
801fa1d
Address the feedback
tarekgh Mar 24, 2021
cab2247
Feedback
tarekgh Mar 24, 2021
7eb0f84
Add test
tarekgh Mar 26, 2021
716b079
ensure the file deletion in the test
tarekgh Mar 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

@danmoseley danmoseley Mar 27, 2021

Choose a reason for hiding this comment

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

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...

Copy link
Member

@safern safern Mar 27, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

@danmoseley danmoseley Mar 27, 2021

Choose a reason for hiding this comment

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

Oh..nuts☺️

// This specifies the Julian day, with n between 0 and 365. February 29 is counted in leap years.
//
// n would be a relative number from the beginning of the year. which should handle if the
Expand All @@ -1313,11 +1312,30 @@ private static DateTime ParseTimeOfDay(ReadOnlySpan<char> time)
// 0 30 31 58 59 89 334 364
// |-------Jan--------|-------Feb--------|-------Mar--------|....|-------Dec--------|
//
//
// For example if n is specified as 60, this means in leap year the rule will start at Mar 1,
// while in non leap year the rule will start at Mar 2.
//
// If we need to support n format, we'll have to have a floating adjustment rule support this case.
// This n Julian day format is very uncommon and mostly used for convenience to specify dates like January 1st
// which we can support without any major modification to the Adjustment rules. We'll support this rule for day
// 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)
Copy link
Member

@danmoseley danmoseley Mar 27, 2021

Choose a reason for hiding this comment

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

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

Copy link
Member

@safern safern Mar 27, 2021

Choose a reason for hiding this comment

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

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'.

Copy link
Member

@danmoseley danmoseley Mar 27, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@mattjohnsonpint mattjohnsonpint Mar 27, 2021

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@tarekgh tarekgh Mar 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

{
int d, m;
if (julianDay <= 30) // January
{
m = 1;
d = julianDay + 1;
}
else // February
{
m = 2;
d = julianDay - 30;
}

return TransitionTime.CreateFixedDateRule(ParseTimeOfDay(time), m, d);
}

// Since we can't support this rule, return null to indicate to skip the POSIX rule.
return null;
Expand All @@ -1330,7 +1348,7 @@ private static DateTime ParseTimeOfDay(ReadOnlySpan<char> time)
}

/// <summary>
/// Parses a string like Jn or n into month and day values.
/// Parses a string like Jn into month and day values.
/// </summary>
private static void TZif_ParseJulianDay(ReadOnlySpan<char> date, out int month, out int day)
{
Expand Down
74 changes: 74 additions & 0 deletions src/libraries/System.Runtime/tests/System/TimeZoneInfoTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Globalization;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Runtime.Serialization.Formatters.Binary;
Expand Down Expand Up @@ -2353,6 +2354,79 @@ public static void GetSystemTimeZones_AllTimeZonesHaveOffsetInValidRange()
}
}

private static byte [] timeZoneFileContents = new byte[]
{
0x54, 0x5A, 0x69, 0x66, 0x32, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x54, 0x5A, 0x69, 0x66,
0x32, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x05, 0x00, 0x00, 0x00, 0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
0x00, 0x00, 0x00, 0x05, 0x00, 0x00, 0x00, 0x0C, 0xF8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0xFF, 0xFF, 0xF8, 0xE4, 0x00, 0x00, 0x00, 0x00, 0x0E, 0x10, 0x01, 0x04, 0x00, 0x00, 0x00,
0x00, 0x00, 0x08, 0x00, 0x00, 0x0E, 0x10, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x08, 0x4C,
0x4D, 0x54, 0x00, 0x2B, 0x30, 0x31, 0x00, 0x2B, 0x30, 0x30, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00,
// POSIX Rule
// 0x0A, 0x3C, 0x2B, 0x30, 0x30, 0x3E, 0x30, 0x3C, 0x2B, 0x30, 0x31,
// 0x3E, 0x2C, 0x30, 0x2F, 0x30, 0x2C, 0x4A, 0x33, 0x36, 0x35, 0x2F, 0x32, 0x35, 0x0A
};

[ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
[PlatformSpecific(TestPlatforms.AnyUnix)]
[InlineData("<+00>0<+01>,0/0,J365/25", 1, 1, true)]
[InlineData("<+00>0<+01>,30/0,J365/25", 31, 1, true)]
[InlineData("<+00>0<+01>,31/0,J365/25", 1, 2, true)]
[InlineData("<+00>0<+01>,58/0,J365/25", 28, 2, true)]
[InlineData("<+00>0<+01>,59/0,J365/25", 0, 0, false)]
[InlineData("<+00>0<+01>,9999999/0,J365/25", 0, 0, false)]
[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";
Copy link
Member

@danmoseley danmoseley Mar 27, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@tarekgh tarekgh Mar 28, 2021

Choose a reason for hiding this comment

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

that is a good idea.

using (FileStream fs = new FileStream(zoneFilePath, FileMode.Create))
{
fs.Write(timeZoneFileContents.AsSpan());

// Append the POSIX rule
fs.WriteByte(0x0A);
foreach (char c in posixRule)
{
fs.WriteByte((byte) c);
}
fs.WriteByte(0x0A);
}

try
{
ProcessStartInfo psi = new ProcessStartInfo() { UseShellExecute = false };
psi.Environment.Add("TZ", zoneFilePath);

RemoteExecutor.Invoke((day, month, succeed) =>
{
bool expectedToSucceed = bool.Parse(succeed);
int d = int.Parse(day);
int m = int.Parse(month);

TimeZoneInfo.AdjustmentRule [] rules = TimeZoneInfo.Local.GetAdjustmentRules();

if (expectedToSucceed)
{
Assert.Equal(1, rules.Length);
Assert.Equal(d, rules[0].DaylightTransitionStart.Day);
Assert.Equal(m, rules[0].DaylightTransitionStart.Month);
}
else
{
Assert.Equal(0, rules.Length);
}
}, dayNumber.ToString(), monthNumber.ToString(), shouldSucceed.ToString(), new RemoteInvokeOptions { StartInfo = psi}).Dispose();
}
finally
{
try { File.Delete(zoneFilePath); } catch { } // don't fail the test if we couldn't delete the file.
}
}

[Fact]
public static void TimeZoneInfo_DaylightDeltaIsNoMoreThan12Hours()
{
Expand Down