Closed Bug 1491159 Opened Last year Closed Last year
improve handling of TZ environment variable
In bug 1484829 comment 14, I wrote: > So the reason the fix didn't work for me is that I had only partially > switched from using TZ=/home/dbaron/.timezone (which works in most places, > but apparently didn't work with something else as well) to > TZ=:/home/dbaron/.timezone (with the added ":"). (By partially, I was using > the colon in my .bashrc (affecting terminals) but not in my .profile > (affecting programs started from GNOME, whose startup uses .profile but not > .bashrc). And I'd only made that change on one of the machines where I was > doing this; on another I wasn't using the : at all.) > > So... while it's not documented at > http://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html, the > filename syntax generally doesn't actually require the initial colon. > > In any case, I've changed my scripts to more reliably use the : since that's > apparently the POSIX-ly correct thing to do, but it may be worth supporting > filenames without the initial colon, i.e., if they start with a /. > > However, I'd also note that according to that documentation, > TZ=America/Los_Angeles isn't supported either, and TZ=:America/Los_Angeles > is required. Yet I'd think thinks like TZ=America/Los_Angeles might be one > of the common forms of using $TZ. I'm filing this bug to fix these issues; I'm currently testing a patch.
This improves on the changes in bug 1484829 by handling the filename form of TZ environment variables both with and without an initial ':' character, and as both relative and absolute paths. (Note that relative paths without the ':' are just Olson time zone names.) Note that the only test that failed without the patch was: setTimeZone("/usr/share/zoneinfo/America/Chicago"); assertEq(timeZoneName(), "Central Daylight Time"); and it reported Error: Assertion failed: got "GMT-06:00", expected "Central Daylight Time" which seems to be a sign that ICU gets some information out of that time zone identifier (i.e., it reported the correct standard-time GMT offset), but not everything.
Given that the patch doesn't change the behavior of either of these tests: setTimeZone(":Europe/Helsinki"); assertEq(timeZoneName(), "Eastern European Summer Time"); setTimeZone("::Europe/London"); // two colons, invalid assertEq(timeZoneName(), systemTimeZoneName); I think it's clear that ICU correctly supports both the "Europe/Helsinki" and ":Europe/Helsinki" timezone formats internally, so I should actually simplify the patch a bit...
Simplified version uploaded.
Comment on attachment 9008965 [details] Bug 1491159 - More complete handling of TZ environment variable when initializing ICU timezone. r=anba André Bargull [:anba] has approved the revision.
Attachment #9008965 - Flags: review+
Attachment #9008965 - Attachment description: Bug 1491159 - More complete handling of TZ environment variable when initializing ICU timezone. → Bug 1491159 - More complete handling of TZ environment variable when initializing ICU timezone. r=anba
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/75664900fcd9 More complete handling of TZ environment variable when initializing ICU timezone. r=anba
Comment on attachment 9008965 [details] Bug 1491159 - More complete handling of TZ environment variable when initializing ICU timezone. r=anba Approval Request Comment [Feature/Bug causing the regression]: bug 1346211 [User impact if declined]: incorrect timezone behavior, e.g., times being an hour off in apps like Slack or IRCCloud [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: no [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none known; this is a follow-up to the already-uplifted bug 1484829 [Is the change risky?]: no [Why is the change risky/not risky?]: it handles an additional case for TZ environment variable handling that may cover some additional users, but does so in a way that matches the way a similar but slightly-different-in-syntax TZ setting is already handled [String changes made/needed]: no
Attachment #9008965 - Flags: approval-mozilla-beta?
Comment on attachment 9008965 [details] Bug 1491159 - More complete handling of TZ environment variable when initializing ICU timezone. r=anba Fix for a timezone issue affecting popular sites, uplift approved for 63 beta 8, thanks.
Attachment #9008965 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.