-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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: validate options passed to configure #1335
Conversation
Looks like there's some trickery going with passing linker flags. Investigating. Edit: Had to avoid setting libraries at all. |
6620870
to
6ea8cbc
Compare
+1 |
6ea8cbc
to
260b8b6
Compare
New build at https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/438/ Hopefully I didn't screw this one up.. |
love it, lgtm with squash |
'Valid values are: arm, arm64, ia32, mips, mipsel, x32, x64') | ||
choices=valid_arch, | ||
help='CPU architecture to build for ({0})'.format(', '.join( | ||
str(i) for i in valid_arch))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can shorten this to:
'CPU architecture to build for ({0})'.format(', '.join(valid_arch))
Or if you don't care about python 3 compat:
'CPU architecture to build for (%s)' % ', '.join(valid_arch)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. My Python is weak.
Holding off until #1331 lands; no point in writing logic for shared v8 flags when we're removing it. Agree? |
Validate some of the arguments passed to configurabel options. Also, move defaults to optparse default.
260b8b6
to
c95847f
Compare
@bnoordhuis: I've looked closer at the icu branching. I'm still keen on removing the check altogether. The issue is that help text says that root,en is default - but it isn't set unless small-icu is in use. Removing the check will just make the build ignore Other than that, I've simplified the help text generation and rebased onto the v8 removal branch. PTAL. |
@bnoordhuis this is what I have in mind: diff --git configure configure
index 1d0e65a..148b208 100755
--- configure
+++ configure
@@ -787,13 +787,7 @@ def configure_intl(o):
return
# --with-intl=<with_intl>
# set the default
- if with_intl is None:
- with_intl = 'none' # The default mode of Intl
- # sanity check localelist
- if options.with_icu_locales and (with_intl != 'small-icu'):
- print 'Error: --with-icu-locales only makes sense with --with-intl=small-icu'
- sys.exit(1)
- if with_intl == 'none' or with_intl is None:
+ if with_intl in (None, 'none'):
o['variables']['v8_enable_i18n_support'] = 0
return # no Intl
elif with_intl == 'small-icu':
@@ -801,8 +795,6 @@ def configure_intl(o):
o['variables']['v8_enable_i18n_support'] = 1
o['variables']['icu_small'] = b(True)
with_icu_locales = options.with_icu_locales
- if not with_icu_locales:
- with_icu_locales = 'root,en'
locs = set(with_icu_locales.split(','))
locs.add('root') # must have root
o['variables']['icu_locales'] = string.join(locs,',') |
@jbergstroem Shouldn't with_icu_locales have a default in that case? Else the line that says |
@bnoordhuis ah, sorry - should say |
What I mean is, with_icu_locales or options.with_icu_locales is going to be None, so calling .split() on it won't work. |
@bnoordhuis |
Right you are. Objection withdrawn. |
@bnoordhuis good to merge? |
LGTM |
Some variables like dest arch or os are now validated to avoid build issues. Move defaults to optparse default while at it. PR-URL: #1335 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Merged in fd90b33. Thanks for the feedback and helping me to get it in shape. Closing. |
Validate some of the arguments passed to configurable options. Also, move defaults to optparse default.
I can't really vouch for options; just pulled them from comments. I've tested a few permutations, but not all.