bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Implement mozconfig finding and loading in Python

RESOLVED FIXED in mozilla20

Status

Firefox Build System
General
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: gps, Assigned: gps)

Tracking

(Blocks: 1 bug)

Trunk
mozilla20
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 678083 [details] [diff] [review]
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.
Attachment #678083 - Flags: review?(mh+mozilla)
(Assignee)

Comment 1

6 years ago
Created attachment 678084 [details] [diff] [review]
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.
Attachment #678084 - Flags: feedback?(mh+mozilla)
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'' ?
Attachment #678083 - Flags: review?(mh+mozilla) → review+
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.
Attachment #678084 - Flags: feedback?(mh+mozilla) → feedback+
(Assignee)

Comment 4

6 years ago
Created attachment 687484 [details] [diff] [review]
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...
Assignee: nobody → gps
Attachment #678083 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #687484 - Flags: review?(mh+mozilla)
(Assignee)

Comment 5

6 years ago
Created attachment 687485 [details] [diff] [review]
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!
Attachment #678084 - Attachment is obsolete: true
Attachment #687485 - Flags: review?(mh+mozilla)
(Assignee)

Updated

6 years ago
Attachment #687484 - Attachment description: Move mozconfig code into own Python module, v1 → Part 1: Move mozconfig code into own Python module, v2
(Assignee)

Updated

6 years ago
Summary: Refactor mozconfig interaction in mozbuild → Implement mozconfig finding and loading in Python
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?
Attachment #687484 - Flags: review?(mh+mozilla) → review+
(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 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.
Attachment #687485 - Flags: review?(mh+mozilla) → review+
(Assignee)

Updated

6 years ago
Blocks: 818377
(Assignee)

Comment 9

6 years ago
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.
Target Milestone: --- → mozilla20
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?
https://hg.mozilla.org/mozilla-central/rev/41981e8073a1
https://hg.mozilla.org/mozilla-central/rev/2c2cf89f79e2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 818606
(Assignee)

Comment 12

6 years ago
(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.
(Assignee)

Updated

6 years ago
Blocks: 808280

Updated

5 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.