Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Timezone update workflow/tool doesn't work? #44865

Closed
richardlau opened this issue Oct 2, 2022 · 2 comments
Closed

Timezone update workflow/tool doesn't work? #44865

richardlau opened this issue Oct 2, 2022 · 2 comments

Comments

@richardlau
Copy link
Member

ICU updated to tzdata 2022d recently unicode-org/icu-data#30.
https://github.com/nodejs/node/actions/runs/3166758920 should have picked this up and opened a PR but it doesn't appear to have done.
@RaisinTen
Copy link
Contributor

RaisinTen commented Oct 2, 2022 •
edited
Loading

Happens because

node/tools/update-timezone.mjs

Lines 29 to 35 in a16f81e

spawnSync(
'icupkg', [
'-a',
file,
'icudt*.dat',
], { cwd: 'deps/icu-small/source/data/in/' }
);
uses globbing, so the command silently fails with a icupkg: unable to open input file "icudt*.dat" error which shows up when you inspect the stderr property on the returned object stderr. Replacing with execSync works (I've tested on my fork, see https://github.com/RaisinTen/node/actions/runs/3171963122/jobs/5165930627 and RaisinTen#2).

RaisinTen added a commit to RaisinTen/node that referenced this issue Oct 3, 2022
@RaisinTen
build: fix timezone update tool
574d9b0 
The spawnSync call was previously silently failing with this error:
```sh
icupkg: unable to open input file "icudt*.dat"
```
because spawnSync doesn't support globbing. This change replaces the
spawnSync call with execSync because that supports globbing.

I have tested this workflow with some minor modifications in my fork and I
can confirm that it works as expected now. The bot opened this PR -
#2 which updates
deps/icu-small/source/data/in/icudt71l.dat.bz2.

Fixes: nodejs#44865
Signed-off-by: Darshan Sen <raisinten@gmail.com>
RaisinTen added a commit to RaisinTen/node that referenced this issue Oct 3, 2022
@RaisinTen
tools: fix timezone update tool
9d26788 
The spawnSync call was previously silently failing with this error:
```sh
icupkg: unable to open input file "icudt*.dat"
```
because spawnSync doesn't support globbing. This change replaces the
spawnSync call with execSync because that supports globbing.

I have tested this workflow with some minor modifications in my fork and I
can confirm that it works as expected now. The bot opened this PR -
#2 which updates
deps/icu-small/source/data/in/icudt71l.dat.bz2.

Fixes: nodejs#44865
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen mentioned this issue Oct 3, 2022
Closed
@RaisinTen
Copy link
Contributor

PR - #44870

RaisinTen added a commit to RaisinTen/node that referenced this issue Oct 3, 2022
@RaisinTen
tools: fix timezone update tool
0210498 
The spawnSync call was previously silently failing with this error:
```sh
icupkg: unable to open input file "icudt*.dat"
```
because spawnSync doesn't support globbing. This change replaces the
spawnSync call with execSync because that supports globbing.

I have tested this workflow with some minor modifications in my fork and
I can confirm that it works as expected now. The bot opened this PR -
#2 which updates
deps/icu-small/source/data/in/icudt71l.dat.bz2.

Fixes: nodejs#44865
Signed-off-by: Darshan Sen <raisinten@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Oct 10, 2022
@RaisinTen @nodejs-github-bot
tools: remove faulty early termination logic from update-timezone.mjs
b5add97 
We do not build Node.js in the workflow so
https://github.com/nodejs/node/blob/f4815fcd7691364d8139b44c1295dbc46f6ee4a8/tools/update-timezone.mjs#L18
is actually the version of `tzdata` in the Node.js in the runner instead
of what's in `main`.

The script is pretty fast even when the versions differ and there is an
update, so this optimization doesn't seem to be worth having given the
problem.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #44870
Fixes: #44865
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this issue Oct 11, 2022
@RaisinTen @danielleadams
tools: fix timezone update tool
19d8574 
The spawnSync call was previously silently failing with this error:
```sh
icupkg: unable to open input file "icudt*.dat"
```
because spawnSync doesn't support globbing. This change replaces the
spawnSync call with execSync because that supports globbing.

I have tested this workflow with some minor modifications in my fork and
I can confirm that it works as expected now. The bot opened this PR -
RaisinTen#2 which updates
deps/icu-small/source/data/in/icudt71l.dat.bz2.

Fixes: #44865
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #44870
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this issue Oct 11, 2022
@RaisinTen @danielleadams
tools: remove faulty early termination logic from update-timezone.mjs
4bdef48 
We do not build Node.js in the workflow so
https://github.com/nodejs/node/blob/f4815fcd7691364d8139b44c1295dbc46f6ee4a8/tools/update-timezone.mjs#L18
is actually the version of `tzdata` in the Node.js in the runner instead
of what's in `main`.

The script is pretty fast even when the versions differ and there is an
update, so this optimization doesn't seem to be worth having given the
problem.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #44870
Fixes: #44865
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
richardlau pushed a commit that referenced this issue Nov 24, 2022
@RaisinTen @richardlau
tools: fix timezone update tool
543493c 
The spawnSync call was previously silently failing with this error:
```sh
icupkg: unable to open input file "icudt*.dat"
```
because spawnSync doesn't support globbing. This change replaces the
spawnSync call with execSync because that supports globbing.

I have tested this workflow with some minor modifications in my fork and
I can confirm that it works as expected now. The bot opened this PR -
RaisinTen#2 which updates
deps/icu-small/source/data/in/icudt71l.dat.bz2.

Fixes: #44865
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #44870
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
richardlau pushed a commit that referenced this issue Nov 24, 2022
@RaisinTen @richardlau
tools: remove faulty early termination logic from update-timezone.mjs
891368c 
We do not build Node.js in the workflow so
https://github.com/nodejs/node/blob/f4815fcd7691364d8139b44c1295dbc46f6ee4a8/tools/update-timezone.mjs#L18
is actually the version of `tzdata` in the Node.js in the runner instead
of what's in `main`.

The script is pretty fast even when the versions differ and there is an
update, so this optimization doesn't seem to be worth having given the
problem.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #44870
Fixes: #44865
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
@RaisinTen @guangwong
tools: fix timezone update tool
af2d9cc 
The spawnSync call was previously silently failing with this error:
```sh
icupkg: unable to open input file "icudt*.dat"
```
because spawnSync doesn't support globbing. This change replaces the
spawnSync call with execSync because that supports globbing.

I have tested this workflow with some minor modifications in my fork and
I can confirm that it works as expected now. The bot opened this PR -
RaisinTen/node#2 which updates
deps/icu-small/source/data/in/icudt71l.dat.bz2.

Fixes: nodejs/node#44865
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs/node#44870
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
@RaisinTen @guangwong
tools: remove faulty early termination logic from update-timezone.mjs
2894ce3 
We do not build Node.js in the workflow so
https://github.com/nodejs/node/blob/f4815fcd7691364d8139b44c1295dbc46f6ee4a8/tools/update-timezone.mjs#L18
is actually the version of `tzdata` in the Node.js in the runner instead
of what's in `main`.

The script is pretty fast even when the versions differ and there is an
update, so this optimization doesn't seem to be worth having given the
problem.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs/node#44870
Fixes: nodejs/node#44865
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
@RaisinTen @guangwong
tools: fix timezone update tool
f25536d 
The spawnSync call was previously silently failing with this error:
```sh
icupkg: unable to open input file "icudt*.dat"
```
because spawnSync doesn't support globbing. This change replaces the
spawnSync call with execSync because that supports globbing.

I have tested this workflow with some minor modifications in my fork and
I can confirm that it works as expected now. The bot opened this PR -
RaisinTen/node#2 which updates
deps/icu-small/source/data/in/icudt71l.dat.bz2.

Fixes: nodejs/node#44865
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs/node#44870
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
@RaisinTen @guangwong
tools: remove faulty early termination logic from update-timezone.mjs
5954779 
We do not build Node.js in the workflow so
https://github.com/nodejs/node/blob/f4815fcd7691364d8139b44c1295dbc46f6ee4a8/tools/update-timezone.mjs#L18
is actually the version of `tzdata` in the Node.js in the runner instead
of what's in `main`.

The script is pretty fast even when the versions differ and there is an
update, so this optimization doesn't seem to be worth having given the
problem.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs/node#44870
Fixes: nodejs/node#44865
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

tools: fix timezone update tool RaisinTen/node
2 participants
@richardlau @RaisinTen