Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: Use option groups in configure output #1533

Closed

Conversation

jbergstroem
Copy link
Member

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

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.
@mscdex mscdex added the build Issues and PRs related to build files or the CI. label Apr 27, 2015
@rvagg
Copy link
Member

rvagg commented Apr 27, 2015

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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--download isn't necessarily an intl option though, although it is for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to add more options here? Guessing you're thinking about npm?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jbergstroem
Copy link
Member Author

@Fishrock123 I'm keen on landing this -- any additional feedback?

Perhaps @srl295 might have an opinion about making intl option parsing stricter?

@Fishrock123
Copy link
Contributor

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':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

@srl295
Copy link
Member

srl295 commented Apr 30, 2015

LGTM as far as I can tell, with the comment that --download should not be Intl specific.

@jbergstroem By "strictness" I assume you meant matching --with-intl to a specific list? Seems fine to me.

@jbergstroem
Copy link
Member Author

@srl295 thanks for the feedback. I'll merge shortly.

@jbergstroem
Copy link
Member Author

Merged in a5dcff8. Thanks for your reviews and comments.

@jbergstroem jbergstroem closed this May 1, 2015
jbergstroem added a commit that referenced this pull request May 1, 2015
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>
@rvagg rvagg mentioned this pull request May 2, 2015
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 19, 2015
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants