1491159 - improve handling of TZ environment variable

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Bug 1491159 - More complete handling of TZ environment variable when initializing ICU timezone. r=anba

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.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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...

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Simplified version uploaded.

Comment 4

[André Bargull [:anba]]

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75664900fcd9
More complete handling of TZ environment variable when initializing ICU timezone.  r=anba

Comment 6

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/75664900fcd9

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 8

[Pascal Chevrel:pascalc (PTO until April 26)]

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.

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/releases/mozilla-beta/rev/918738f6956f