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

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

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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