[Freeassociation-devel] libical: TZID prefix

[Patrick Ohly]

On Fri, 2011-08-12 at 20:23 -0400, Allen Winter wrote:
> On Friday 12 August 2011 2:49:42 AM Patrick Ohly wrote:
> > On Do, 2011-08-11 at 19:21 -0400, Allen Winter wrote:
> > > On Thursday 11 August 2011 5:52:27 AM Patrick Ohly wrote:
> > > > Hello Allen!
> > > > 
> > > > I'm having some issue with the libical compiled from source which might
> > > > be related to your following commit:
> > > > 
> > > Ok, reverted in SVN commit r1083
> > > 
> > > Please try now and let me know if things work better.
> > 
> > I can't test that particular commit because the nightly test machine
> > went down and I cannot reset it remotely myself. But I had already
> > tested with a version where I reverted the commit myself. I can confirm
> > that this fixes the problem for me.
> > 
> > > regression test fails now btw.  I'll try to fix that once Patrick reports back.
> > 
> > Please continue.
> > 
> 
> I think I fixed the regression test.
> There was a bug in icaltimezone_get_builtin_timezone_from_tzid()
> that didn't skip past the "/Tzfile" when extracting the location from the TZID.

Your fix is:

commit a65a0894ce7cf9672f9837dd38a95bbcba84b49b
Author: awinterz <awinterz at b29e25b1-4845-0410-9c6e-9608a45a5811>
Date:   Sat Aug 13 00:11:14 2011 +0000

    in icaltimezone_get_builtin_timezone_from_tzid(), skip past the 4th '/' character
    to find the location from the tzid, not the 3rd '/' character.  This is needed
    because there has been a "/Tzfile" encoded into the tzid as of late.
    
    fixes the 'test_convenience' regression test.

But that is not correct for the TZID as produced by the latest libical.
In the test program from my initial email, I get
TZID /freeassociation.sourceforge.net/Tzfile/Europe/Berlin. With the
commit above, icaltimezone_get_builtin_timezone_from_tzid() skips
forward to the "Berlin" part, which then fails to be found.

I fail to see how the commit relates to the 'test_convenience'. Isn't
that the one which uses
icaltimezone_get_builtin_timezone("Europe/Rome")? Is
icaltimezone_get_builtin_timezone_from_tzid() used indirectly in that
test?

[talking to myself while looking at the source...]

Yes, that's it. The tests call
icaltimezone_get_builtin_timezone_from_tzid() like this:

#0  icaltimezone_get_builtin_timezone_from_tzid (
    tzid=0x62d1f0 "/softwarestudio.org/Olson_20010626_2/Tzfile/Europe/Rome")
    at /home/nightly/freeassociation/libical/src/libical/icaltimezone.c:1468
#1  0x00007ffff777c3c8 in icalcomponent_get_datetime (comp=<value optimized out>, prop=<value optimized out>)
    at /home/nightly/freeassociation/libical/src/libical/icalcomponent.c:1587
#2  0x00007ffff777c41c in icalcomponent_get_dtstart (comp=0x6288f0)
    at /home/nightly/freeassociation/libical/src/libical/icalcomponent.c:1615
#3  0x0000000000407d79 in test_convenience () at /home/nightly/freeassociation/libical/src/test/regression.c:2282
#4  0x0000000000416f5a in test_run (test_name=<value optimized out>, test_fcn=0x406c40 <test_convenience>, 
    do_test=0, headeronly=<value optimized out>)
    at /home/nightly/freeassociation/libical/src/test/regression-utils.c:170
#5  0x0000000000406b37 in main (argc=<value optimized out>, argv=0x7fffffffdff8)
    at /home/nightly/freeassociation/libical/src/test/regression.c:3813

The problem is that
"/softwarestudio.org/Olson_20010626_2/Tzfile/Europe/Rome" is not an
internal TZID for which icaltimezone_get_builtin_timezone_from_tzid() is
meant to work - at least when internal timezones are taken from the zone
database.

Allen, were you perhaps testing on a system where internal TZIDs are not
of the form /freeassociation.sourceforge.net/Tzfile/Europe/Berlin? How
did you make strcmp("1997-08-01 12:00:00 Europe/Rome", ictt_as_string())
pass in test_convenience? In my understanding of the source it'll always
insert the TZID, which is not Europe/Rome either way.

I'm not sure what the right solution is:
     1. Use three slashes consistently, including all tests?
     2. Use four slashes consistently, including icaltz-util.c?
     3. #ifdefs?

Attached a patch which implements solution 1. I chose to go that route
because the format /<source>/<version>/<location>/ seems to be the
established one, with "Tzfile" taking the place of <version> when
parsing zone info. Adding one more slash would introduce an unnecessary
change compared to previous libical releases, too.

-- 
Bye, Patrick Ohly
--  
Patrick.Ohly at gmx.de
http://www.estamos.de/

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Revert-in-icaltimezone_get_builtin_timezone_from_tzi.patch
Type: text/x-patch
Size: 1384 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/libical-devel/attachments/20110816/c8249927/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-regression-fixed-test_convenience.patch
Type: text/x-patch
Size: 1905 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/libical-devel/attachments/20110816/c8249927/attachment-0001.bin>