808357 - Implement mozconfig finding and loading in Python

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

Attachment: Part 1: Move mozconfig code into own Python module, v1

I'm trying to marginalize mozbuild.base.MozbuildObject. IMO the mozconfig code deserves its own module. The first patch refactors it into one.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Part 2: Implement mozconfig finding and loading in Python, v1

I'm not sure why I wrote this patch. But, I did. We can now perform mozconfig finding and loading from Python, complete with parsing of mozconfig execution results.

This /could/ make mozconfig2client-mk obsolete. It can't make mozconfig2configure obsolete because that script executes in the same shell as configure and configure relies on local/non-exported variables in mozconfigs. I would like to see us move to a world where mozconfigs can execute in their own shells and all commands with intended side effects are either explicit "export VAR = FOO" or (even better) are function calls (like mk_add_options). That way we can have an API for mozconfigs and can do things like validate that the output actually does something. But, I'm not asking for that in this bug: I'm just saying it is possible and this patch would lay the foundation for that. Who knows, maybe when building with mach/mozbuild we go through a subshell while simultaneously support the wild-west on the old build system.

I'm not asking for review of this patch because it is incomplete. There are likely many bugs and it could use a lot of testing love. I haven't ported shell escaping yet, so yeah... I held off putting more work into it because I want to see what the reception is first. I fully anticipate f- from Mike Hommey. I won't be disappointed if I get that. That being said, I would like to remove the dependency on pymake from part 1. So, if you have any ideas for direction, I'd love to know what you are comfortable with.

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 678083 [details] [diff] [review]
Part 1: Move mozconfig code into own Python module, v1

Review of attachment 678083 [details] [diff] [review]:
-----------------------------------------------------------------

Could you hg cp base.py loader.py, so that history of the moved lines is not lost?

::: python/mozbuild/mozbuild/base.py
@@ +52,5 @@
> +        if self._mozconfig_result['topobjdir'] is not None:
> +            self._topobjdir = self._mozconfig_result['topobjdir']
> +        else:
> +            assert self._mozconfig_result['config_guess']
> +            self._topobjdir = 'obj-%s' % self._mozconfig_result['config_guess']

You're effectively going to overwrite self._topobjdir with the same value at each call. You should just return self._topobjdir if it's set.

@@ +82,5 @@
>              return
>  
>          os.makedirs(path)
>  
> +    def _ensure_mozconfig_loaded(self):

Why not do an @property like for topobjdir?

::: python/mozbuild/mozbuild/mozconfig/loader.py
@@ +40,5 @@
> +    def read_mozconfig(self, path=None):
> +        env = dict(os.environ)
> +        if path is not None:
> +            # Python < 2.7.3 doesn't like unicode strings in env keys.
> +            env[b'MOZCONFIG'] = path

Why the change from str() to b'' ?

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 678084 [details] [diff] [review]
Part 2: Implement mozconfig finding and loading in Python, v1

Review of attachment 678084 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/mozconfig/loader
@@ +33,5 @@
> +echo "BEGIN_BEFORE_ENV"
> +env
> +echo "END_BEFORE_ENV"
> +
> +. $1

You may want to add the output from "set" in there, so that you can detect new unexported variables, which do have an effect on configure.

::: python/mozbuild/mozbuild/mozconfig/loader.py
@@ +274,5 @@
> +
> +            if line == 'END_AFTER_ENV':
> +                assert current_type == 'env_after'
> +                current_type = None
> +                continue

Seems like a lot of boilerplate. It seems it would be easier to just use what is after BEGIN_ and END_ as current_type.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Part 1: Move mozconfig code into own Python module, v2

Moved things around a bit and incorporated review feedback.

The use of b'' instead of inline unicode (because unicode_literals is imported) is to work around a Python 2.7.2 and below bug where os.environ won't accept unicode, just str. It's really annoying. I'm tempted to monkeypatch os.environ in our virtualenv to work around it...

Comment 5

[Gregory Szorc [:gps]]

Attachment: Part 2: Implement mozconfig finding and loading in Python, v2

Now using the |set| shell built-in for variable gathering. Since we are using Bourne shell, the format is even easy to parse (Bourne just prints variables whereas bash prints functions).

I also added a load of tests for this. I'm pretty happy with the state of the code. Of course, nothing is using it yet, but that will change in the future...

I've even tests this on Windows and it passes the unit tests!

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 687484 [details] [diff] [review]
Part 1: Move mozconfig code into own Python module, v2

Review of attachment 687484 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/base.py
@@ +76,5 @@
>  
>      @property
>      def statedir(self):
>          return os.path.join(self.topobjdir, '.mozbuild')
>  

I'm curious. Where does this come from? I've never had problems with 2.5 and 2.6, is that a 2.7.x < 2.7.3 issue?

Comment 7

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #6)
> I'm curious. Where does this come from? I've never had problems with 2.5 and
> 2.6, is that a 2.7.x < 2.7.3 issue?

Erf, comment 4 answers partly.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 687485 [details] [diff] [review]
Part 2: Implement mozconfig finding and loading in Python, v2

Review of attachment 687485 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/mozconfig.py
@@ +47,5 @@
>  
> +    RE_MAKE_VARIABLE = re.compile('''
> +        ^\s*                    # Leading whitespace
> +        (?P<var>[a-zA-Z_0-9]+)  # Variable name
> +        \s* [?:]?= \s*          # Assignment operator surrounded by spaces

There doesn't have to be spaces around the assignment operator.

@@ +262,5 @@
> +
> +                if current_type == 'AC_OPTION':
> +                    ac_options.append('\n'.join(current))
> +                elif current_type == 'MK_OPTION':
> +                    mk_options.append('\n'.join(current))

You could do something like result[current_type].append(). Likewise for before_exec/after_exec.

@@ +271,5 @@
> +
> +            assert current_type is not None
> +
> +            if current_type in ('BEFORE_EXEC', 'AFTER_EXEC'):
> +                # mozconfigs are executed using the Bourne shell. Unlike bash

bash is a bourne shell ;) Also note that /bin/sh is not guaranteed not to be bash. In fact, it is on many systems. Fortunately, when run as sh, bash switches to posix mode, and doesn't output functions in "set".

@@ +291,5 @@
> +                    # Reached the end of a multi-line variable.
> +                    if line.endswith("'") and not line.endswith("\\'"):
> +                        current.append(line[:-1])
> +                        value = '\n'.join(current)
> +                        in_variable = None

Note that a line can legitimately end with a single quote while not ending the variable value:
$ foo="a='b'
c='d'"
$ set
foo='a='"'"'b'"'"'
c='"'"'d'"'"

::: python/mozbuild/mozbuild/mozconfig_loader
@@ +31,5 @@
> +    echo "------END_MK_OPTION"
> +  done
> +}
> +
> +echo "------BEGIN_BEFORE_EXEC"

technically, "." doesn't exec, it sources.

Comment 9

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41981e8073a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c2cf89f79e2

(In reply to Mike Hommey [:glandium] from comment #8)
> bash is a bourne shell ;) Also note that /bin/sh is not guaranteed not to be
> bash. In fact, it is on many systems. Fortunately, when run as sh, bash
> switches to posix mode, and doesn't output functions in "set".

How do you know these things?!

> Note that a line can legitimately end with a single quote while not ending
> the variable value:
> $ foo="a='b'
> c='d'"
> $ set
> foo='a='"'"'b'"'"'
> c='"'"'d'"'"

Filed bug 818377 as a follow-up.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 687484 [details] [diff] [review]
Part 1: Move mozconfig code into own Python module, v2

Review of attachment 687484 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/base.py
@@ +46,5 @@
>  
>      @property
>      def topobjdir(self):
>          if self._topobjdir is None:
> +            if self.mozconfig['topobjdir'] is None:

self.mozconfig or self._mozconfig?

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/41981e8073a1
https://hg.mozilla.org/mozilla-central/rev/2c2cf89f79e2

Comment 12

[Gregory Szorc [:gps]]

(In reply to :Ms2ger from comment #10)
> Comment on attachment 687484 [details] [diff] [review]
> Part 1: Move mozconfig code into own Python module, v2
> 
> Review of attachment 687484 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/base.py
> @@ +46,5 @@
> >  
> >      @property
> >      def topobjdir(self):
> >          if self._topobjdir is None:
> > +            if self.mozconfig['topobjdir'] is None:
> 
> self.mozconfig or self._mozconfig?

This is intentional. We go through the public @property to trigger loading if it hasn't happened already.