doc: document and warn if the ICU version is too old#23766
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added build 
addaleax commented Oct 20, 2018
addaleax added the author ready 
configure.py Outdated
There was a problem hiding this comment.
may? what can happen if it is? why not go all out and error?
(escape hatch is to manually edit config.gypi)
There was a problem hiding this comment.
probably a compile error. which they will see soon enough… ±0 on making it an error, either way.
refack left a comment
refack left a comment There was a problem hiding this comment.
Good concept. Left two comments.
configure.py Outdated
There was a problem hiding this comment.
There was a problem hiding this comment.
@refack hm. Doesn't make sense with the format of current_ver.dep … the minimum ICU is global, doesn't need to be repeated multiple times.
There was a problem hiding this comment.
@refack I split the minimum version to a separate file.
There was a problem hiding this comment.
We could change the format of tools/icu/current_ver.dep to be:
{"current_minimum": 57, "packages": [ ... current content ... ] }
refack commented Oct 23, 2018
CI (just in case you don't wanna patch this any more): https://ci.nodejs.org/job/node-test-pull-request/18060/ |
refack removed the author ready 
srl295 commented Oct 23, 2018
@refack how about moving the contents to the new .json file in a future PR once these land? (this and the 2 other PRs) |
Fixes: #19657 PR-URL: #23766 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>
Fixes: #19657 PR-URL: #23766 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>
MylesBorins commented Nov 26, 2018
This lands cleanly on 10.x, should we backport to 8.x? |
Fixes: #19657 PR-URL: #23766 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>
Fixes: #19657 PR-URL: #23766 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>
8 participants
Fixes: #19657
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesExample: