build: fix build break when omitting icu

[MSLaguana]

When building without ICU (vcbuild.bat intl-none) the unicode/ucnv.h
header is not available, which caused compilation errors prior to this
change.

Checklist

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

Affected core subsystem(s)

Build

[MSLaguana]

Note that the unicode/ucnv.h header is already included in the node_i18n.cc file, gated by an appropriate ifdef.

[TimothyGu]

LGTM. Can you see if #14489 will fix build first though?

[MSLaguana]

I didn't see that PR earlier. Just had a look now, and I don't think it will fix the missing header issue that I was seeing since you don't touch any of the C files or headers. I'll try it out when I get some time.

[MSLaguana]

@TimothyGu just tried your PR, and got the same error about a missing header as I expected.

[TimothyGu]

@MSLaguana Okay, thanks for checking!

[refack]

Need to figure out how to CI this...

[refack]

CI (forced to build without a273b03) https://ci.nodejs.org/job/node-test-commit/11483/

[refack]

@MSLaguana this might be incomplete, see CI results:

../src/inspector_io.cc:14:28: fatal error: unicode/unistr.h: No such file or directory
 #include <unicode/unistr.h>

src\inspector_io.cc(14): fatal error C1083: Cannot open include file: 'unicode/unistr.h': No such file or directory [c:\workspace\node-compile-windows\label\win-vs2017\node.vcxproj]

[refack]

Might be incomplete

[MSLaguana]

Looks like you are right @refack, wonder why it built successfully for me without changes to that file too. I'll make sure I get the same error, then fix it as well.

[kfarnung]

@refack I don't think your change to disable intl was complete. Inspector depends on it and checks options.with_intl to figure that out:

def configure_inspector(o):
  disable_inspector = (options.without_inspector or
                       options.with_intl in (None, 'none') or
                       options.without_ssl)
  o['variables']['v8_enable_inspector'] = 0 if disable_inspector else 1

Maybe you can override options.with_intl instead?

[MSLaguana]

Here's a CI run where I've overridden options.with_intl as @kfarnung suggested: https://ci.nodejs.org/job/node-test-commit/11488/ using commit MSLaguana@ea44d36

[refack]

Well it builds, next get the tests to pass ( @MSLaguana I mean in a diffirent future PR )

[MSLaguana]

I think that #14489 will help with the next failures, e.g. lint failing due to a lack of ICU should be fixed by that.

[refack]

Extra check of just this commit: https://ci.nodejs.org/job/node-test-commit-linuxone/7676/

[refack]

Landed in 1782b38