GNOME Bugzilla – Bug 686128
GTimeZone should be able to parse POSIX format for TZ environment variable
Last modified: 2012-12-19 13:51:35 UTC
GTimeZone should parse "std offset [dst [offset],start[/time],end[/time]]" format in TZ environment variable. The "tzn[+|–]hh[:mm[:ss]][dzn]" format used in Windows should be parsed too, but in a separate bug report (https://bugzilla.gnome.org/show_bug.cgi?id=683998).
Plans: - Accept POSIX format if operating system is Unix, POSIX and Windows formats if operating system is Windows. - Use "Pacific Standard Time" rules in Windows format with daylight. Both the older (pre-2007) and newer rules will be used (unless Dynamic DST is not supported by the operating system). This will be obtained from the registry. - Accept TZ variables in zoneinfo files too. - Do not parse zoneinfo files in file system if operating system is Windows. - Might map IANA names to equivalent Windows names of time zones. Abbreviations might be mapped too. - Other plans in bgo#683998.
John Ralls, do you agree on the first one? Or should I accept Windows format too in Unix? Or should it just support Unix format?
Created attachment 226435 [details] [review] Patch for parsing TZ Current version of the patch. I'll probably move Windows-specific parts to a different commit.
> John Ralls, do you agree on the first one? Or should I accept Windows format > too in Unix? Or should it just support Unix format? I don't know what you mean by "the first one". I agree with the sentiment of the description, but comment 1 is utterly confused. > - Other plans in bgo#683998. Write this as Bug 683998 so that Bugzilla turns it into a link. > Current version of the patch. I'll probably move Windows-specific parts to a different commit. This is way too much for a single commit. Break it up into pieces. A graphical git tool like gitx can help organize things, and git rebase -i lets you restructure a set of commits so that it makes sense. I've also decided that turning the timezone file into a byte-field and accessing it directly is an ugly hack and that making a fake-TZif structure, complete with bogus header, makes it worse. I've got something in progress to address that.
(In reply to comment #4) > > John Ralls, do you agree on the first one? Or should I accept Windows format > > too in Unix? Or should it just support Unix format? > > > I don't know what you mean by "the first one". I agree with the sentiment of > the description, but comment 1 is utterly confused. > "Accept POSIX format if operating system is Unix, POSIX and Windows formats if operating system is Windows." > > - Other plans in bgo#683998. > > Write this as Bug 683998 so that Bugzilla turns it into a link. > > > Current version of the patch. I'll probably move Windows-specific parts to a > different commit. > > This is way too much for a single commit. Break it up into pieces. A graphical > git tool like gitx can help organize things, and git rebase -i lets you > restructure a set of commits so that it makes sense. > I'll move them after my exams today. > I've also decided that turning the timezone file into a byte-field and > accessing it directly is an ugly hack and that making a fake-TZif structure, > complete with bogus header, makes it worse. I've got something in progress to > address that. Are you planning to do it yourself? Or should I change them myself?
fwiw, this is the best resource I've found on the topic: http://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html
Created attachment 226486 [details] [review] Adding more offset formats used by TZ
Created attachment 226487 [details] [review] New constructor from offset Should I accept offset to add to local time to get UTC, or offset to add to UTC to get local time? It currently uses the later.
Created attachment 226488 [details] [review] Parse POSIX TZ format I confused offsets of POSIX time zones from RFC3339/ISO 8601 timezones: TZ variable actually uses the same one as in Windows. Also I fixed the default daylight offset. Time are now interpreted as local time. It now matches with IANA values when I tried the 1987-2006 rule for "PST8PDT,M4.1.0,M10.5.0" as TZ environment variable and "America/Los_Angeles" (equivalent to PST, Pacific Standard Time) from IANA. I used 1990 results to check.
Please apply them in correct order. Windows patches in Bug 683998. New structures not implemented yet, still using fake tzif.
> Are you planning to do it yourself? Or should I change them myself? I've got a patch (actually 2) that I'm working on. I hope to have it done in a few hours; it's turning out to be more fiddly than I expected to get everything in the right place. BTW, I was mistaken about the position of the abbreviations: They're between the ttinfos and the leaps.
The patches changing to structs are on Bug 686184. I think you'll find your patches a bit cleaner if you base them on mine. I'll push them in a few days if there aren't any objections. In the meantime, I need to spend some time on other things.
Should I recreate the transition times too? Or do I have to change it so that I only need to store the date (month/week/weekday)?
Created attachment 226593 [details] [review] Adding more offset formats used by TZ
Created attachment 226594 [details] [review] New constructor from offset
Created attachment 226595 [details] [review] Parse POSIX TZ format These patches will now apply cleanly on latest patch of Bug 686184. It is now dependent on those patches. Used new structures. Can now handle multiple rules (see Windows patches on how to use). I tested them by comparing values generated by zic with the following rules file: ================================================================================ # Rule NAME FROM TO TYPE IN ON AT SAVE LETTER/S Rule US min max - Nov Sun>=1 2:00 0 S Rule US min max - Mar Sun>=8 2:00 1:00 D Rule NZ min max - Apr Sun>=1 2:00 0 S Rule NZ min max - Sep lastSun 2:00 1:00 D Rule PT min 2006 - Oct lastSun 2:00 0 S Rule PT min 2006 - Apr Sun>=1 2:00 1:00 D Rule PT 2007 max - Nov Sun>=1 2:00 0 S Rule PT 2007 max - Mar Sun>=8 2:00 1:00 D # Zone NAME GMTOFF RULES FORMAT [UNTIL] Zone America/Los_Angeles -8:00 US P%sT Zone Pacific/Auckland 12:00 NZ NZ%sT Zone PST8PDT -8:00 PT P%sT ================================================================================ Those values are the latest rules of America/Los_Angeles and Pacific/Auckland, though I changed the time of Pacific/Auckland to wall clock time (it is actually in standard time, both 2:00s). That's because TZ variables only supports wall clock time. Only one rule is supported by TZ. The PST8PDT is just for testing with Windows Dynamic DST. The PT rule is actually US. Only pre-2006 and post-2007 rules are supported by Windows. I used diff to check if there's any difference between TZ output and zic output, using the GTimeZone checker also attached in this bug. The outputs are completely the same for the two cities. All times and offsets are correct too in PST8PDT, only abbreviations are basically different (since abbreviations are not supported in Windows).
Created attachment 226597 [details] Get time zone information You may use this to compare transition times.
TZ values used for testing: Pacific/Auckland (New Zealand Standard Time): NZST-12NZDT,M10.1.0,M3.3.0 America/Los_Angeles (Pacific Standard Time): PST8PDT,M3.2.0,M11.1.0
Should I expose the rules structure then add a new constructor for that? I'll probably call it something like GTimeZoneRule. init_zone_from_rules() accepts an array of rules, the last rule is only used to get the maximum date, so it's usually (rules+1) in length.
Also, if I will expose it, do I have to change the data type of the members of the rules structure (currently named "tz_rule", might become "GTimeZoneRule") from all gint to some as GDateYear, GDateMonth, GDateDay and GDateWeekday? GDateTime (used to get the specific date for a certain nth weekday) uses gint for all values, while GDate (used to get the date for a Julian date) uses those data types/enums.
Created attachment 226874 [details] [review] Parse more offset formats
Created attachment 226875 [details] [review] Add constructor that uses offset
Created attachment 226876 [details] [review] Add constructor that uses rules
Created attachment 226877 [details] [review] Parse POSIX format for TZ
Created new structures GTimeZoneDate and GTimeZoneRule. New constructor using rules. Split of rules patch from POSIX-format patch. More documentation. Correct TZ variables for testing: Los_Angeles: PST8PDT,M3.2.0,M11.1.0 Auckland: NZST-12NZDT,M9.5.0,M4.1.0/3 Zic rules file: ================================================================================ # Rule NAME FROM TO TYPE IN ON AT SAVE LETTER/S Rule US min max - Nov Sun>=1 2:00 0 S Rule US min max - Mar Sun>=8 2:00 1:00 D Rule NZ min max - Apr Sun>=1 2:00s 0 S Rule NZ min max - Sep lastSun 2:00s 1:00 D # Zone NAME GMTOFF RULES FORMAT [UNTIL] Zone Los_Angeles -8:00 US P%sT Zone Auckland 12:00 NZ NZ%sT ================================================================================ Windows Time Zone in registry (Wine): ================================================================================ [Software\\Microsoft\\Windows NT\\CurrentVersion\\Time Zones\\Pacific Standard Time] 1347240835 "Display"="America/Los_Angeles" "Dlt"="Pacific Daylight Time" "Std"="Pacific Standard Time" "TZI"=hex:e0,01,00,00,00,00,00,00,c4,ff,ff,ff,00,00,0b,00,00,00,01,00,02,00,00,\ 00,00,00,00,00,00,00,03,00,00,00,02,00,02,00,00,00,00,00,00,00 [Software\\Microsoft\\Windows NT\\CurrentVersion\\Time Zones\\New Zealand Standard Time] 1347240835 "Display"="Pacific/Auckland" "Dlt"="New Zealand Daylight Time" "Std"="New Zealand Standard Time" "TZI"=hex:30,fd,ff,ff,00,00,00,00,c4,ff,ff,ff,00,00,04,00,00,00,01,00,03,00,00,\ 00,00,00,00,00,00,00,09,00,00,00,05,00,02,00,00,00,00,00,00,00 ================================================================================ Time intervals created from TZ environment variable (POSIX format), rules construction and dummy zoneinfo files in Linux, and time intervals created from TZ environment variable (POSIX and Microsoft/Windows format), registry and GetTimeZoneInformation in Windows matches.
Created attachment 226898 [details] [review] Add constructor that uses rules Added support for local standard time and UTC start and end dates for daylight savings time in rules.
Created attachment 226899 [details] [review] Parse POSIX format for TZ Related to previous.
(In reply to comment #19) > Should I expose the rules structure then add a new constructor for that? I'll > probably call it something like GTimeZoneRule. init_zone_from_rules() accepts > an array of rules, the last rule is only used to get the maximum date, so it's > usually (rules+1) in length. Sorry, I didn't really understand what you were asking here until you wrote it. No, I don't think that it's a good idea to have g_time_zone_new_for_offset() and g_time_zone_new_for_rules() be public functions, and I especially don't like exposing the structs, nor expecting users to stuff a struct and pass it in to a constructor. I think there should be no changes to the public API. POSIX-style strings should be passed in via identifier, and construction from rules should come from OS data, either files or in the case of MSWin, the Registry. BTW, the "typedef struct _Foo Foo;" pattern is used to *hide* the content of structs. The definition is usually in the .c file, though there are a couple of exceptions when it's needed by other internal implementations. A public struct should be defined like GBSearchConfig in gbsearcharray.h.
(In reply to comment #28) > (In reply to comment #19) > > Should I expose the rules structure then add a new constructor for that? I'll > > probably call it something like GTimeZoneRule. init_zone_from_rules() accepts > > an array of rules, the last rule is only used to get the maximum date, so it's > > usually (rules+1) in length. > > Sorry, I didn't really understand what you were asking here until you wrote it. > > No, I don't think that it's a good idea to have g_time_zone_new_for_offset() > and g_time_zone_new_for_rules() be public functions, and I especially don't > like exposing the structs, nor expecting users to stuff a struct and pass it in > to a constructor. Okay, I'll remove them then. Should I keep them in separate patches though? > > I think there should be no changes to the public API. POSIX-style strings > should be passed in via identifier, and construction from rules should come > from OS data, either files or in the case of MSWin, the Registry. > > BTW, the "typedef struct _Foo Foo;" pattern is used to *hide* the content of > structs. The definition is usually in the .c file, though there are a couple of > exceptions when it's needed by other internal implementations. A public struct > should be defined like GBSearchConfig in gbsearcharray.h. I actually can't find a good example in GLib, there are lots of structs. Thanks for the information, that might help in the future.
Created attachment 226923 [details] [review] Add initialization functions for rules
Created attachment 226924 [details] [review] Parse POSIX format for TZ
This code triggers fatal-by-default warnings with gcc/Linux: http://ostree.gnome.org/work/tasks/gnomeos-3.8/glib/i686/2012.161/log If you want to do byte arithmetic, use pointers of type "guint8*"
../../glib/gtimezone.c: In function 'init_zone_from_iana_info': ../../glib/gtimezone.c:477:34: error: pointer of type 'void *' used in arithmetic [-Werror=pointer-arith] ../../glib/gtimezone.c:478:29: error: pointer of type 'void *' used in arithmetic [-Werror=pointer-arith] ../../glib/gtimezone.c:479:24: error: pointer of type 'void *' used in arithmetic [-Werror=pointer-arith] ../../glib/gtimezone.c:481:23: error: pointer of type 'void *' used in arithmetic [-Werror=pointer-arith] ../../glib/gtimezone.c:482:23: error: pointer of type 'void *' used in arithmetic [-Werror=pointer-arith]
Fixed, thanks.
no, you haven't fixed it at all.
Created attachment 231869 [details] [review] timezone: Fix byte arithmetic Use guint8* instead of gpointer, to avoid warnings and side effects.
Review of attachment 231869 [details] [review]: One minor style comment, otherwise looks good. ::: glib/gtimezone.c @@ +474,3 @@ g_assert (type_count == isstd_count); + tz_transitions = (guint8 *) (header + 1); Personally I would write this as: tz_transitions = (((guint8*)header) + sizeof (*header)) Otherwise it's really easy to get confused between byte increments and sizeof-structure increments.
pushed with the change proposed by Colin. Attachment 231869 [details] pushed as 5f1f9cb - timezone: Fix byte arithmetic