build: Use option groups in configure output #1533
Conversation
Minor edits to current build flags and its help texts as well as grouping shared and i18n options into separate option groups. Also, validate i18n default/logic similar to how we treat other options.
|
lgtm, good work |
| action='store', | ||
| dest='with_icu_source', | ||
| help='Intl mode: optional local path to icu/ dir, or path/URL of icu source archive.') | ||
|
|
||
| intl_optgroup.add_option('--download', |
There was a problem hiding this comment.
--download isn't necessarily an intl option though, although it is for now.
There was a problem hiding this comment.
Do we really want to add more options here? Guessing you're thinking about npm?
There was a problem hiding this comment.
Judging by the push-back I'd say that we may not be adding anything else and an objective would be to remove the one we have now. Although I'm personally in favour of download all the things.
There was a problem hiding this comment.
@rvagg I agree. I'd prefer removing the download thing altogether and in the intl help suggest curl -O $url -- but that's for another PR.
There was a problem hiding this comment.
Right, --download is not Intl specific and shouldn't be described as such. In any event download is off by default, please cc me on the other PR
|
@Fishrock123 I'm keen on landing this -- any additional feedback? Perhaps @srl295 might have an opinion about making intl option parsing stricter? |
|
Not really, my python-fu is not so great yet. So, lgtm if it works. |
| @@ -812,7 +829,7 @@ def configure_intl(o): | |||
| with_intl = options.with_intl | |||
| with_icu_source = options.with_icu_source | |||
| have_icu_path = bool(options.with_icu_path) | |||
| if have_icu_path and with_intl: | |||
| if have_icu_path and with_intl != 'none': | |||
|
LGTM as far as I can tell, with the comment that @jbergstroem By "strictness" I assume you meant matching |
|
@srl295 thanks for the feedback. I'll merge shortly. |
|
Merged in a5dcff8. Thanks for your reviews and comments. |
Minor edits to current build flags and its help texts as well as grouping shared and i18n options into separate option groups. Also, validate i18n default/logic similar to how we treat other options. `--download` isn't really intl-specific but is only used for that purpose which is why it's grouped similarly. PR-URL: #1533 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Steven R. Loomis <srl@icu-project.org>
Minor edits to current build flags and its help texts as well as grouping shared and i18n options into separate option groups. Also, validate i18n default/logic similar to how we treat other options. `--download` isn't really intl-specific but is only used for that purpose which is why it's grouped similarly. PR-URL: nodejs#1533 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Steven R. Loomis <srl@icu-project.org>
Minor edits to current build flags and its help texts as well as grouping shared and i18n options into separate option groups.
Also, validate i18n default/logic similar to how we treat other options.
Edit: example output
/R=@iojs/build