src: refactor configure args to config_flags

[rvagg]

Main goal here is to get --download-path to configure so we can solve the ICU problems, but while in there I decided that I was sick of the compounding variables we're using to set up the full string so I refactored.

Removed a bunch of variables and rely on %config_flags% alone where possible, also allow for an external %CONFIG_FLAGS% variable to supply additional arguments to configure to match the behaviour of the Makefile. That way we can CONFIG_FLAGS="--download-path=c:\node-icu".

PTAL @nodejs/platform-windows @nodejs/build

[jbergstroem]

Perhaps append all old variables to config_flags and pass that as a way of being backwards compatible since we don't know how many consumers we have outside of our own build systems.

[rvagg]

those variables are all initialized (emptied if they are already set) at the top of the script, not intended to be used from the outside

[jbergstroem]

I can confirm that config_flags works as intended but as per above question I might not be knowledgable enough to be the only reviewer.

[jbergstroem]

LGTM

[jasnell]

LGTM

[MylesBorins]

@jasnell LTS?

[rvagg]

@thealphanerd yes, this will have to be in LTS, if it works.

Warning @nodejs/platform-windows, imma merge this and if I messed something up because you didn't review then imma blame y'all.

[orangemocha]

Environment variables are case insensitive on Windows

[orangemocha]

..so if you want to achieve passing additional config flags through an environment var, you would have to name it something different. E.g.: additional_config_flags, or name the other one differently e.g. configure_flags, or _configure_flags

[joaocgreis]

LGTM

[jbergstroem]

Me too. LGTM.

[rvagg]

will land after 5.1.0 cause there's parts of this that can't be tested by CI but could break releases and I don't want to cause yet another release delay

https://ci.nodejs.org/job/node-test-commit/1158/

[rvagg]

I forgot about this, it was landed on 0.12 and 0.10 already. Landed just now on master @ b47d823

[MylesBorins]

@rvagg this is not patching cleanly on v4.x-staging

Would you be able to make a PR with a working patch

[MylesBorins]

hey @rvagg another ping regarding this not merging cleanly in 4.x-staging

[rvagg]

thanks @thealphanerd, merged @ 093bdcb, running a CI for v4.x-staging just to be sure @ https://ci.nodejs.org/job/node-test-commit/1577/, vcbuild.bat seems to have worked just fine on those. The discrepancy was the addition of the vtune stuff which was noise in the patch and I assume we're not backporting to v4.x.

[MylesBorins]

thanks @rvagg