build: include ICU by default for win dll option #17547
Conversation
nodejs-github-bot
added
build
|
cc/ @nodejs/platform-windows thoughts? This looks like an oversight to me, but maybe I'm missing something. |
|
I double checked, and |
|
It looks like currently Perhaps |
|
|
|
@richardlau agreed, but that could be a separate PR |
There was a problem hiding this comment.
Spoke to @BethGriggs offline and the issue being seen with tests with the shared library on Windows was on Node.js v4.x. The default value of --without-intl in v4.x is none (https://github.com/nodejs/node/blob/v4.8.7/configure#L325) whereas in v6.x and later it is small-icu (https://github.com/nodejs/node/blob/v6.12.2/configure#L363). So this is only an issue for v4.x and is not needed for later versions.
|
What is the conclusion here? |
|
I think the PR needs to be retargeted at Node 4.x, but I'll have to check with @BethGriggs . |
BethGriggs
force-pushed
the
include-icu-win-shared-lib
branch
2 times, most recently
from
0942069
to
47fbe06
Compare
BethGriggs
force-pushed
the
include-icu-win-shared-lib
branch
from
47fbe06
to
920699c
Compare
There was a problem hiding this comment.
This PR has been retargetted at v4.x since the discrepancy between whether or not ICU is enabled exists in that release line (since in v4.x configure defaults intl to none) but not on later release lines (i.e. v6.x, etc where configure defaults intl to small-icu).
We (IBM) intend to float this patch for our own builds, but it would be nice if we didn't have to, hence this PR for v4.x.
Cc @nodejs/lts
vcbuild.bat
Outdated
| @@ -68,7 +68,7 @@ if /i "%1"=="test-pummel" set test_args=%test_args% pummel&goto arg-ok | |||
| if /i "%1"=="test-all" set test_args=%test_args% sequential parallel message gc internet pummel&set buildnodeweak=1&set jslint=1&goto arg-ok | |||
| if /i "%1"=="test-known-issues" set test_args=%test_args% known_issues --expect-fail&goto arg-ok | |||
| if /i "%1"=="jslint" set jslint=1&goto arg-ok | |||
| if /i "%1"=="package" set package=1&goto arg-ok | |||
| if /i "%1"=="package" set package=1&set i18n_arg=small-icu&goto arg-ok | |||
There was a problem hiding this comment.
Please could you also set download_arg="--download=all" (also for the dll option) as v4.x does not contain an in-tree copy of ICU.
There was a problem hiding this comment.
originally it was desired to not automatically download without the user opting in
There was a problem hiding this comment.
Fair point. What happens though if small-icu is used where there is no local ICU?
There was a problem hiding this comment.
To answer my own question:
Looking for Visual Studio 2015
Found Visual Studio 2015
configure --with-intl=small-icu --dest-cpu=x64 --tag=
ctrpp not found in WinSDK path--using pre-gen files from tools/msvs/genfiles.
creating icu_config.gypi
* ECMA-402 (Intl) support didn't find ICU in deps\icu..
Warning: Not downloading package "icu". You could pass "--download=all"
(Windows: "download-all") to try auto-downloading it.
Cannot build Intl without ICU in deps\icu.
(Fix, or disable with "--with-intl=none" )
Failed to create vc project files.
So for v4.x it doesn't seem to make sense to specify set i18n_arg=small-icu without also specifying set download_arg="--download=all". Note this is currently done for the msi option and I think package and dll should do the same for consistency:
https://github.com/nodejs/node/blob/920699c5053447ef5888243bdff67b7f8effbeb9/vcbuild.bat#L72
BethGriggs
force-pushed
the
include-icu-win-shared-lib
branch
from
920699c
to
f216e15
Compare
Change the Windows .dll builds to include small-icu by default for parity with the regular and msi build options.
BethGriggs
force-pushed
the
include-icu-win-shared-lib
branch
from
f216e15
to
e9afdae
Compare
|
I don't think we plan to do another 4.x release that isn't security related. Does it make sense to keep this open? |
I'm undecided. On the one hand I agree that we aren't likely to do another 4.x release. On the other, if we do do a release, then we would miss this once it's closed. If you're 90% sure we won't do another non-security release, then yeah, let's close. |
|
@gibfahn I'm 99% sure there is no reason to do another non security release |
|
Closing due to the mentioned concerns about getting this landed. In case surprisingly there will be another release I guess it will also be possible to remember this and reopen and land it. @BethGriggs thanks a lot for your contribution anyway! It is much appreciated. |
Change the Windows .dll builds to include small-icu by default for
parity with the regular and msi build.
I noticed this while running the tests against the windows shared library.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
build