GNOME Bugzilla – Bug 722419
Inconsistent mapping between Olson and Windows format
Last modified: 2014-02-07 20:00:54 UTC
For a few old/obscure timezones we can face the following problem: - Zone offset is mapped to closest possible in MSDN, but as the timezone is not used anymore, resulting in a different time. One example is Asia/Novosibirsk, that used to be UTC+7, but nowadays is in the same one than Asia/Almaty, that is UTC+6. Looks like Asia/Novosibirsk is not used anymore but tzdata still keeps it and mapping to MSDN format it results in a UTC+6. The mapping table can be seen here: http://msdn.microsoft.com/en-us/library/ms912391(v=winembedded.11).aspx
Talking to Yoshito Umaoka, CLDR[0] maintainer of the mapping table between Olson (nowadays IANA) and MSDN format[1], I got the information that here are a few Olson's timezone that are unmmappables[2]. Long story (not too) short is (from [3]): /* * There are some Olson time zones that do not have the same base UTC offset in * Windows time zones. These zones are not supported by Windows. */ static final String[] NO_BASE_OFFSET_MATCH_ZONES_ARRAY = { "Australia/Eucla", // +8:45 "Australia/Lord_Howe", // +10:30 "Etc/GMT-14", // +14:00 "Pacific/Chatham", // +12:45 "Pacific/Kiritimati", // +14:00 "Pacific/Marquesas", // -9:30 "Pacific/Norfolk", // +11:30 }; /* * These Olson time zones are using different DST rules from Windows zones with * same base offset. */ static final String[] NO_DST_RULE_MATCH_ZONES_ARRAY = { // UTC-10:00/North American DST rule. // Closest match - "Hawaiian Standard Time" (no DST) "America/Adak", // UTC-08:00/no DST. // Closest match - "Pacific Standard Time" (observes DST). "Etc/GMT+8", "America/Metlakatla", "Pacific/Pitcairn", // UTC-09:00/no DST // Closest match - "Alaskan Standard Time" (observes DST). "Etc/GMT+9", "Pacific/Gambier", // UTC-06:00/Southern Hemisphere style DST rule. // Closest match - "Central America Standard Time" (observes Northern Hemisphere style DST rule). "Pacific/Easter", // UTC-03:00 zone with North American DST rule. // Closest match - "Greenland Standard Time" (observes EU DST rule). "America/Miquelon", // UTC+02:00 with DST (Mar - Sep). // Closest match - "E. Europe Standard Time", "Israel Standard Time" and some others "Asia/Gaza", "Asia/Hebron", }; The whole Yoshito's answer can be seen here[4] And what I'd like to propose is: We can drop our mapping table in favor to use CLDR's one[1] and use it (almost) as it is, only with a few changes added on the end of the file (what means that will be easy to maintain), once they are working seriously on maintain this up-to-dated and I already have it ready to go on EWS code. For those unmmappable timezones, I'd like remove it from our list as we don't want show wrong info to the users. Milan, do you agree in filtering out those data? Okay. Now the side-effects: CLDR uses apache-license, besides on this mapping file, which one uses unicode license. According to Alberto, we can use the file inside EWS without any problem. Even with apache-license being not compatible with GPLv2, the unicode one does the trick! :-) I'd like to see this code inside libical and not inside evolution-ews. But I'm not going to add it there for now. It would be a lot of time spent in a question that's being done in a simple way in our project (thanks glib and its friends :-)) Milan, do you agree with the proposal? Let me know your thoughts, please. Best Regards, -------------------------------------------------------------------------------- [0]: http://cldr.unicode.org/ [1]: http://www.unicode.org/repos/cldr/trunk/common/supplemental/windowsZones.xml [2]: http://cldr.unicode.org/development/development-process/design-proposals/extended-windows-olson-zid-mapping#TOC-Unmappable-Olson-Time-Zones [3]: http://source.icu-project.org/repos/icu/icuapps/trunk/WinTZ/src/com/ibm/icu/dev/tools/wintz/mapper/MapData.java [4]: http://mm.icann.org/pipermail/tz/2014-January/020586.html
What you might need to do is to make timezone picker dialog extensible, and then write an ews extension which will filter out those unmapable timezones from it. I do not want to filter out certain timezones only because one of the 3rd-party connectors doesn't have proper mapping to its native timezones. Still, you need to make sure that if a user copies an old event into ews calendar, then it'll use right timezone or claim an error. Thus it seems like much easier to just reject create/modify for timezones you cannot store on the MS server; though we found a way to describe the timezone, even if it's not part of the MS predefined zones, did we not? With respect of copying anything from CLDR, what would be the gain with it, please? You mentioned possible legal issues, while what you really want is only the mapping table, which you'll still need to maintain, at least in a sense of making sure that you use the right/most-up-to-date version from CLDR, which doesn't add too much value, especially with the (possible) licensing issue. Thus I would just sync current mapping table with the findings from their, as I suppose most of the current mapping is still correct, and you only need to add some exceptions, thus the most of the work is done by you, not by the CLDR project (I'm not much used to legal questions, to be honest).
(In reply to comment #2) > What you might need to do is to make timezone picker dialog extensible, and > then write an ews extension which will filter out those unmapable timezones > from it. I do not want to filter out certain timezones only because one of the > 3rd-party connectors doesn't have proper mapping to its native timezones. Yeah, I completely agree with you, if it's really necessary to do. > Still, you need to make sure that if a user copies an old event into ews > calendar, then it'll use right timezone or claim an error. Thus it seems like > much easier to just reject create/modify for timezones you cannot store on the > MS server; though we found a way to describe the timezone, even if it's not > part of the MS predefined zones, did we not? Kind of. We can save the events, it will be store but, IIUC, the whole timezone "management" will not be done (basically, handle DST and bla bla bla). > > With respect of copying anything from CLDR, what would be the gain with it, > please? The gain would be: - We don't have to maintain and update our table, we can just c&p their file and update it when they update it in CLDR. - With small lines of code (or another file, or on bottom of their file, or on our code) filter that small list of not covered timezone. > You mentioned possible legal issues, while what you really want is only > the mapping table, which you'll still need to maintain, at least in a sense of > making sure that you use the right/most-up-to-date version from CLDR, which > doesn't add too much value, especially with the (possible) licensing issue. > Thus I would just sync current mapping table with the findings from their, as I > suppose most of the current mapping is still correct, and you only need to add > some exceptions, thus the most of the work is done by you, not by the CLDR > project (I'm not much used to legal questions, to be honest). About the legal issues, I was afraid of c&p and use a file from another project when the licenses don't match. I can be a problem, from my point of view, but I'm not into these legal stuff. I've asked Alberto about his and he said we can use the file without any problem. The unicode license matches with any version of GPL, IIUC.
Created attachment 267607 [details] [review] Bug #722419 - Inconsistent mapping between Olson and MSDN format
(In reply to comment #4) > Bug #722419 - Inconsistent mapping between Olson and MSDN format Ah, Milan, this patch was rebased on the top of the patch provided for https://bugzilla.gnome.org/show_bug.cgi?id=722416 So, please, review the 722416 firstly.
Review of attachment 267607 [details] [review]: So this patch looks fine, the below things are only side notes, or my questions, nothing important. Unfortunately, I've got this crash, twice in a row, thus it seems like we are not able to use this, because of libical bug:
+ Trace 233095
Thread 4 (Thread 0x7f971effd700 (LWP 12605))
::: src/calendar/e-cal-backend-ews-utils.c @@ +88,3 @@ } + xpath_eval_exp = "//supplementalData/windowsZones/mapTimezones/mapZone"; double slash at the beginning of the xpath? CalDAV uses single slash. @@ +1787,3 @@ } else { + if (satisfies) + e_ews_message_replace_server_version (msg, E_EWS_EXCHANGE_2007_SP1); Why not do it always? It might not have any bad side-effects, right? ditto for the similar case above. ::: tests/ews-test-timezones.c @@ +25,3 @@ #include "ews-test-common.h" +void (* populate_windows_zones) (void); I did not look on this earlier, but such definition looks suspicious to me. Why cannot it be something sane, like without that leading '*'?
Milan, I've tried to reproduce this crash but I'm not able to (using latest libical, from its svn). Could you try again with the new patches, please? About the: +void (* populate_windows_zones) (void); How is the sanest way to declare a pointer to a function? I'm updating the patch with the another problems fixed.
Created attachment 267907 [details] [review] Bug #722419 - Inconsistent mapping between Olson and MSDN format
(In reply to comment #7) > I've tried to reproduce this crash but I'm not able to (using latest libical, > from its svn). Better to test with libical which people use, than with something which will be released in next several months (I mean, even the 0.48 might be used the most, probably, then 1.0 should be OK to test with). > About the: > +void (* populate_windows_zones) (void); > How is the sanest way to declare a pointer to a function? Oh, so it was meant to be a pointer to a function, hmm, OK, in that case this seems a bit crazy to have a global variable for a function pointer. Why cannot you pass a pointer to that function in a function parameter? It might make more sense this way, not depending on any 3rd party to setup the function pointer before certain functions are called (remember how hard it is to follow ews memory management, this kind of construction doesn't add much to it too). I'll retest and will let you know.
Created attachment 268260 [details] [review] Bug #722419 - Inconsistent mapping between Olson and MSDN format
Created attachment 268332 [details] [review] Bug #722419 - Inconsistent mapping between Olson and MSDN format
Review of attachment 268332 [details] [review]: Untested, but the change looks fine. I thought we also spoke that you'll claim an error if users will try to save or modify an event with an unsupported timezone, thus they will know about the issue that such timezone cannot be saved on the Exchange server and that the time can be slightly shifted afterwards (which also leads to a possibility to not use CLDR file directly in evolution-ews, but only extract some parts out of it). ::: src/calendar/e-cal-backend-ews-utils.c @@ +148,3 @@ + + if (msdn_to_ical != NULL) + g_hash_table_unref (msdn_to_ical); Thinking of it, the last unref will not set the variable to NULL, thus you can get to use-after-free easily (applicable to both variables in this function). @@ +161,2 @@ g_rec_mutex_lock (&tz_mutex); + msdn_tz_location = g_hash_table_lookup (ical_to_msdn, ical_tz_location); You may claim error when the global variables are not initialized here. Similarly in the next function. ::: src/server/e-ews-message.c @@ +343,3 @@ + result = xpath_eval (xpctx, "/s:Envelope/s:Header/t:RequestServerVersion"); + if (result == NULL) + goto exit; this goto can be avoided, right? ::: tests/ews-test-timezones.c @@ +25,3 @@ #include "ews-test-common.h" +void (* populate_windows_zones) (void); We had a chat about this on IRC few days ago, and the outcome is that it's OK to have it done this way for now, but later you should pass the common parts into common libraries, either under src/server or src/utils, thus the functions will be reusable easily in the test code and in the production code.
Created attachment 268416 [details] [review] Bug #722419 - Inconsistent mapping between Olson and Windows format
Review of attachment 268416 [details] [review]: Untested, but looks fine. Please fix the below things and commit to master. Thanks. ::: src/calendar/e-cal-backend-ews-utils.c @@ +100,3 @@ + if (xpath_obj == NULL) { + g_warning (G_STRLOC "Unable to evaluate xpath expression \"%s\".", xpath_eval_exp); + xmlXPathFreeContext (xpath_ctxt); xmlFreeDoc is missing here @@ +135,3 @@ + g_strfreev (tokens); + g_free (ical); + g_free (msdn); as long as it's a libxml allocated memory, you should use xmlFree() to free it (or some such function) ::: src/server/e-ews-message.c @@ +303,3 @@ + va_end (args); + + result = xmlXPathEvalExpression ((xmlChar *) expr, ctx); it would be nice to stay consistent with casting, either use BAD_CAST macro, or the above (xmlChar *) thing. (I didn't know of the BAD_CAST macro, by the way, but it's nice they provide it) :)
Created attachment 268448 [details] [review] Bug #722419 - Inconsistent mapping between Olson and Windows format
Attachment 268448 [details] pushed as 0e40814 (evo-ews 3.11.90+)