This bug is about importing the necessary code to handle the proposed testcase from https://pastebin.mozilla.org/8859427 (or variations of it). This will add the proper modules but won't take care or hooking with the build system.

This is my work in progress. It is not review quality, but allows to start doing things. There is some minor refactoring to do, a few TODO items, and some moving around. I changed things since the pastebin, which allowed to make the sandbox code simpler than it was.

Oh, forgot to mention, the WIP lacks actual support for --help (but I have a plan for that), and it lacks user-friendly error messages (they're, for most of them, assertions).

Note the testcase is synthetic and unrealistic, but it has comments on the basic features available in the sandbox.

This adds a configure.py script at the top level that makes the sandbox code from the WIP read the top-level moz.configure file. All the configure.py script does for now is handle the command line/environment variables according to moz.configure, and outputs the yielded config items to stdout. Oh, and if several @depends functions yield the same config key, the last one wins, for now. I haven't implemented aggregation yet. I don't want to go much further on the configure.py script itself until we know how far we can go with a conversion in a short time frame (like, the work week), and discuss what the migration plan is.

Progress, now with rudimentarily formatted (dynamic) help, config aggregation, and a few changes around giving functions access to all builtins.

I actually started converting actual things from configure.in. There's a full list of all the 215 flags in build/moz.configure/flags.configure, extracted from configure.in and js/src/configure.in with m4. This also handles bootstrapping the virtualenv, but as noted in the comment there, some should move to VirtualenvManager. There's certainly a lot of bikeshedding to do. Help output, at the moment, looks like this: $ ../configure.py --help Usage: configure.py [options] Options: [defaults in brackets after descriptions] --help print this message --enable-artifact-builds Download and use prebuilt binary artifacts. --disable-compile-environment Disable compiler/library checks --host --target --enable-build-backend={AndroidEclipse,CppEclipse,ChromeMap,RecursiveMake,CompileDB,FasterMake+RecursiveMake,FasterMake,VisualStudio} Enable additional build backends --with-l10n-base path to l10n repositories Environment variables: PYTHON AWK HOST_CC CC PERL MOZTTDIR And for the dynamic part: $ ../configure.py --help --enable-artifact-builds Usage: configure.py [options] Options: [defaults in brackets after descriptions] --help print this message --enable-artifact-builds Download and use prebuilt binary artifacts. --disable-compile-environment Disable compiler/library checks --enable-build-backend={AndroidEclipse,CppEclipse,ChromeMap,RecursiveMake,CompileDB,FasterMake+RecursiveMake,FasterMake,VisualStudio} Enable additional build backends --with-l10n-base path to l10n repositories Environment variables: PYTHON AWK PERL MOZTTDIR

(Refreshed with my last changes)

Review commit: https://reviewboard.mozilla.org/r/37801/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37801/

Review commit: https://reviewboard.mozilla.org/r/37797/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37797/

Review commit: https://reviewboard.mozilla.org/r/37799/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37799/

https://reviewboard.mozilla.org/r/37799/#review34343 ::: python/mozbuild/mozbuild/configure/__init__.py:107 (Diff revision 1) > + relpath=os.path.relpath, Turns out I'll need to add realpath too.

Comment on attachment 8726092 [details] MozReview Request: Bug 1247836 - Building blocks for python configure Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37799/diff/1-2/

Comment on attachment 8726090 [details] MozReview Request: Bug 1247836 - Add a ReadOnlyNamespace class to mozbuild.util https://reviewboard.mozilla.org/r/37801/#review34485

Comment on attachment 8726090 [details] MozReview Request: Bug 1247836 - Add a ReadOnlyNamespace class to mozbuild.util https://reviewboard.mozilla.org/r/37801/#review34577

Comment on attachment 8726091 [details] MozReview Request: Bug 1247836 - Add classes for python configure options handling https://reviewboard.mozilla.org/r/37797/#review34491 I have some questions, and some naming nits, but nothing substantive. Forward! ::: python/mozbuild/mozbuild/configure/options.py:23 (Diff revision 1) > + The `origin` attribute holds where the option comes from (e.g. environment, Quote the options: 'environment', 'command-line', 'default'. Is it worth ensuring the origin is in that set? ::: python/mozbuild/mozbuild/configure/options.py:90 (Diff revision 1) > +class InvalidOption(Exception): This looks a lot like a regular class. `InvalidOptionError`? ::: python/mozbuild/mozbuild/configure/options.py:140 (Diff revision 1) > + if not env.isupper(): Check for empty? Ah, it's kinda below. ::: python/mozbuild/mozbuild/configure/options.py:165 (Diff revision 1) > + # --disable and --without options mean the default is enabled. Wow, configure is special. ::: python/mozbuild/mozbuild/configure/options.py:206 (Diff revision 1) > + if isinstance(self.default, PositiveOptionValue): This whole effort is going to avoid so many bugs. ::: python/mozbuild/mozbuild/configure/options.py:227 (Diff revision 1) > + def split_option(option): Can we get a docstring here? ::: python/mozbuild/mozbuild/configure/options.py:285 (Diff revision 1) > + return 1 if self.nargs == '?' else -1 Why not `maxargs = sys.maxint` or similar? ::: python/mozbuild/mozbuild/configure/options.py:346 (Diff revision 1) > + command line, or as FOO=1 in the environment *or* on the command line. I wasn't aware of this command line approach. Is this, `configure --foo FOO=1`? That's new to me. ::: python/mozbuild/mozbuild/configure/options.py:358 (Diff revision 1) > + self._args = OrderedDict() `args` and `extra_args` isn't particularly descriptive. Consider `args_from_*` or something to make the split clearer. ::: python/mozbuild/mozbuild/configure/options.py:378 (Diff revision 1) > + self._extra_args[name] = arg, -1 - len(self._extra_args) This negatation and subsequent `abs` is confusing. What is it you're trying to achieve? ::: python/mozbuild/mozbuild/configure/options.py:401 (Diff revision 1) > + elif option.env and args is self._args: The interplay between `_prepare`, `handle`, `self._args`, and `self._extra_args` is pretty complex. I don't want to block on style, but consider if this can be simplified. ::: python/mozbuild/mozbuild/test/configure/test_options.py:98 (Diff revision 1) > + # Those need defaults nit: These. Full sentences (through-out). ::: python/mozbuild/mozbuild/test/configure/test_options.py:576 (Diff revision 1) > + helper.add('FOO=a,b,c', 'implied') What does 'implied' mean in this context? It's not refered to in the main patch.

Comment on attachment 8726091 [details] MozReview Request: Bug 1247836 - Add classes for python configure options handling https://reviewboard.mozilla.org/r/37797/#review34587 Just nits here. Sorry if they overlap with Nick's, I see he just posted a review. ::: python/mozbuild/mozbuild/configure/options.py:143 (Diff revision 1) > + if not name and not env: > + raise InvalidOption('At least an option name or an environment ' > + 'variable name must be given') This would be a little easier to read if this check came at the beginning. ::: python/mozbuild/mozbuild/configure/options.py:150 (Diff revision 1) > + if (not isinstance(default, types.StringTypes) and > + not isinstance(default, (bool, types.NoneType)) and `not isinstance(default, (bool, types.StringTypes, types.NoneType))` would save one "and". ::: python/mozbuild/mozbuild/configure/options.py:214 (Diff revision 1) > + raise InvalidOption( > + 'The `default` value must be one of the `choices`') Might be helpful to include the choices in the error message. ::: python/mozbuild/mozbuild/configure/options.py:252 (Diff revision 1) > + @staticmethod > + def _join_option(prefix, name, values=()): I don't see this method being called with a value for `values`, is the parameter needed? ::: python/mozbuild/mozbuild/configure/options.py:360 (Diff revision 1) > + self._origins = {} Consider re-naming this to reflect that it's only relevant to extra args. ::: python/mozbuild/mozbuild/configure/options.py:378 (Diff revision 1) > + self._extra_args[name] = arg, -1 - len(self._extra_args) I don't see where making these negative impacts the output. ::: python/mozbuild/mozbuild/configure/options.py:382 (Diff revision 1) > + arg, pos = None, 0 `pos` is only significant in the `if from_name and from_env:` branch, so no need to set it here. ::: python/mozbuild/mozbuild/test/configure/test_options.py:388 (Diff revision 1) > + value = env_option.get_value('--foo') No need to assign to this.

Comment on attachment 8726092 [details] MozReview Request: Bug 1247836 - Building blocks for python configure https://reviewboard.mozilla.org/r/37799/#review34739 I have one big concern: we're deeply entangling the command line and mozconfig given and the evaluation of the configure definitions. That's different from moz.build, which tried hard to be declarative; here we declare lots of non-declarative things which consume the command line and environment ad-hoc. However, I'm not going to block on that, since this is such a huge win over our current situation. I have some nits and questions, but nothing more. ::: python/mozbuild/mozbuild/configure/__init__.py:54 (Diff revision 2) > + self.implied_options.append((option, reason)) Consider not allowing duplicate definitions. ::: python/mozbuild/mozbuild/configure/__init__.py:73 (Diff revision 2) > + `option` and `include` are functions. `depends`, `template` and `advanced` Consider adding a note about the `_impl` magic here. ::: python/mozbuild/mozbuild/configure/__init__.py:77 (Diff revision 2) > + simple as possible. Instead, helpers shall be created within the sandbox nit: s/shall/should/. Or just make it imperative: "Instead, create helpers ..." ::: python/mozbuild/mozbuild/configure/__init__.py:129 (Diff revision 2) > + self._implied_options = {} Annotate this too. ::: python/mozbuild/mozbuild/configure/__init__.py:139 (Diff revision 2) > + if self._db[self._help_option]: When will this be true? You set `self._db = {}` above. ::: python/mozbuild/mozbuild/configure/__init__.py:188 (Diff revision 2) > + 'Option `%s` is not referenced by any @depends' Consider making this more explanatory: "Option FOO is not handled -- reference it with an @depends" ::: python/mozbuild/mozbuild/configure/__init__.py:208 (Diff revision 2) > + if (not isinstance(value, DummyFunction) and This means one can't set a top-level temporary variable, like local_version = "23" or similar, right? ::: python/mozbuild/mozbuild/configure/__init__.py:287 (Diff revision 2) > + declares new configuration items. What does "configuration items" mean? Do you mean changes the set of declared configure options? All of your examples are of the form `setconfig('ENV', value)`. Can you `setconfig('--enable-FOO')`? `setconfig('--with=FOO')`? ::: python/mozbuild/mozbuild/configure/__init__.py:373 (Diff revision 2) > + what = self._resolve(what) Do you want this call to `_resolve`? `_resolve` passes through non-functions. What does it mean to `include` a function? ::: python/mozbuild/mozbuild/test/configure/data/moz.configure:172 (Diff revision 2) > + if len(value) and value[0] == 'break': I think I would have preferred this conditional to have been done in separate test files, for clarity.

https://reviewboard.mozilla.org/r/37797/#review34587 > `not isinstance(default, (bool, types.StringTypes, types.NoneType))` would save one "and". You'd think so, but types.StringTypes is a tuple. > I don't see this method being called with a value for `values`, is the parameter needed? It would appear it's not needed.

https://reviewboard.mozilla.org/r/37799/#review34739 > Consider not allowing duplicate definitions. I thought about this, and punted to not do it because we're likely to have multiple options implying the same thing. In fact, I know we do: --enable-dmd implies --enable-profiling, and so does --enable-jprof, --enable-instruments, --enable-callgrind and --enable-vtune. > When will this be true? You set `self._db = {}` above. It might be confusing, but calling self.option_impl will set _db[self._help_option] if --help was on the command line. > This means one can't set a top-level temporary variable, like > > local_version = "23" > > or similar, right? right > What does "configuration items" mean? Do you mean changes the set of declared configure options? > > All of your examples are of the form `setconfig('ENV', value)`. Can you `setconfig('--enable-FOO')`? `setconfig('--with=FOO')`? no, that would be imply_option('--enable-foo') > Do you want this call to `_resolve`? `_resolve` passes through non-functions. What does it mean to `include` a function? It means you can use the result of a @depends function as the file name to include. That is of limited use. It's only really meant to be used for --enable-application, although it might also be used for the toolchain. > I think I would have preferred this conditional to have been done in separate test files, for clarity. Not sure what you mean.

https://reviewboard.mozilla.org/r/37799/#review34739 > Not sure what you mean. You could have `testbad.configure` which throws, and `testgood.configure` which doesn't. Lots of smaller test files, for different tests.

Comment on attachment 8726092 [details] MozReview Request: Bug 1247836 - Building blocks for python configure https://reviewboard.mozilla.org/r/37799/#review34795 Just nits. The implementation is a lot easier to understand than I feared. ::: python/mozbuild/mozbuild/configure/__init__.py:57 (Diff revision 2) > +def forbidden_import(*args, **kwargs): > + raise ImportError('Importing modules is forbidden') We could advise people to use @advanced if necessary in this error message. ::: python/mozbuild/mozbuild/configure/__init__.py:133 (Diff revision 2) > + self._config, self._stdout, self._stderr = config, stdout, stderr I don't see the self.\_stdout or self.\_stderr attributes being written to anywhere in this class, what's their intended use? ::: python/mozbuild/mozbuild/configure/__init__.py:232 (Diff revision 2) > + resolved arguments (use the result of functions when functions are > + passed). In most cases, the result of this function is not expected to This should be "(uses the result..." if I understand this comment.

https://reviewboard.mozilla.org/r/37797/#review34491 > Quote the options: 'environment', 'command-line', 'default'. > > Is it worth ensuring the origin is in that set? I'm not quite sure about that yet. There are also 'implied' and 'mozconfig' in subsequent patches. > Wow, configure is special. We could change that, but I find it convenient: if the option is --disable-foo, it means foo is enabled by default, and that you can optionally disable it, right? > I wasn't aware of this command line approach. Is this, `configure --foo FOO=1`? That's new to me. That actually doesn't work with autoconf 2.13, but does with newer autoconf. It's also very convenient because it would allow us to phase out non ac_add_options/mk_add_options lines in mozconfig, and solve most problems of "where do I set $random variable in my mozconfig?" > `args` and `extra_args` isn't particularly descriptive. Consider `args_from_*` or something to make the split clearer. I'll leave this to a followup. > This negatation and subsequent `abs` is confusing. What is it you're trying to achieve? How does it look in bug 1253203? > What does 'implied' mean in this context? It's not refered to in the main patch. In the test it means nothing. It's a value like any other.

https://reviewboard.mozilla.org/r/37797/#review34587 > Consider re-naming this to reflect that it's only relevant to extra args. I'm going to move the changes to this class from bug 1253203 here, so this comment doesn't apply anymore. I'll leave other comments regarding this class (mostly about _args and _extra_args being confusing and the overall code from this class being complex to a followup)

https://reviewboard.mozilla.org/r/37799/#review34739 > You could have `testbad.configure` which throws, and `testgood.configure` which doesn't. Lots of smaller test files, for different tests. I want to add more bad tests, I'll file a followup for that.

https://reviewboard.mozilla.org/r/37799/#review34795 > We could advise people to use @advanced if necessary in this error message. I'd rather not. @avanced should really be a last resort thing, and I'd rather people come talk to us than have them blindly add @advanced and be done with it. > I don't see the self.\_stdout or self.\_stderr attributes being written to anywhere in this class, what's their intended use? _stdout is used for _help.usage. _stderr is not used, though, but let's keep if for now, for consistency.

(In reply to Mike Hommey [:glandium] from comment #26) > > `args` and `extra_args` isn't particularly descriptive. Consider `args_from_*` or something to make the split clearer. > > I'll leave this to a followup. This really is the only thing I added, yet mozreview reposted all the previous comments. sigh.

https://hg.mozilla.org/integration/mozilla-inbound/rev/246ed92c974a https://hg.mozilla.org/integration/mozilla-inbound/rev/04b853074860 https://hg.mozilla.org/integration/mozilla-inbound/rev/85ead00f0a39

https://hg.mozilla.org/mozilla-central/rev/246ed92c974a https://hg.mozilla.org/mozilla-central/rev/04b853074860 https://hg.mozilla.org/mozilla-central/rev/85ead00f0a39