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

tools: fix iculslocs to support ICU 65.1 #29523

Closed
wants to merge 0 commits into from
Closed

tools: fix iculslocs to support ICU 65.1 #29523

wants to merge 0 commits into from

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Sep 10, 2019

The internal ICU alias table format changed in ICU 65.1: ICU-20627

Because of this, iculslocs.cc needs to handle URES_TABLE format
contents in the res_index.txt file.

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@srl295 srl295 added tools Issues and PRs related to the tools directory. i18n-api Issues and PRs related to the i18n implementation. labels Sep 10, 2019
@srl295 srl295 self-assigned this Sep 10, 2019
@srl295
Copy link
Member Author

srl295 commented Sep 10, 2019

not super urgent but the 65.1 RC will be out soonish.

bnoordhuis
bnoordhuis approved these changes Sep 11, 2019
View reviewed changes
tools/icu/iculslocs.cc Outdated Show resolved Hide resolved
mhdawson
mhdawson approved these changes Sep 11, 2019
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 12, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@srl295
Copy link
Member Author

srl295 commented Sep 12, 2019

^ help please, i can't find the failing CI logs…

ryzokuken
ryzokuken approved these changes Sep 12, 2019
View reviewed changes
Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

Thanks!

@ryzokuken
Copy link
Contributor

@srl295 https://ci.nodejs.org/job/node-test-binary-windows-2/COMPILED_BY=vs2017,RUNNER=win2008r2-vs2017,RUN_SUBSET=1/2994/console

23:49:32 not ok 123 parallel/test-child-process-fork-exec-path
23:49:32   ---
23:49:32   duration_ms: 0.498
23:49:32   severity: fail
23:49:32   exitcode: 1
23:49:32   stack: |-
23:49:32     Can't clean tmpdir: c:\workspace\node-test-binary-windows-2\test\.tmp.123
23:49:32     Files blocking: []
23:49:32     
23:49:32     c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:136
23:49:32         throw e;
23:49:32         ^
23:49:32     
23:49:32     Error: ENOTEMPTY: directory not empty, rmdir 'c:\workspace\node-test-binary-windows-2\test\.tmp.123'
23:49:32         at Object.rmdirSync (fs.js:779:3)
23:49:32         at rmdirSync (c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:86:10)
23:49:32         at rimrafSync (c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:41:7)
23:49:32         at process.onexit (c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:121:5)
23:49:32         at process.emit (events.js:214:15) {
23:49:32       errno: -4051,
23:49:32       syscall: 'rmdir',
23:49:32       code: 'ENOTEMPTY',
23:49:32       path: 'c:\\workspace\\node-test-binary-windows-2\\test\\.tmp.123'
23:49:32     }
23:49:32   ...

@srl295
Copy link
Member Author

srl295 commented Sep 12, 2019

@srl295 https://ci.nodejs.org/job/node-test-binary-windows-2/COMPILED_BY=vs2017,RUNNER=win2008r2-vs2017,RUN_SUBSET=1/2994/console

23:49:32 Can't clean tmpdir: c:\workspace\node-test-binary-windows-2\test.tmp.123
23:49:32 Files blocking: []

should've used rimraf 😅

OK, is this a blocker or should I land this? ICU 65.1 RC is out now, so this will start to hit…

@nodejs-github-bot
Copy link
Collaborator

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

@srl295
Copy link
Member Author

srl295 commented Sep 12, 2019

Landed in fa6839e

srl295 added a commit that referenced this pull request Sep 12, 2019
@srl295
tools: fix iculslocs to support ICU 65.1
fa6839e 
The ICU alias table format changed in
https://unicode-org.atlassian.net/browse/ICU-20627

Because of this, iculslocs.cc needs to handle URES_TABLE format
contents in the res_index.txt file.

PR-URL: #29523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@srl295 srl295 closed this Sep 12, 2019
@srl295 srl295 deleted the fix-iculslocs2 branch September 12, 2019 22:31
@srl295 srl295 mentioned this pull request Sep 12, 2019
Closed
targos pushed a commit that referenced this pull request Sep 20, 2019
@srl295 @targos
tools: fix iculslocs to support ICU 65.1
e73a307 
The ICU alias table format changed in
https://unicode-org.atlassian.net/browse/ICU-20627

Because of this, iculslocs.cc needs to handle URES_TABLE format
contents in the res_index.txt file.

PR-URL: #29523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
Merged
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
@srl295 @BridgeAR
tools: fix iculslocs to support ICU 65.1
0db7ebe 
The ICU alias table format changed in
https://unicode-org.atlassian.net/browse/ICU-20627

Because of this, iculslocs.cc needs to handle URES_TABLE format
contents in the res_index.txt file.

PR-URL: #29523
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
This was referenced Sep 25, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@gengjiawen gengjiawen gengjiawen approved these changes

@richardlau richardlau richardlau approved these changes

@mhdawson mhdawson mhdawson approved these changes

@ryzokuken ryzokuken ryzokuken approved these changes

@devsnek devsnek Awaiting requested review from devsnek

Assignees

@srl295 srl295

Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@srl295 @nodejs-github-bot @ryzokuken @bnoordhuis @jasnell @addaleax @gengjiawen @richardlau @mhdawson