[Freeassociation-devel] libical: TZID prefix

[Patrick Ohly]

Hello!

Any comments on this? It's still broken in Trunk.

Bye, Patrick

On Di, 2011-08-16 at 13:03 +0200, Patrick Ohly wrote:
> 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.
> 
> ------------------------------------------------------------------------------
> uberSVN's rich system and user administration capabilities and model 
> configuration take the hassle out of deploying and managing Subversion and 
> the tools developers use with it. Learn more about uberSVN and get a free 
> download at:  http://p.sf.net/sfu/wandisco-dev2dev
> _______________________________________________
> Freeassociation-devel mailing list
> Freeassociation-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/freeassociation-devel