After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 686128 - GTimeZone should be able to parse POSIX format for TZ environment variable
GTimeZone should be able to parse POSIX format for TZ environment variable
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
2.34.x
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2012-10-15 00:51 UTC by Arnel Borja
Modified: 2012-12-19 13:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for parsing TZ (20.41 KB, patch)
2012-10-15 01:03 UTC, Arnel Borja
none Details | Review
Adding more offset formats used by TZ (2.44 KB, patch)
2012-10-15 16:47 UTC, Arnel Borja
none Details | Review
New constructor from offset (2.41 KB, patch)
2012-10-15 16:48 UTC, Arnel Borja
none Details | Review
Parse POSIX TZ format (17.68 KB, patch)
2012-10-15 16:56 UTC, Arnel Borja
none Details | Review
Adding more offset formats used by TZ (2.42 KB, patch)
2012-10-16 22:54 UTC, Arnel Borja
none Details | Review
New constructor from offset (2.47 KB, patch)
2012-10-16 22:55 UTC, Arnel Borja
none Details | Review
Parse POSIX TZ format (16.01 KB, patch)
2012-10-16 23:06 UTC, Arnel Borja
none Details | Review
Get time zone information (5.02 KB, text/x-csrc)
2012-10-16 23:23 UTC, Arnel Borja
  Details
Parse more offset formats (2.47 KB, patch)
2012-10-20 09:47 UTC, Arnel Borja
committed Details | Review
Add constructor that uses offset (2.52 KB, patch)
2012-10-20 09:48 UTC, Arnel Borja
none Details | Review
Add constructor that uses rules (14.00 KB, patch)
2012-10-20 09:49 UTC, Arnel Borja
none Details | Review
Parse POSIX format for TZ (10.38 KB, patch)
2012-10-20 09:50 UTC, Arnel Borja
none Details | Review
Add constructor that uses rules (14.74 KB, patch)
2012-10-20 17:03 UTC, Arnel Borja
none Details | Review
Parse POSIX format for TZ (10.44 KB, patch)
2012-10-20 17:08 UTC, Arnel Borja
none Details | Review
Add initialization functions for rules (9.11 KB, patch)
2012-10-21 13:33 UTC, Arnel Borja
committed Details | Review
Parse POSIX format for TZ (10.60 KB, patch)
2012-10-21 13:35 UTC, Arnel Borja
committed Details | Review
timezone: Fix byte arithmetic (2.79 KB, patch)
2012-12-19 11:04 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review

Description Arnel Borja 2012-10-15 00:51:57 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).
Comment 1 Arnel Borja 2012-10-15 00:58:08 UTC
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.
Comment 2 Arnel Borja 2012-10-15 01:02:06 UTC
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?
Comment 3 Arnel Borja 2012-10-15 01:03:37 UTC
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.
Comment 4 John Ralls 2012-10-15 01:33:54 UTC
> 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.
Comment 5 Arnel Borja 2012-10-15 01:59:26 UTC
(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?
Comment 6 Allison Karlitskaya (desrt) 2012-10-15 02:26:42 UTC
fwiw, this is the best resource I've found on the topic:

http://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html
Comment 7 Arnel Borja 2012-10-15 16:47:13 UTC
Created attachment 226486 [details] [review]
Adding more offset formats used by TZ
Comment 8 Arnel Borja 2012-10-15 16:48:56 UTC
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.
Comment 9 Arnel Borja 2012-10-15 16:56:50 UTC
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.
Comment 10 Arnel Borja 2012-10-15 17:08:43 UTC
Please apply them in correct order. Windows patches in Bug 683998. New structures not implemented yet, still using fake tzif.
Comment 11 John Ralls 2012-10-15 17:45:46 UTC
> 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.
Comment 12 John Ralls 2012-10-15 21:52:53 UTC
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.
Comment 13 Arnel Borja 2012-10-16 04:27:58 UTC
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)?
Comment 14 Arnel Borja 2012-10-16 22:54:34 UTC
Created attachment 226593 [details] [review]
Adding more offset formats used by TZ
Comment 15 Arnel Borja 2012-10-16 22:55:46 UTC
Created attachment 226594 [details] [review]
New constructor from offset
Comment 16 Arnel Borja 2012-10-16 23:06:45 UTC
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).
Comment 17 Arnel Borja 2012-10-16 23:23:57 UTC
Created attachment 226597 [details]
Get time zone information

You may use this to compare transition times.
Comment 18 Arnel Borja 2012-10-16 23:55:58 UTC
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
Comment 19 Arnel Borja 2012-10-17 14:41:10 UTC
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.
Comment 20 Arnel Borja 2012-10-17 14:48:34 UTC
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.
Comment 21 Arnel Borja 2012-10-20 09:47:22 UTC
Created attachment 226874 [details] [review]
Parse more offset formats
Comment 22 Arnel Borja 2012-10-20 09:48:17 UTC
Created attachment 226875 [details] [review]
Add constructor that uses offset
Comment 23 Arnel Borja 2012-10-20 09:49:18 UTC
Created attachment 226876 [details] [review]
Add constructor that uses rules
Comment 24 Arnel Borja 2012-10-20 09:50:11 UTC
Created attachment 226877 [details] [review]
Parse POSIX format for TZ
Comment 25 Arnel Borja 2012-10-20 10:02:26 UTC
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.
Comment 26 Arnel Borja 2012-10-20 17:03:38 UTC
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.
Comment 27 Arnel Borja 2012-10-20 17:08:53 UTC
Created attachment 226899 [details] [review]
Parse POSIX format for TZ

Related to previous.
Comment 28 John Ralls 2012-10-20 20:41:26 UTC
(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.
Comment 29 Arnel Borja 2012-10-21 01:38:20 UTC
(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.
Comment 30 Arnel Borja 2012-10-21 13:33:09 UTC
Created attachment 226923 [details] [review]
Add initialization functions for rules
Comment 31 Arnel Borja 2012-10-21 13:35:28 UTC
Created attachment 226924 [details] [review]
Parse POSIX format for TZ
Comment 32 Colin Walters 2012-12-19 00:29:30 UTC
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*"
Comment 33 Colin Walters 2012-12-19 00:29:43 UTC
../../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]
Comment 34 John Ralls 2012-12-19 01:21:57 UTC
Fixed, thanks.
Comment 35 Emmanuele Bassi (:ebassi) 2012-12-19 10:03:57 UTC
no, you haven't fixed it at all.
Comment 36 Emmanuele Bassi (:ebassi) 2012-12-19 11:04:52 UTC
Created attachment 231869 [details] [review]
timezone: Fix byte arithmetic

Use guint8* instead of gpointer, to avoid warnings and side effects.
Comment 37 Colin Walters 2012-12-19 13:41:47 UTC
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.
Comment 38 Emmanuele Bassi (:ebassi) 2012-12-19 13:51:24 UTC
pushed with the change proposed by Colin.

Attachment 231869 [details] pushed as 5f1f9cb - timezone: Fix byte arithmetic