Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lib: support setting process.env.TZ on windows #38642

Closed
wants to merge 2 commits into from
Closed

lib: support setting process.env.TZ on windows #38642

wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented May 11, 2021 •
edited
Loading

Fixes: #4230
Signed-off-by: James M Snell jasnell@gmail.com

Update: Even tho this is semver-patch, I'm marking it as a notable change for when it lands. The bug was first reported back in Dec 2015, it was closed for a while, one partial attempt at fixing it was made a couple of years ago, and this completes the fix.

/cc @targos @srl295

@github-actions github-actions bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 11, 2021
@jasnell jasnell mentioned this pull request May 12, 2021
Closed
jasnell
jasnell commented May 12, 2021
View reviewed changes
src/node_env_var.cc Outdated Show resolved Hide resolved
srl295
srl295 reviewed May 12, 2021
View reviewed changes
src/node_env_var.cc Outdated Show resolved Hide resolved
@srl295
Copy link
Member

srl295 commented May 12, 2021 •
edited
Loading

Might just have NODE_DEFAULT_ZONE instead of trying to emulate TZ. don't use abbrevs just tz IDs like America/Los_Angeles.

@jasnell
Copy link
Member Author

jasnell commented May 12, 2021

@srl295:

Might just have NODE_DEFAULT_ZONE instead of trying to emulate TZ.

In the current version of the PR, use of set TZ="..." does work. Is there a particular limitation here that would make it undesirable? What would using NODE_DEFAULT_ZONE give us? My concern would be introducing something that is inconsistent with TZ on non-windows systems.

@jasnell jasnell marked this pull request as ready for review May 12, 2021 02:25
@srl295
Copy link
Member

srl295 commented May 12, 2021

@srl295:

Might just have NODE_DEFAULT_ZONE instead of trying to emulate TZ.

In the current version of the PR, use of set TZ="..." does work. Is there a particular limitation here that would make it undesirable? What would using NODE_DEFAULT_ZONE give us? My concern would be introducing something that is inconsistent with TZ on non-windows systems.

What matt was alluding to is that it's already inconsistent. Posix does a bunch of other processing. Node-default-zone would make it clear we are simply choosing a new default zone by the full id rather than trying To emulate Posix TZ on windows.

srl295
srl295 approved these changes May 12, 2021
View reviewed changes
Copy link
Member

@srl295 srl295 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll say lgtm with caveats that it will not be === TZ behavior on posix, suggestions as noted in thread.

@jasnell
Copy link
Member Author

jasnell commented May 12, 2021

I'll say lgtm with caveats that it will not be === TZ behavior on posix, suggestions as noted in thread.

Understood. I think those differences will be ok here. I'll try to find a good place to describe those differences in the docs

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@srl295
Copy link
Member

srl295 commented May 12, 2021 •
edited
Loading

I'll say lgtm with caveats that it will not be === TZ behavior on posix, suggestions as noted in thread.

Understood. I think those differences will be ok here. I'll try to find a good place to describe those differences in the docs

one reference: https://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html
ICU won't necessarily handle those completely either, but it will try to find a matching zone.
Perhaps something like this:

While the TZ env variable on Node.js won't handle all of the various ways that the TZ variable is handled in other environments, it will support tz ids (such as Etc/UTC, Europe/Paris or America/New_York. It may support a few other abbreviations or aliases, but these are strongly discouraged.

sorry not sorry for being a stick in the timezone mud :)

@jasnell jasnell force-pushed the datetime-change-notify branch 2 times, most recently from 3f15012 to cc783d2 Compare May 12, 2021 19:42
srl295
srl295 approved these changes May 12, 2021
View reviewed changes
Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🇮🇪

@nodejs-github-bot

This comment has been minimized.

Sign in to view
addaleax
addaleax requested changes May 12, 2021
View reviewed changes
src/node_i18n.cc Outdated Show resolved Hide resolved
mcollina
mcollina approved these changes May 12, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

shalvah
shalvah reviewed May 13, 2021
View reviewed changes
doc/api/cli.md Outdated Show resolved Hide resolved
@jasnell jasnell requested a review from addaleax May 13, 2021 03:35
@nodejs-github-bot

This comment has been minimized.

Sign in to view
addaleax
addaleax approved these changes May 13, 2021
View reviewed changes
doc/api/cli.md Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@jasnell
lib: support setting process.env.TZ on windows
b6a0fac 
Fixes: nodejs#4230
Signed-off-by: James M Snell <jasnell@gmail.com>
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label May 14, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 14, 2021 •
edited by jasnell
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/38099/ 💚

@jasnell
Copy link
Member Author

jasnell commented May 17, 2021

Landed in 16cb4f7

@jasnell jasnell closed this May 17, 2021
jasnell added a commit that referenced this pull request May 17, 2021
@jasnell
lib: support setting process.env.TZ on windows
16cb4f7 
Fixes: #4230
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #38642
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell jasnell added the notable-change PRs with changes that should be highlighted in changelogs. label May 17, 2021
targos pushed a commit that referenced this pull request May 18, 2021
@jasnell @targos
lib: support setting process.env.TZ on windows
c0deeea 
Fixes: #4230
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #38642
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos added a commit that referenced this pull request May 18, 2021
@targos
2021-05-18, Version 16.2.0 (Current)
32a8e15 
Notable changes:

async_hooks:
  * (SEMVER-MINOR) use new v8::Context PromiseHook API (Stephen Belanger) #36394
lib:
  * support setting process.env.TZ on windows (James M Snell) #38642
module:
  * (SEMVER-MINOR) add support for `URL` to `import.meta.resolve` (Antoine du Hamel) #38587
process:
  * (SEMVER-MINOR) add `'worker'` event (James M Snell) #38659
util:
  * (SEMVER-MINOR) add util.types.isKeyObject and util.types.isCryptoKey (Filip Skokan) #38619

PR-URL: #38719
@targos targos mentioned this pull request May 18, 2021
Merged
targos added a commit that referenced this pull request May 18, 2021
@targos
2021-05-18, Version 16.2.0 (Current)
d3e64be 
Notable changes:

async_hooks:
  * (SEMVER-MINOR) use new v8::Context PromiseHook API (Stephen Belanger) #36394
lib:
  * support setting process.env.TZ on windows (James M Snell) #38642
module:
  * (SEMVER-MINOR) add support for `URL` to `import.meta.resolve` (Antoine du Hamel) #38587
process:
  * (SEMVER-MINOR) add `'worker'` event (James M Snell) #38659
util:
  * (SEMVER-MINOR) add util.types.isKeyObject and util.types.isCryptoKey (Filip Skokan) #38619

PR-URL: #38719
targos added a commit that referenced this pull request May 19, 2021
@targos
2021-05-19, Version 16.2.0 (Current)
6e69f84 
Notable changes:

async_hooks:
  * (SEMVER-MINOR) use new v8::Context PromiseHook API (Stephen Belanger) #36394
lib:
  * support setting process.env.TZ on windows (James M Snell) #38642
module:
  * (SEMVER-MINOR) add support for `URL` to `import.meta.resolve` (Antoine du Hamel) #38587
process:
  * (SEMVER-MINOR) add `'worker'` event (James M Snell) #38659
util:
  * (SEMVER-MINOR) add util.types.isKeyObject and util.types.isCryptoKey (Filip Skokan) #38619

PR-URL: #38719
targos added a commit that referenced this pull request May 19, 2021
@targos
2021-05-19, Version 16.2.0 (Current)
44ffdda 
Notable changes:

async_hooks:
  * (SEMVER-MINOR) use new v8::Context PromiseHook API (Stephen Belanger) #36394
lib:
  * support setting process.env.TZ on windows (James M Snell) #38642
module:
  * (SEMVER-MINOR) add support for `URL` to `import.meta.resolve` (Antoine du Hamel) #38587
process:
  * (SEMVER-MINOR) add `'worker'` event (James M Snell) #38659
util:
  * (SEMVER-MINOR) add util.types.isKeyObject and util.types.isCryptoKey (Filip Skokan) #38619

PR-URL: #38719
codebytere added a commit to electron/electron that referenced this pull request May 19, 2021
@codebytere
lib: support setting process.env.TZ on windows
0c498e7 
nodejs/node#38642
codebytere added a commit to electron/electron that referenced this pull request May 20, 2021
@codebytere
lib: support setting process.env.TZ on windows
e00d861 
nodejs/node#38642
codebytere added a commit to electron/electron that referenced this pull request May 31, 2021
@codebytere
lib: support setting process.env.TZ on windows
a34be21 
nodejs/node#38642
codebytere added a commit to electron/electron that referenced this pull request May 31, 2021
@codebytere
lib: support setting process.env.TZ on windows
78972e5 
nodejs/node#38642
codebytere added a commit to electron/electron that referenced this pull request Jun 8, 2021
@codebytere
lib: support setting process.env.TZ on windows
f0349b7 
nodejs/node#38642
codebytere added a commit to electron/electron that referenced this pull request Jun 9, 2021
@codebytere
lib: support setting process.env.TZ on windows
3492dca 
nodejs/node#38642
codebytere added a commit to electron/electron that referenced this pull request Jun 10, 2021
@codebytere
lib: support setting process.env.TZ on windows
6766b65 
nodejs/node#38642
@richardlau richardlau mentioned this pull request Jul 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shalvah shalvah shalvah left review comments

@addaleax addaleax addaleax approved these changes

@srl295 srl295 srl295 approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation. notable-change PRs with changes that should be highlighted in changelogs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

I can't set a default timezone on windows.
7 participants
@jasnell @srl295 @nodejs-github-bot @mcollina @addaleax @shalvah @targos