Last Comment Bug 410931 - Update internal timezone definitions
: Update internal timezone definitions
Status: VERIFIED FIXED
:
Product: Calendar
Classification: Client Software
Component: Internal Components (show other bugs)
: unspecified
: All All
: -- normal (vote)
: 0.8
Assigned To: Gary Kwong [:gkw] [:nth10sd]
:
Mentors:
: 419793 423304 (view as bug list)
Depends on: 429521
Blocks: 417541
  Show dependency treegraph
 
Reported: 2008-01-05 08:22 PST by Stefan Sitter
Modified: 2008-08-28 02:39 PDT (History)
7 users (show)
bugzilla: blocking‑calendar0.8+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Change Caracas' timezone from -0400 to -0430 (689 bytes, patch)
2008-01-11 11:15 PST, Gary Kwong [:gkw] [:nth10sd]
no flags Details | Diff | Review
better, more complete patch (703.20 KB, patch)
2008-01-11 13:14 PST, Gary Kwong [:gkw] [:nth10sd]
no flags Details | Diff | Review
version 2 incorporating Jan Mayen changes (704.79 KB, patch)
2008-01-16 11:17 PST, Gary Kwong [:gkw] [:nth10sd]
no flags Details | Diff | Review
patch version 3 (709.50 KB, patch)
2008-01-25 14:41 PST, Gary Kwong [:gkw] [:nth10sd]
no flags Details | Diff | Review
storage TZID update piece (5.72 KB, patch)
2008-01-26 17:05 PST, Daniel Boelzle [:dbo]
cmtalbert: review-
Details | Diff | Review
patch v3.1 (708.06 KB, patch)
2008-01-28 12:30 PST, Gary Kwong [:gkw] [:nth10sd]
cmtalbert: review+
Details | Diff | Review
New tzdata.c file with proper specification on Jerusalem (363.47 KB, patch)
2008-02-08 00:16 PST, cmtalbert
dbo.moz: review+
Details | Diff | Review
Updated ICS file for Jerusalem (2.12 KB, patch)
2008-02-08 00:16 PST, cmtalbert
dbo.moz: review+
Details | Diff | Review
Tests for the upgrade (3.86 KB, application/octet-stream)
2008-02-08 00:21 PST, cmtalbert
no flags Details
storage TZID update piece (11.58 KB, patch)
2008-02-11 10:19 PST, Daniel Boelzle [:dbo]
cmtalbert: review+
Details | Diff | Review
Patch v5 for tzdata.c with Australian DST exceptions (369.18 KB, patch)
2008-02-16 08:57 PST, :Hb
cmtalbert: review+
Details | Diff | Review
Fix v1 (162.02 KB, patch)
2008-02-19 07:57 PST, Gary Kwong [:gkw] [:nth10sd]
no flags Details | Diff | Review
Fix v2 (161.87 KB, patch)
2008-02-19 08:49 PST, Gary Kwong [:gkw] [:nth10sd]
philipp: review+
Details | Diff | Review

Description Stefan Sitter 2008-01-05 08:22:54 PST
Update internal timezone definitions

There were some recent timezone changes e.g. in South America. For the 0.8 release we should include updated timezone information. As of today the current Olson release if 2007k. 

This could be done together with the already planed timezone backend changes.
Comment 2 Gary Kwong [:gkw] [:nth10sd] 2008-01-11 11:15:25 PST
Created attachment 296569 [details] [diff] [review]
Change Caracas' timezone from -0400 to -0430

My first patch for Calendar!

Changed from -0400 to -0430. I have no idea what else to change.
Comment 3 Gary Kwong [:gkw] [:nth10sd] 2008-01-11 13:14:51 PST
Created attachment 296590 [details] [diff] [review]
better, more complete patch 

A much better, more comprehensive patch.

Incorporating all changes from Olson database 2007k dated 31 Dec 2007.

Specifically, new timezone info for America/Marigot, America/Resolute, America/St_Barthelemy and America/Indiana/Tell_City over the previous data dated Jan 2007.
Comment 4 Daniel Boelzle [:dbo] 2008-01-16 08:43:17 PST
Having a quick look I think "Atlantic/Jan_Mayen" has been purged out. we shouldn't do that since we don't know if someone has referenced it in his storage calendar.
Comment 5 Gary Kwong [:gkw] [:nth10sd] 2008-01-16 11:17:00 PST
Created attachment 297384 [details] [diff] [review]
version 2 incorporating Jan Mayen changes

many thanks to ctalbert and ssitter in IRC for this one, and dbo for pointing out Jan Mayen.
Comment 6 Daniel Boelzle [:dbo] 2008-01-25 03:00:48 PST
Comment on attachment 297384 [details] [diff] [review]
version 2 incorporating Jan Mayen changes

>Index: base/src/calTimezoneService.cpp
>         if (tzid.EqualsLiteral("/mozilla.org/20050126_1/Africa/Asmera")) {
>             _retval.AssignLiteral("/mozilla.org/20070129_1/Africa/Asmara");
>         } else if (tzid.EqualsLiteral("/mozilla.org/20050126_1/Africa/Timbuktu")) {
>             _retval.AssignLiteral("/mozilla.org/20070129_1/Africa/Bamako");
>         }
>     } else if (continent.EqualsLiteral("Atlantic")) {
>         if (tzid.EqualsLiteral("/mozilla.org/20050126_1/Atlantic/Faeroe")) {
>             _retval.AssignLiteral("/mozilla.org/20070129_1/Atlantic/Faroe");
>+        } else if (tzid.EqualsLiteral("/mozilla.org/20070129_1/Atlantic/Jan_Mayen")) {
>+            _retval.AssignLiteral("/mozilla.org/20071231_1/Europe/Oslo");
>         }
>     } else if (continent.EqualsLiteral("America")) {
>         if (tzid.EqualsLiteral("/mozilla.org/20050126_1/America/Argentina/ComodRivadavia")) {
>             _retval.AssignLiteral("/mozilla.org/20070129_1/America/Argentina/Catamarca");
>         } else if (tzid.EqualsLiteral("/mozilla.org/20050126_1/America/Louisville")) {
>             _retval.AssignLiteral("/mozilla.org/20070129_1/America/Kentucky/Louisville");
>         }
>     } else if (continent.EqualsLiteral("Europe")) {

Gary, w.r.t. the above code: IMO you need to adopt all aliases to 20071231_1, not only Europe/Oslo.
Moreover, please keep in mind that storage calendar won't upgrade its TZIDs to 20071231_1, see <http://mxr.mozilla.org/mozilla1.8/source/calendar/providers/storage/calStorageCalendar.js#1169>. However, this will still work since the timezone service can handle those TZIDs.
Comment 7 Gary Kwong [:gkw] [:nth10sd] 2008-01-25 14:41:02 PST
Created attachment 299301 [details] [diff] [review]
patch version 3

a 3rd version changing all 20070129_1 entries in calTimezoneService.cpp to 20071231_1.

Also changes the entries in calStorageCalendar.js (a comment) and a unit test case in test_datetime.js since these are the only 2 other files with 20070129_1 that were not touched on previously.
Comment 8 Daniel Boelzle [:dbo] 2008-01-26 08:43:58 PST
Gary, think of the case that people may already have upgraded their storage.sdb to the latest schema version. That way calStorageCalendar won't run into the migration code again (and won't update the TZIDs).
Comment 9 Daniel Boelzle [:dbo] 2008-01-26 17:05:24 PST
Created attachment 299482 [details] [diff] [review]
storage TZID update piece

More testing welcome!
Comment 10 Gary Kwong [:gkw] [:nth10sd] 2008-01-28 12:30:14 PST
Created attachment 299822 [details] [diff] [review]
patch v3.1

removed my previous comment changes in calStorageCalendar.js in this patch as Daniel has provided a better one.
Comment 11 cmtalbert 2008-02-08 00:02:57 PST
Comment on attachment 299482 [details] [diff] [review]
storage TZID update piece

>Index: calendar/providers/storage/calStorageCalendar.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/providers/storage/calStorageCalendar.js,v
> 
>-        if (oldVersion == 6 && this.DB_SCHEMA_VERSION >= 7) {
>-            dump ("**** Upgrading schema from 6 -> 7\n");
>+        // run TZID updates both on db of version 6 and 7:
>+        if ((oldVersion == 6 || oldVersion == 7) && this.DB_SCHEMA_VERSION >= 8) {
>+            dump ("**** Upgrading schema from 6/7 -> 8\n");
Daniel as far as I can tell, this if statement is not evaluating to true.  I can't say why that would happen, it makes no sense.  It's got to be something obvious, I'm just too tired to see it.  But because of that the database doesn't get upgraded.

>                 // Update the version stamp, and commit.
>-                this.mDB.executeSimpleSQL("UPDATE cal_calendar_schema_version SET version = 7;");
>+                this.mDB.executeSimpleSQL("UPDATE cal_calendar_schema_version SET version = 8;");
>                 this.mDB.commitTransaction();
>-                oldVersion = 7;
>+                oldVersion = 8;
>             } catch (e) {
>                 dump ("+++++++++++++++++ DB Error: " + this.mDB.lastErrorString + "\n");
>                 Components.utils.reportError("Upgrade failed! DB Error: " +
>                                              this.mDB.lastErrorString);
>                 this.mDB.rollbackTransaction();
>                 throw e;
>             }
>         }
When the database upgrade fails, I DON'T see this catch ^^^ which makes me think we never go into the if statement at all.
> 
>-        if (oldVersion != 7) {
>+        if (oldVersion != 8) {
>             dump ("#######!!!!! calStorageCalendar Schema Update failed -- db version: " + oldVersion + " this version: " + this.DB_SCHEMA_VERSION + "\n");
>             throw Components.results.NS_ERROR_FAILURE;
Instead, I see this dump statement and the resulting throw.  I put in a bunch of dump statements to try to debug this, but none of them are showing up at all.  I'm not sure if I have a bad build or a bad head at this point.

So, I'm going to r- on this until we can get it working.
Comment 12 cmtalbert 2008-02-08 00:05:33 PST
Comment on attachment 299822 [details] [diff] [review]
patch v3.1

I tested the handling of the timezone upgrade on a new profile (no upgrade performed) and it worked very well.  I'm going to r=ctalbert on this part, with one caveat: Jerusalem is specified incorrectly.  I'll add a follow-on patch that properly details Jerusalem.
Comment 13 cmtalbert 2008-02-08 00:16:00 PST
Created attachment 302089 [details] [diff] [review]
New tzdata.c file with proper specification on Jerusalem

This is the new tzdata.c file containing a proper Jerusalem timezone.  My recommendation is to land the giant patch first, erase tzdata.c, update to the old tzdata.c from CVS, and then apply this patch.  The same will go for the next patch - the update to calendar/libical/zoneinfo/asia/jerusalem.ics, although the ICS file is small enough to simply copy by hand.
Comment 14 cmtalbert 2008-02-08 00:16:35 PST
Created attachment 302090 [details] [diff] [review]
Updated ICS file for Jerusalem
Comment 15 cmtalbert 2008-02-08 00:21:59 PST
Created attachment 302094 [details]
Tests for the upgrade

These are the ICS files you need to adequately test the upgrade of the timezones to the current definition.  You have to be sure that the Daylight savings updates are applied to Adeliade, Auckland, that Jerusalem is correct, that JanMayen becomes Oslo, and that Caracas gets it's half hour offset.  You should also just vet the other zones here too.

 == Steps ==
1. Start up Calendar using a nightly
2. Import the "import me" calendar into a storage calendar
3. Open the "open me" calendar as an ICS file.
4. Close Calendar
5. Apply the patches to your build, rebuild
6. Open that same profile with the patched build.
7. Use http://www.timeanddate.com/worldclock/advmeeting.html to help check that all timezones are upgraded properly.
Comment 16 Daniel Boelzle [:dbo] 2008-02-11 10:18:39 PST
Comment on attachment 302089 [details] [diff] [review]
New tzdata.c file with proper specification on Jerusalem

r=dbo
Comment 17 Daniel Boelzle [:dbo] 2008-02-11 10:18:55 PST
Comment on attachment 302090 [details] [diff] [review]
Updated ICS file for Jerusalem

r=dbo
Comment 18 Daniel Boelzle [:dbo] 2008-02-11 10:19:35 PST
Created attachment 302622 [details] [diff] [review]
storage TZID update piece
Comment 19 :Hb 2008-02-12 05:40:50 PST
UTC is needed as a separate time zone. Europe/London is not a replacement because it has a normal time / daylight saving time border in it.
Comment 20 :Hb 2008-02-12 09:30:53 PST
(In reply to comment #19)

Filled the separate bug 417020 for this.

Comment 21 :Hb 2008-02-16 08:57:55 PST
Created attachment 303732 [details] [diff] [review]
Patch v5 for tzdata.c with Australian DST exceptions

The original file and/or patch #302089 (2008-02-08 00:16 PST) has for the timezones Cairo, Currie, Hobart and Perth identical TZOFFSETFROM and TZOFFSETTO values in standard time. 

Additionally RDATEs are set to keep the Australian DT/DST changes valid since  year 2000 with the new recurrence rules. I assume that the the referendum after the great "Western Australia Daylight Saving Trial 2006-2009" which affects the Perth time zone will give a GO for daylight saving time and that the DT/DST switch days will be changed to the nationwide dates.

Should the single ICS files under mozilla/calendar/libical/zoneinfo/ be maintained? This is done for Jerusalem but for Buenos Aires not for example.
Comment 22 :Hb 2008-02-16 09:02:37 PST
*** Bug 417808 has been marked as a duplicate of this bug. ***
Comment 23 cmtalbert 2008-02-16 19:58:56 PST
(In reply to comment #21)
> Created an attachment (id=303732) [details]
> Patch v5 for tzdata.c with Australian DST exceptions
Hb, thanks for the patch, it looks really good.  I do have a couple of general questions before I r+ though.
> 
> The original file and/or patch #302089 (2008-02-08 00:16 PST) has for the
> timezones Cairo, Currie, Hobart and Perth identical TZOFFSETFROM and TZOFFSETTO
> values in standard time. 
I'll look at these.  Thanks for mentioning it.  

> 
> Additionally RDATEs are set to keep the Australian DT/DST changes valid since 
> year 2000 with the new recurrence rules. I assume that the the referendum after
> the great "Western Australia Daylight Saving Trial 2006-2009" which affects the
> Perth time zone will give a GO for daylight saving time and that the DT/DST
> switch days will be changed to the nationwide dates.
Where did you pull the rDate information from?

> 
> Should the single ICS files under mozilla/calendar/libical/zoneinfo/ be
> maintained? This is done for Jerusalem but for Buenos Aires not for example.
> 
It's probably not necessary so much.  We basically only have them in there for reference, and the most up-to-date files are only checked in on trunk (which I just discovered).  So, we'll be fixing that with these checkin.


Comment 24 cmtalbert 2008-02-16 23:06:21 PST
Comment on attachment 303732 [details] [diff] [review]
Patch v5 for tzdata.c with Australian DST exceptions

r=ctalbert

I verified these, and I verified the zones that you updated (Cairo, Curries, Hobart, and Perth). Thanks for catching those.  This looks good.  In the future, the bugzilla user I watch is ctalbert at mozilla dot com.  So, us that one next time you need a review.  Nice work.
Comment 25 cmtalbert 2008-02-16 23:15:26 PST
Comment on attachment 302622 [details] [diff] [review]
storage TZID update piece

>Index: calendar/providers/storage/calStorageCalendar.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/providers/storage/calStorageCalendar.js,v
>+                // Schema changes between v7 and v8:
>+                //
>+                // - Migrate all stored mozilla.org timezones from 20070129_1
>+                //   to 20071231_1.
>+
>                 // Get a list of the /mozilla.org/* timezones used in the db
>                 var tzId;
>                 getTzIds = createStatement(this.mDB,
>                     "SELECT DISTINCT(zone) FROM ("+
>                         "SELECT recurrence_id_tz AS zone FROM cal_attendees  WHERE recurrence_id_tz LIKE '/mozilla.org%' UNION " +
>                         "SELECT recurrence_id_tz AS zone FROM cal_events     WHERE recurrence_id_tz LIKE '/mozilla.org%' UNION " +
>                         "SELECT event_start_tz   AS zone FROM cal_events     WHERE event_start_tz   LIKE '/mozilla.org%' UNION " +
>                         "SELECT event_end_tz     AS zone FROM cal_events     WHERE event_end_tz     LIKE '/mozilla.org%' UNION " +
>@@ -1215,29 +1221,29 @@ calStorageCalendar.prototype = {
>                             "UPDATE cal_todos      SET recurrence_id_tz = '" + update.newTzId + "' WHERE recurrence_id_tz = '" + update.oldTzId + "'; " +
>                             "UPDATE cal_todos      SET todo_entry_tz    = '" + update.newTzId + "' WHERE todo_entry_tz    = '" + update.oldTzId + "'; " +
>                             "UPDATE cal_todos      SET todo_due_tz      = '" + update.newTzId + "' WHERE todo_due_tz      = '" + update.oldTzId + "'; " +
>                             "UPDATE cal_todos      SET alarm_time_tz    = '" + update.newTzId + "' WHERE recurrence_id_tz = '" + update.oldTzId + "';");

I have one concern, but I don't think it is that bad as long as we communicate this very clearly.  During my testing, I worked with a 0.7 build converting it to a 0.8 build, which is what most people will likely be doing.  That worked ok.  However, when I first began testing, I used an old 0.8 nightly and attempted to use that.  In the old 0.8 nightly, this code above failed to upgrade any timezones because the TZID was not stored in the database, instead the full timezone definition was being stored for "mozilla" zones in the old 0.8 build.  This is incorrect per the implementation as far as I can tell, so I'm not sure how it got into that state.  Working with a current 0.8 nightly, only the TZID is stored in the database for mozilla zones, which is proper.  

So, the point here is that for some users on some builds of our nightlies, the timezone upgrade will fail.  It will cause the application to throw and it will no longer startup properly.  I think users running old releases will be ok.  What concerns me is that we do have a fair contingent of users running nightlies.

I'm going to wait a day or two to check this in to see if anyone has comment on it.  I'll also post to the blog about it to warn folks.

r=ctalbert
Comment 26 Gary Kwong [:gkw] [:nth10sd] 2008-02-17 03:31:00 PST
> So, the point here is that for some users on some builds of our nightlies, the
> timezone upgrade will fail.  It will cause the application to throw and it will
> no longer startup properly.  I think users running old releases will be ok. 
> What concerns me is that we do have a fair contingent of users running
> nightlies.

Re: nightlies issue, is that worthy enough to go into the release notes for 0.8?
Comment 27 cmtalbert 2008-02-18 20:18:37 PST
(In reply to comment #26)
> 
> Re: nightlies issue, is that worthy enough to go into the release notes for
> 0.8?
> 
No, I don't think so because the release notes are for the release, not the nightlies.

Ok.  I have checked in this monster.  Thanks to everyone that stepped up to help out with patches: nth10sd, dbo, and Hb.  

While attempting the checkin I realized that back in the 0.3.1 days the new ICS files only got checked in on TRUNK, so these diff's only apply to those trunk files.  I checked in the new ICS files only on trunk, and cross-committed the rest of the code.  

I think that since we only check these in for a loose reference anyway, this is ok.  It'd be difficult and pointless to update those files on BRANCH too, so we should probably just make a follow on bug to CVS REMOVE them from branch and continue with this tradition of updating them on trunk only.

With great pleasure, marking this bug as FIXED.  

 

Comment 28 cmtalbert 2008-02-18 20:21:42 PST
Follow on bug mentioned is Bug 418336
Comment 29 Bas van den Bosch 2008-02-18 22:37:49 PST
> > Re: nightlies issue, is that worthy enough to go into the release notes for
> > 0.8?
> > 
> No, I don't think so because the release notes are for the release, not the
> nightlies.

I think this should be relnoted. We've been referring to nigtly builds on the forums, the weblog and the ng's for much desired features a lot. So probably a lot of users are running 0.8 nightlies already and may run into trouble.
Comment 30 Stefan Sitter 2008-02-19 04:39:19 PST
The timezone dialog in the preference pane and the event dialog are not updated, therefore they will not work anymore:
http://lxr.mozilla.org/seamonkey/search?string=20070129_
Comment 31 Gary Kwong [:gkw] [:nth10sd] 2008-02-19 07:57:41 PST
Created attachment 304235 [details] [diff] [review]
Fix v1

Fix for broken timezone and event dialogs.
Comment 32 Gary Kwong [:gkw] [:nth10sd] 2008-02-19 08:49:55 PST
Created attachment 304241 [details] [diff] [review]
Fix v2

Apologies, forgot to remove menu item for Jan Mayen now that it's no longer in the 2007k Olson database.
Comment 33 Philipp Kewisch [:Fallen] 2008-02-19 09:46:19 PST
Comment on attachment 304241 [details] [diff] [review]
Fix v2

Taking over review since I have a moment of time :)

Changes look good, r=philipp
Comment 34 Philipp Kewisch [:Fallen] 2008-02-19 09:50:47 PST
Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Comment 35 cmtalbert 2008-02-19 11:19:59 PST
Thanks Philipp!  Good catch Stefan!
Comment 36 :Hb 2008-02-27 23:39:50 PST
*** Bug 419793 has been marked as a duplicate of this bug. ***
Comment 37 Stefan Sitter 2008-03-17 03:09:17 PDT
*** Bug 423304 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.