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: validate options passed to configure #1335

Closed

Conversation

jbergstroem
Copy link
Member

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.

@jbergstroem jbergstroem added the build Issues and PRs related to build files or the CI. label Apr 3, 2015
@jbergstroem
Copy link
Member Author

@jbergstroem
Copy link
Member Author

Looks like there's some trickery going with passing linker flags. Investigating.

Edit: Had to avoid setting libraries at all.

@jbergstroem jbergstroem force-pushed the feature/stricter_configure branch 2 times, most recently from 6620870 to 6ea8cbc Compare April 3, 2015 05:11
@mscdex
Copy link
Contributor

mscdex commented Apr 3, 2015

+1

@jbergstroem jbergstroem force-pushed the feature/stricter_configure branch from 6ea8cbc to 260b8b6 Compare April 3, 2015 06:02
@jbergstroem
Copy link
Member Author

New build at https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/438/

Hopefully I didn't screw this one up..

@rvagg
Copy link
Member

rvagg commented Apr 3, 2015

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)))
Copy link
Member

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)

Copy link
Member Author

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.

@jbergstroem
Copy link
Member Author

Holding off until #1331 lands; no point in writing logic for shared v8 flags when we're removing it. Agree?

@jbergstroem jbergstroem force-pushed the feature/stricter_configure branch from 260b8b6 to c95847f Compare April 13, 2015 05:42
@jbergstroem
Copy link
Member Author

@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 with_icu_locales (well, it already ignores it - but it won't bail for no other reason) for anything else than small-icu. For small-icu we now set it through optparse. With your agreement I'll remove this logic.

Other than that, I've simplified the help text generation and rebased onto the v8 removal branch. PTAL.

@jbergstroem
Copy link
Member Author

@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,',')

@bnoordhuis
Copy link
Member

@jbergstroem Shouldn't with_icu_locales have a default in that case? Else the line that says locs = set(with_icu_locales.split(',')) is going to error.

@jbergstroem
Copy link
Member Author

@bnoordhuis ah, sorry - should say options.with_icu_locales

@bnoordhuis
Copy link
Member

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.

@jbergstroem
Copy link
Member Author

@bnoordhuis with_icu_locales will always be set to root,en unless you override it: https://github.com/iojs/io.js/pull/1335/files#diff-e2d5a00791bce9a01f99bc6fd613a39dR259

@bnoordhuis
Copy link
Member

Right you are. Objection withdrawn.

@jbergstroem
Copy link
Member Author

@bnoordhuis good to merge?

@bnoordhuis
Copy link
Member

LGTM

jbergstroem added a commit that referenced this pull request Apr 14, 2015
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]>
@jbergstroem
Copy link
Member Author

Merged in fd90b33. Thanks for the feedback and helping me to get it in shape. Closing.

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.

4 participants