Closed Bug 1491159 Opened Last year Closed Last year

improve handling of TZ environment variable

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(1 file)

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 dbaron@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/75664900fcd9
More complete handling of TZ environment variable when initializing ICU timezone.  r=anba
https://hg.mozilla.org/mozilla-central/rev/75664900fcd9
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
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.