1247836 - Building blocks for a python configure

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

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.

Comment 1

[Mike Hommey [:glandium]]

Attachment: WIP

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.

Comment 2

[Mike Hommey [:glandium]]

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

Comment 3

[Mike Hommey [:glandium]]

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

Comment 4

[Mike Hommey [:glandium]]

Attachment: Demo

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.

Comment 5

[Mike Hommey [:glandium]]

Attachment: WIP

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

Comment 6

[Mike Hommey [:glandium]]

Attachment: Demo

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

Comment 7

[Mike Hommey [:glandium]]

Attachment: WIP

Comment 8

[Mike Hommey [:glandium]]

Attachment: Demo

Comment 9

[Mike Hommey [:glandium]]

(Refreshed with my last changes)

Comment 10

[Mike Hommey [:glandium]]

Attachment: part 1 - Add classes for python configure options

Comment 11

[Mike Hommey [:glandium]]

Attachment: part 2 - WIP

Comment 12

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1247836 - Add a ReadOnlyNamespace class to mozbuild.util

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

Comment 13

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1247836 - Add classes for python configure options handling

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

Comment 14

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1247836 - Building blocks for python configure

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

Comment 15

[Mike Hommey [:glandium]]

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 16

[Mike Hommey [:glandium]]

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 17

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8726090 [details]
MozReview Request: Bug 1247836 - Add a ReadOnlyNamespace class to mozbuild.util

https://reviewboard.mozilla.org/r/37801/#review34485

Comment 18

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8726090 [details]
MozReview Request: Bug 1247836 - Add a ReadOnlyNamespace class to mozbuild.util

https://reviewboard.mozilla.org/r/37801/#review34577

Comment 19

[Nick Alexander :nalexander [he/him]]

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 20

[Chris Manchester (limited bugmail, email directly)]

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 21

[Nick Alexander :nalexander [he/him]]

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.

Comment 22

[Mike Hommey [:glandium]]

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.

Comment 23

[Mike Hommey [:glandium]]

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.

Comment 24

[Nick Alexander :nalexander [he/him]]

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 25

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 26

[Mike Hommey [:glandium]]

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.

Comment 27

[Mike Hommey [:glandium]]

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)

Comment 28

[Mike Hommey [:glandium]]

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.

Comment 29

[Mike Hommey [:glandium]]

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.

Comment 30

[Mike Hommey [:glandium]]

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

Comment 31

[Pulsebot]

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

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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