src: fix without-intl build #6820
Conversation
In 5d38d54, an additional property in node_config.cc was added whose definition depends on having the local `env` variable declared, which in turn depended on `NODE_HAVE_I18N_SUPPORT` being defined. Moving `env = ...` out of the `#ifdef` block allows building via `./configure --without-intl` again.
addaleax
added
c++
|
ha! good catch... completely forgot that the env had been moved inside the ifdef. |
|
LGTM We just shipped v6.2.0 with intl included If this is broken it might not be possible for individuals to build node from our tag without INTL... @mhart noticed a 20% increase in binary size with v6.2.0 even with one more ci run because I'm assuming bsd + smaros + windows failures are infra, don't feel like digging atm. https://ci.nodejs.org/job/node-test-pull-request/2682/ If it is green and there are no objects it would be nice to land this asap... at the very least we can have a reviewed patch people can apply for their custom builds (this doesn't really affect people downloading our precompiled assets) |
|
Yeah, so digging in a little more, the jump with The jump in a regular build was less pronounced (4.4MB), but still kinda large: mhart/alpine-node@0493edb#diff-04c6e90faac2675aa89e2176d2eec7d8L11 – but I haven't checked yet how much of that was going from npm v3.8.9 to v3.9.0 in the image too |
|
Huh, npm v3.9.0 (with deps) must have actually dropped quite a bit in size from v3.8.9 – so the difference between node v6.1 and v6.2 is larger than I thought, regardless of whether you build it statically or not. On Alpine, v6.1.0 w/ npm v3.9.0 is 39.63 MB, v6.2.0 w/ npm v3.9.0 is 45.99MB – so that's a 6.4MB difference, (not 4.4MB as I reported above) Definitely a pretty big bump in any case |
|
LGTM. I filed nodejs/build#419 for getting no-i18n test coverage. |
In 5d38d54, an additional property in node_config.cc was added whose definition depends on having the local `env` variable declared, which in turn depended on `NODE_HAVE_I18N_SUPPORT` being defined. Moving `env = ...` out of the `#ifdef` block allows building via `./configure --without-intl` again. PR-URL: nodejs#6820 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
Landed in 178e634 |
|
(FWIW this hasn't changed the size of a |
|
@mhart thanks for the heads up! I think you should likely open another issue about the file size so we can dig in and figure out where it came from |
|
@thealphanerd done: #6860 |
|
Any chance for quick minor release with this PR? Without this OpenEmbedded can't build Node.js v6.2.0 non-icu packages. |
] Closes #41 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
In 5d38d54, an additional property in node_config.cc was added whose definition depends on having the local `env` variable declared, which in turn depended on `NODE_HAVE_I18N_SUPPORT` being defined. Moving `env = ...` out of the `#ifdef` block allows building via `./configure --without-intl` again. PR-URL: #6820 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
In 5d38d54, an additional property in node_config.cc was added whose definition depends on having the local `env` variable declared, which in turn depended on `NODE_HAVE_I18N_SUPPORT` being defined. Moving `env = ...` out of the `#ifdef` block allows building via `./configure --without-intl` again. PR-URL: #6820 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Checklist
Affected core subsystem(s)
src
Description of change
In 5d38d54, an additional property in node_config.cc was added whose definition depends on having the local
envvariable declared, which in turn depended onNODE_HAVE_I18N_SUPPORTbeing defined.Moving
env = ...out of the#ifdefblock allows building via./configure --without-intlagain./cc @jasnell