Timezone list should contain recent and future rules

RESOLVED FIXED in 7.0

Status

enhancement
RESOLVED FIXED
10 months ago
2 months ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

unspecified
Dependency tree / graph

Details

Attachments

(2 attachments, 4 obsolete attachments)

Our zones file only contains one set of rules per timezone, but there's a change in timezone rules, then future events beyond the date of the change are wrong, and past events before the change are wrong once the change happens.

The timezone service is capable of handling multiple rules in the zones file, but we need to work out how to select which rules to have (avoiding unnecessary junk inflating the file size), and how to do it without changing just about every rule in the file (currently we're using sanitised data, but that exists only for the current rules).
Posted patch 1494160-tzupdate-1.diff (obsolete) β€” β€” Splinter Review
Fixing this was a lot harder than it looks, especially while trying to minimise the changes to zones.json. Please check those changes, I *think* they all correlate to actual changes, but I could easily have missed something.
Attachment #9013236 - Flags: review?(philipp)
Would it be possible with a reasonable effort to incorporate also definitions for past times here? See bug 1497463.
Flags: needinfo?(geoff)
Possible, yes, but it would result in a much larger and more difficult to review zones file, and what does it achieve?
Flags: needinfo?(geoff)
Blocks: 1502832
Comment on attachment 9013236 [details] [diff] [review]
1494160-tzupdate-1.diff

Review of attachment 9013236 [details] [diff] [review]:
-----------------------------------------------------------------

It looks pretty hard, tbh, which is why I've been delaying this. I'm not quite sure how to best check this patch, the code looks fine.

What I'm wondering, sometimes there are timezone changes that are corrections due to the rules previously being wrong. Would that be handled here, or would it then show the wrong (but technically correct) information?
Attachment #9013236 - Flags: review?(philipp) → review+

We'd be shipping the latest information about current and past timezones (i.e. with mistakes hopefully corrected), not the latest and previous versions of the information.

I guess if we shipped faulty information that could result in events changing time when the fault gets corrected, since we generally only record the timezone ID of an event. But that's already the case.

Does that answer your question, or am I misunderstanding?

I need to update the zones information again with this code, so I'll do that and get it checked. NI'ing myself so I remember.

Flags: needinfo?(geoff)

Yes, that answers my question. Thanks!

Posted patch 1494160-tzsplit-1.diff β€” β€” Splinter Review

I want to add a part zero to this, because checking changes make my brain explode. I propose here to store each STANDARD/DAYLIGHT component on a separate line of the JSON file (i.e. in an array of strings, not one big long string) for easier reading.

Flags: needinfo?(geoff)
Attachment #9059410 - Flags: review?(philipp)
Posted patch 1494160-tzupdate-2.diff (obsolete) β€” β€” Splinter Review

This is just a minor update to the earlier patch, and I've taken the actual zone changes out of it.

Attachment #9013236 - Attachment is obsolete: true
Attachment #9059411 - Flags: review+
Posted patch 1494160-dotzupdate-1.diff (obsolete) β€” β€” Splinter Review

And these are the actual changes, still at 2018i although there is a 2019a. It should be much easier to review now. I think I've done the right thing about bug 1515937.

Comment on attachment 9059410 [details] [diff] [review]
1494160-tzsplit-1.diff

Review of attachment 9059410 [details] [diff] [review]:
-----------------------------------------------------------------

With a change like this we'd likely have to make a change to the major version of the timezone file, but since we are not distributing the extension I think we can get away without.
Attachment #9059410 - Flags: review?(philipp) → review+

I'm unhappy with this, so I haven't shipped it. There's definitely some bad data produced, although we already had some bad data. Casablanca, for example, has daylight savings time but we don't list it. I bet we don't have many Moroccan users, but if we can't get the timezone right we'll have even fewer.

Posted patch 1494160-tzupdate-3.diff (obsolete) β€” β€” Splinter Review
Attachment #9059411 - Attachment is obsolete: true
Attachment #9059412 - Attachment is obsolete: true
Comment on attachment 9064635 [details] [diff] [review]
1494160-tzupdate-3.diff

Oh damn, now I remember why I didn't post this yesterday.
Attachment #9064635 - Attachment is obsolete: true

Okay, finally I think I have this right.

I've included the changes this patch would introduce for the years 2018-2022, which actually doesn't include the America/Caracas changes that the tests rely on. I'm not sure if we should go back that far, but if we do it will be easier to review as a second change.

I also haven't updated to version 2019a in this patch.

Attachment #9064663 - Flags: review?(philipp)
Comment on attachment 9064663 [details] [diff] [review]
1494160-tzupdate-4.diff

Review of attachment 9064663 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/timezones/update-zones.py
@@ +241,5 @@
>  
>          zones = {}
>          for entry in os.listdir(path):
> +            if entry == "Etc":
> +                continue

Are we not including Etc zones?
Attachment #9064663 - Flags: review?(philipp) → review+

Are we not including Etc zones?

We didn't include them before. I changed to a newer version of vzic, and I think that's what caused them to appear in the list.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/116e52b10f8f
Store timezone components in an array of strings, not one long string; r=Fallen
https://hg.mozilla.org/comm-central/rev/8c2e0c1e7756
Modify timezone update script so that zones.json can contain recent and future rules; r=Fallen

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED

I ended up pushing this without the tests that rely on America/Caracas changing in 2016. I did however, add a test for America/Sao_Paulo.

Target Milestone: --- → 7.0
You need to log in before you can comment on or make changes to this bug.