780329 - mozbuild features to land mach

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

This will contain patches to mozbuild required to land mach (bug 751795).

Comment 1

[Gregory Szorc [:gps]]

Attachment: Part 1: Add config and base class modules, v1

This adds a config module to hold mozbuild configuration state and a base class to hold common functionality. These are used by a lot of components that will appear in future patches on this bug.

I imagine both of these will change rather drastically as mozbuild matures, especially config.py.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Part 2: Add testing module, v1

Adds some modules to interact with test runners. This is incomplete, I know. We can file follow-up bugs to add missing functionality, IMO.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Part 3: Add configure module, v1

This adds a module that interacts with configure. It duplicates a lot of functionality in client.mk. See comments in code for details on possible resolutions.

I think it is good enough to land. f? to Mike Hommey for obvious issues that should be corrected.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Part 4: Add modules for building the tree, v1

This adds code for building the tree. Most of it is to support progress indicators in mach. But, there is some meat in there too. I imagine this will all get rewritten once I'm satisfied with an API for backends, etc. It is good enough for now, IMO.

Comment 5

[Gregory Szorc [:gps]]

Attachment: Part 5: Ability to register structured loggers, v1

Simple change to LoggingManager to allow other structured loggers to be registered. This is needed to support mach, as it chains to its own root logger ("mach"), not "mozbuild."

Comment 6

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

Comment on attachment 648918 [details] [diff] [review]
Part 2: Add testing module, v1

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

Looks really nice!

Wondering if we want to put result parsing code here too at some point?

::: python/mozbuild/mozbuild/testing/mochitest.py
@@ +6,5 @@
> +
> +from mozbuild.base import Base
> +
> +
> +class MochitestRunner(Base):

Another way to go here would be having MochitestPlainRunner, MochitestChromeRunner, ... classes, probably with a base class for shared code. Not sure which way is better.

@@ +22,5 @@
> +        # TODO hook up Python harness runner.
> +        self._run_make(directory='.', target='mochitest-browser-chrome')
> +
> +    def run_mochitest_test(self, test_file=None, plain=False, chrome=False,
> +            browser=False):

This feels like a strange API; why not suite=(one of "plain", "chrome", "browser")?

@@ +28,5 @@
> +            raise Exception('test_file must be defined.')
> +
> +        if test_file == 'all':
> +            self.run_plain_suite()
> +            return

This doesn't make sense; I'm not sure how this will end up being called, but either this special case needs to be removed or it needs to handle other suites.

@@ +58,5 @@
> +            normalized = test_path[len(self.srcdir):]
> +
> +        return {
> +            'normalized': normalized,
> +            'is_dir': os.path.isdir(test_path)

Minor optimization: call os.path.isdir only once?

::: python/mozbuild/mozbuild/testing/suite.py
@@ +19,5 @@
> +
> +          all - All test suites
> +          mochitest-plain - Plain mochitests
> +          mochitest-browser-chrome - mochitests with browser chrome
> +          xpcshell - xpcshell tests

Followup for make check, m-a11y, m-chrome, reftests, crashtests, jsreftests, m-ipcplugins, please.

::: python/mozbuild/mozbuild/testing/xpcshell.py
@@ +31,5 @@
> +            return
> +
> +        # dirname() gets confused if there isn't a trailing slash.
> +        if os.path.isdir(test_file) and not test_file.endswith(os.path.sep):
> +            test_file += os.path.sep

Seen this code before... Sharing? :)

@@ +36,5 @@
> +
> +        relative_dir = test_file
> +
> +        if test_file.find(self.srcdir) == 0:
> +            relative_dir = test_file[len(self.srcdir):]

Same here, though the other one used startswith().

Comment 7

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

Comment on attachment 648921 [details] [diff] [review]
Part 3: Add configure module, v1

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

::: python/mozbuild/README.rst
@@ +11,5 @@
>  * mozbuild.compilation -- Functionality related to compiling. This
>    includes managing compiler warnings.
> +* mozbuild.configuration -- Configuration of the build system backend.
> +  includes interfaces for interacting with configure. This is not to be
> +  configured with the config.py module in the base mozbuild package which

confused?

::: python/mozbuild/mozbuild/configuration/configure.py
@@ +36,5 @@
> +
> +    def __init__(self, config):
> +        Base.__init__(self, config)
> +
> +        self._autoconf = False

I think I'd initialize to None instead

@@ +71,5 @@
> +
> +            if oldest is None or mtime < oldest:
> +                oldest = mtime
> +
> +        return oldest

Does

return min(os.path.getmtime(configure) for configure in self.configure_scripts)

work?

@@ +76,5 @@
> +
> +    def run_configure(self):
> +        """Runs configure.
> +
> +        Unlike other methods, this one always runs configure.

I guess that makes sense for a method called run_configure :)

@@ +88,5 @@
> +        env = self.config.get_environment_variables()
> +
> +        # configure calls out to things that expect MAKE to be defined.
> +        # We must use the same heuristic as Base._run_make to determine which
> +        # path to specify.

Sharable?

@@ +93,5 @@
> +        if self._is_windows():
> +            pymake = os.path.join(self.config.source_directory, 'build',
> +            'pymake', 'make.py')
> +
> +            env['MAKE'] = ' '.join([sys.executable, pymake]).replace('\\', '/')

Base._run_make doesn't seem to bother with the sys.executable?

Comment 8

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

Comment on attachment 648923 [details] [diff] [review]
Part 4: Add modules for building the tree, v1

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

::: python/mozbuild/README.rst
@@ +12,2 @@
>  * mozbuild.compilation -- Functionality related to compiling. This
>    includes managing compiler warnings.

The distinction between .building and .compiling isn't entirely clear to me. Is .compiling about just one file at a time?

::: python/mozbuild/mozbuild/building/treebuilder.py
@@ +44,5 @@
> +
> +        build -- This BuildInvocation instance.
> +        action -- Single word str describing the action being performed.
> +        directory -- If a directory state change caused this update, this will
> +            be he str of the directory that changed.

Will be what?

@@ +61,5 @@
> +
> +        self.action = action
> +
> +        for k, v in self.directories.iteritems():
> +            self.directories[k] = {'start_time': None, 'finish_time': None}

iterkeys()?

Comment 9

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

Comment on attachment 648917 [details] [diff] [review]
Part 1: Add config and base class modules, v1

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

::: python/mozbuild/mozbuild/base.py
@@ +143,5 @@
> +        # PyMake everywhere. Maybe in the future.
> +        if self._is_windows():
> +            # We invoke pymake as a sub-process. Ideally, we would import the
> +            # module and make a method call. Unfortunately, the PyMake API
> +            # doesn't easily support this.

Bug for that?

@@ +154,5 @@
> +
> +        fn = self._run_command_in_objdir
> +
> +        if srcdir:
> +            fn = self._run_command_in_srcdir

fn = self._run_command_in_srcdir if srcdir else self._run_command_in_objdir?

Comment 10

[Gregory Szorc [:gps]]

Attachment: Add testing module, v2

Incorporated feedback from Ms2ger.

Comment 11

[Gregory Szorc [:gps]]

Attachment: Part 3: Add configure module, v2

Incorporate feedback from Ms2ger.

Comment 12

[Gregory Szorc [:gps]]

Ms2ger: Thank you for the drive-by reviews! They are very much appreciated! I have incorporated most of your feedback into my patches. Where it resulted in significant enough changes, I have uploaded new patch versions. In some cases, we were only talking about documentation, so I'll save everyone the bugspam.

Some misc replies:

(In reply to :Ms2ger from comment #6)
> Wondering if we want to put result parsing code here too at some point?

Possibly. This will evolve over time. We need to hook up the native Python test runner first, etc.
 
> ::: python/mozbuild/mozbuild/testing/mochitest.py
> @@ +6,5 @@
> > +
> > +from mozbuild.base import Base
> > +
> > +
> > +class MochitestRunner(Base):
> 
> Another way to go here would be having MochitestPlainRunner,
> MochitestChromeRunner, ... classes, probably with a base class for shared
> code. Not sure which way is better.

Ditto.

> ::: python/mozbuild/mozbuild/configuration/configure.py
> @@ +36,5 @@
> > +
> > +    def __init__(self, config):
> > +        Base.__init__(self, config)
> > +
> > +        self._autoconf = False
> 
> I think I'd initialize to None instead

Actually, I'm using False to prevent redundant path searching. I've added a comment to make this clearer.
 
> @@ +71,5 @@
> > +
> > +            if oldest is None or mtime < oldest:
> > +                oldest = mtime
> > +
> > +        return oldest
> 
> Does
> 
> return min(os.path.getmtime(configure) for configure in
> self.configure_scripts)
> 
> work?

Yes. Changed in new patch.

> @@ +88,5 @@
> > +        env = self.config.get_environment_variables()
> > +
> > +        # configure calls out to things that expect MAKE to be defined.
> > +        # We must use the same heuristic as Base._run_make to determine which
> > +        # path to specify.
> 
> Sharable?
> 
> @@ +93,5 @@
> > +        if self._is_windows():
> > +            pymake = os.path.join(self.config.source_directory, 'build',
> > +            'pymake', 'make.py')
> > +
> > +            env['MAKE'] = ' '.join([sys.executable, pymake]).replace('\\', '/')
> 
> Base._run_make doesn't seem to bother with the sys.executable?

You've stumbled across the hackiness that is building on Windows :) The MAKE environment variable must have the Python interpreter in it to appease the pymake gods. We don't need this in run_make() because, well, the build system is funky like that. I could unify bits, but we still need to handle pymake specially. We're talking about simple path computation, so I don't think the unification is worth it at this stage.

Comment 13

[Gregory Szorc [:gps]]

Attachment: Part 6: Define CompilerWarning.__cmp__, v1

This is used by mach (future patch) so a sorted() on all the emitted warnings yields a list in a consistent and sane order.

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 648923 [details] [diff] [review]
Part 4: Add modules for building the tree, v1

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

::: python/mozbuild/mozbuild/building/tiers.py
@@ +13,5 @@
> +    def get_tiers(self):
> +        return TIERS
> +
> +    def get_actions(self):
> +        return ACTIONS

I don't think it makes sense to keep the notion of tier, especially at a high level.

::: python/mozbuild/mozbuild/building/treebuilder.py
@@ +16,5 @@
> +RE_TIER_DECLARE = re.compile(r'tier_(?P<tier>[a-z]+):\s(?P<directories>.*)')
> +RE_TIER_ACTION = re.compile(r'(?P<action>[a-z]+)_tier_(?P<tier>[a-z_]+)')
> +
> +RE_ENTERING_DIRECTORY = re.compile(
> +    r'^make(?:\[\d+\])?: Entering directory `(?P<directory>[^\']+)')

gmake localizes these messages, and it can hide them, too.

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 649377 [details] [diff] [review]
Part 3: Add configure module, v2

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

I have mixed feelings with this. This seems like an awful lot of code to replace the very much smaller and more concise Makefile form from client.mk. Not to mention that hardcoded paths spread all around the code doesn't make it easy maintenance.

::: python/mozbuild/mozbuild/configuration/configure.py
@@ +19,5 @@
> +    arguably necessary evil. It was originally implemented like this because
> +    mozbuild was completely outside the build system and making it work with
> +    client.mk would require hacking client.mk to support some "odd" scenarios.
> +    This would have made an already difficult-to-read makefile even more
> +    complicated.

This sound more like a comment for bugzilla than for code documentation.

@@ +96,5 @@
> +                require_unix_environment=True,
> +                log_name='configure_output')
> +
> +        # Not sure why this is here. But, it's in client.mk.
> +        os.utime(self._get_objdir_path('Makefile'), None)

Probably worth removing.

@@ +150,5 @@
> +            'allmakefiles.sh',
> +            'nsprpub/configure',
> +            'config/milestone.txt',
> +            'js/src/config/milestone.txt',
> +            'browser/config/version.txt',

I know this is copied from client.mk, but this is wrong. This should depend on the application being built.

@@ +158,5 @@
> +        for p in simple_paths:
> +            paths.append(self._get_srcdir_path(p))
> +
> +        for p in glob.glob('%s/*/confvars.sh' % self.srcdir):
> +            paths.append(p)

Likewise.

Comment 16

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #14)
> ::: python/mozbuild/mozbuild/building/tiers.py
> @@ +13,5 @@
> > +    def get_tiers(self):
> > +        return TIERS
> > +
> > +    def get_actions(self):
> > +        return ACTIONS
> 
> I don't think it makes sense to keep the notion of tier, especially at a
> high level.

This is mainly for progress tracking. This will all get rewritten as soon as we have the concept of different backends.

> 
> ::: python/mozbuild/mozbuild/building/treebuilder.py
> @@ +16,5 @@
> > +RE_TIER_DECLARE = re.compile(r'tier_(?P<tier>[a-z]+):\s(?P<directories>.*)')
> > +RE_TIER_ACTION = re.compile(r'(?P<action>[a-z]+)_tier_(?P<tier>[a-z_]+)')
> > +
> > +RE_ENTERING_DIRECTORY = re.compile(
> > +    r'^make(?:\[\d+\])?: Entering directory `(?P<directory>[^\']+)')
> 
> gmake localizes these messages, and it can hide them, too.

Good catch on the localization.

As far as hiding, I'm explicitly passing -w to gmake to force printing of these messages. This overrides -s.

(In reply to Mike Hommey [:glandium] from comment #15)
> I have mixed feelings with this. This seems like an awful lot of code to
> replace the very much smaller and more concise Makefile form from client.mk.
> Not to mention that hardcoded paths spread all around the code doesn't make
> it easy maintenance.

Then please separate out configure logic into its own .mk file so that file can be consumed on its own, outside of client.mk. I refuse the wrap client.mk from this code.

> @@ +96,5 @@
> > +                require_unix_environment=True,
> > +                log_name='configure_output')
> > +
> > +        # Not sure why this is here. But, it's in client.mk.
> > +        os.utime(self._get_objdir_path('Makefile'), None)
> 
> Probably worth removing.

Do you know why?

> @@ +150,5 @@
> > +            'allmakefiles.sh',
> > +            'nsprpub/configure',
> > +            'config/milestone.txt',
> > +            'js/src/config/milestone.txt',
> > +            'browser/config/version.txt',
> 
> I know this is copied from client.mk, but this is wrong. This should depend
> on the application being built.
> 
> @@ +158,5 @@
> > +        for p in simple_paths:
> > +            paths.append(self._get_srcdir_path(p))
> > +
> > +        for p in glob.glob('%s/*/confvars.sh' % self.srcdir):
> > +            paths.append(p)
> 
> Likewise.

So, uh, let's fix client.mk (by moving logic into a standalone .mk file or similar).

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 648917 [details] [diff] [review]
Part 1: Add config and base class modules, v1

I want to make significant changes to how configs work before this lands. So, I'm cancelling that part of the review. Feel free to look at the base class if you want.

Comment 18

[Gregory Szorc [:gps]]

Attachment: Part 1: Generic config file management, v1

I totally revamped how configs are managed. We now have generic functionality for representing config settings. Ideally, this code would live outside mozbuild. But, I have no clue where it could go and I was too lazy to create a new package in python/ for it. If you want, I can do this, however.

Anyway, the new config system is pretty cool. It's all just a glorified wrapper around ConfigParser. But, you have strong typing, validation, a saner API, etc. You even have gettext to provide translations for the usage strings for individual options. (I'm using this later in mach to generate a mach.ini file with in-line comments on what each option is for.)

Anyway, the modules are mostly done. I didn't implement validate(). I can do that in a follow-up, methinks.

Comment 19

[Gregory Szorc [:gps]]

Attachment: Part 2: Base module, v2

The new Base and BuildConfig modules, rebased on top of the generic config modules. It even includes an en-US locale file. I'll figure out in follow-up bugs how to best integrate all this with the l10n processes at Mozilla. For now, we "support l10n" but only have en-US support.

Comment 20

[Gregory Szorc [:gps]]

Attachment: Part 3: Testing modules, v2

Rebased.

Comment 21

[Gregory Szorc [:gps]]

Attachment: Part 4: Add configure module, v3

Rebased.

Comment 22

[Gregory Szorc [:gps]]

Attachment: Part 5: Add building modules, v2

Rebased.

Comment 23

[Gregory Szorc [:gps]]

Attachment: Part 6: Ability to register structured loggers, v2

Comment 24

[Gregory Szorc [:gps]]

Attachment: Part 7: Define CompilerWarning.__cmp__, v2

Comment 25

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

Comment on attachment 651956 [details] [diff] [review]
Part 1: Generic config file management, v1

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

::: python/mozbuild/mozbuild/config.py
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +r"""

A regex?

@@ +53,5 @@
> +        """Registers config settings.
> +
> +        This is called automatically be the constructor. Child classes are
> +        expected to implement it. It likely makes 1 or more calls to
> +        _register_setting() for the registration of individual settings.

_register_settings or register_setting?

Comment 26

[Mike Hommey [:glandium]]

(In reply to :Ms2ger from comment #25)
> > +r"""
> 
> A regex?

r'' is not specifically for regexps, although it's useful for that. It's for raw strings. As in, backslashes don't do anything in them.

Comment 27

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #16)
> Then please separate out configure logic into its own .mk file so that file
> can be consumed on its own, outside of client.mk. I refuse the wrap
> client.mk from this code.

Seriously, what's so wrong with wrapping make -f client.mk configure? I also don't see why this would need to be separated out of client.mk.

Comment 28

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #27)
> (In reply to Gregory Szorc [:gps] from comment #16)
> > Then please separate out configure logic into its own .mk file so that file
> > can be consumed on its own, outside of client.mk. I refuse the wrap
> > client.mk from this code.
> 
> Seriously, what's so wrong with wrapping make -f client.mk configure? I also
> don't see why this would need to be separated out of client.mk.

1) I killed mozconfigs. client.mk expects them to exist and spews crap and has side-effects related to them.
2) client.mk's API is fragile and I don't want to build on that.
3) client.mk is going to die as soon as the growing faction lynches it.
4) client.mk touches Makefile after running configure. In my future patches, Makefile doesn't exist by the time configure finishes running and this will cause the client.mk invocation to fail. (This may change as my code integrates deeper with config.status.)

I admit duplicating functionality is less than ideal. I would prefer to not do that. I would create a patch to factor things out of client.mk, but I'm scared to touch that file.

Comment 29

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #28)
> (In reply to Mike Hommey [:glandium] from comment #27)
> > (In reply to Gregory Szorc [:gps] from comment #16)
> > > Then please separate out configure logic into its own .mk file so that file
> > > can be consumed on its own, outside of client.mk. I refuse the wrap
> > > client.mk from this code.
> > 
> > Seriously, what's so wrong with wrapping make -f client.mk configure? I also
> > don't see why this would need to be separated out of client.mk.
> 
> 1) I killed mozconfigs.

Wait. what?!

> 2) client.mk's API is fragile and I don't want to build on that.
> 3) client.mk is going to die as soon as the growing faction lynches it.
> 4) client.mk touches Makefile after running configure.

And I don't think it really needs to.

> In my future patches,
> Makefile doesn't exist by the time configure finishes running

You mean those where the build doesn't happen with make?

> I admit duplicating functionality is less than ideal. I would prefer to not
> do that. I would create a patch to factor things out of client.mk, but I'm
> scared to touch that file.

You shouldn't be scared. And I still don't see why this would need to move out of client.mk. This can pretty well stay where it is.

Comment 30

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #29)
> > 4) client.mk touches Makefile after running configure.
> 
> And I don't think it really needs to.

FWIW, I'm not even sure it was added for a clear purpose... it comes from this changeset, which introduced MOZ_MAKE_FLAGS (and thus, is completely unrelated):
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla&command=DIFF_FRAMESET&file=client.mk&rev2=1.59&rev1=1.58

Comment 31

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #29)
> (In reply to Gregory Szorc [:gps] from comment #28)
> > (In reply to Mike Hommey [:glandium] from comment #27)
> > > (In reply to Gregory Szorc [:gps] from comment #16)
> > > > Then please separate out configure logic into its own .mk file so that file
> > > > can be consumed on its own, outside of client.mk. I refuse the wrap
> > > > client.mk from this code.
> > > 
> > > Seriously, what's so wrong with wrapping make -f client.mk configure? I also
> > > don't see why this would need to be separated out of client.mk.
> > 
> > 1) I killed mozconfigs.
> 
> Wait. what?!

mach/mozbuild doesn't use mozconfigs. Instead, there is part 1 of this patch that provides a generic settings class. mozbuild provides some configuration settings related to building. mach provides additional settings. mach reads a settings file (looks like an .ini) into the generic settings class. That's the replacement for mozconfigs.

That's only with this code. I don't see mozconfigs going away until this becomes the official build system. That may never happen.

> 
> > In my future patches,
> > Makefile doesn't exist by the time configure finishes running
> 
> You mean those where the build doesn't happen with make?

Yup.

> > I admit duplicating functionality is less than ideal. I would prefer to not
> > do that. I would create a patch to factor things out of client.mk, but I'm
> > scared to touch that file.
> 
> You shouldn't be scared. And I still don't see why this would need to move
> out of client.mk. This can pretty well stay where it is.

Given that we'd like to get rid of client.mk, I think it makes more sense to factor out rather than modify in-place.

Comment 32

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #31)
> That's only with this code. I don't see mozconfigs going away until this
> becomes the official build system. That may never happen.

> Given that we'd like to get rid of client.mk, I think it makes more sense to
> factor out rather than modify in-place.

Isn't that contradictory?

Comment 33

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #32)
> (In reply to Gregory Szorc [:gps] from comment #31)
> > That's only with this code. I don't see mozconfigs going away until this
> > becomes the official build system. That may never happen.
> 
> > Given that we'd like to get rid of client.mk, I think it makes more sense to
> > factor out rather than modify in-place.
> 
> Isn't that contradictory?

Yes. I'm hedging that this will be the official frontend to the build system someday ;)

Comment 34

[Gregory Szorc [:gps]]

Comment on attachment 651958 [details] [diff] [review]
Part 2: Base module, v2

The config stuff may change. Everything else should be pretty stable.

Comment 35

[Gregory Szorc [:gps]]

Comment on attachment 651960 [details] [diff] [review]
Part 4: Add configure module, v3

We should be able to use work from bug 770498 here. This will eliminate the DRY violation. Will provide a new patch once things have landed...

Comment 36

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

Comment on attachment 651959 [details] [diff] [review]
Part 3: Testing modules, v2

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

::: python/mozbuild/mozbuild/testing/mochitest.py
@@ +15,5 @@
> +    """
> +    def run_plain_suite(self):
> +        """Runs all plain mochitests."""
> +        # TODO hook up Python harness runner.
> +        self._run_make(directory='.', target='mochitest-plain')

Is this supposed to be run in the objdir, then?

Comment 37

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

Comment on attachment 651956 [details] [diff] [review]
Part 1: Generic config file management, v1

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

::: python/mozbuild/mozbuild/config.py
@@ +294,5 @@
> +        ignored.
> +        """
> +        filtered = [f for f in filenames if os.path.exists(f)]
> +
> +        self.load_fps([open(f, 'r') for f in filtered])

Not closing those fp's?

Comment 38

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

Comment on attachment 651958 [details] [diff] [review]
Part 2: Base module, v2

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

::: python/mozbuild/mozbuild/base.py
@@ +317,5 @@
> +        assert os.path.isabs(source_dir)
> +
> +        self.settings.paths.source_directory = source_dir
> +        self.settings.paths.object_directory = os.path.join(source_dir,
> +            'objdir')

Does anybody put their objdir there? :)

Comment 39

[Jeff Hammel]

Comment on attachment 651958 [details] [diff] [review]
Part 2: Base module, v2

+class Base(object):

A bit of a vague name, don't you think?  Can we be more explicit?

+    def __init__(self, settings, log_manager):
+        self.settings = settings

It'd be nice to have a docstring telling what these things are.

+            # We invoke pymake as a sub-process. Ideally, we would import the
+            # module and make a method call. Unfortunately, the PyMake API
+            # doesn't easily support this.

Is there a bug on this? (If not, there probably should be)

+        env -- Dict of environment variables to append to the current set of
+            environment variables.
+        explicit_env -- Dict of environment variables to set for the new
+            process. Any existing environment variables will be ignored.

I don't really like this pattern, but I guess this is fine for now.

+        assert isinstance(args, list) and len(args) > 0

I'd prefer just `len(args)` vs `len(args) > 0`

+    def _find_executable_in_path(self, name):
+        """Implementation of which.

So one thing that really annoys me is that python doesn't have this in the stdlib. So instead we have this function in various forms duplicated 10+ times in Mozilla alone.  If we wanted to add http://pypi.python.org/pypi/which to the build I would not be opposed.  If not, I would *highly* encourage moving this to a standalone function in some appropriate module (perhaps also named `which`).  There is no reason this should be a class/instance method.

+    """Represents a configuration for the build environment."""

Can probably lose the "Represents"

+    def configure_args(self):

This is a bit hard-codey but fine for now

+        self.settings.paths.object_directory = os.path.join(source_dir,
+            'objdir')

Does this mean we don't want to/can't specify objdir anymore?  FWIW, I prefer an objdir that is not a subdir of the source_dir

I'd *really* like to see `which` moved to its own module or what not, but other than that looks good.

Comment 40

[Jeff Hammel]

Comment on attachment 651959 [details] [diff] [review]
Part 3: Testing modules, v2

+import os.path

`import os`, plz

+class MochitestRunner(Base):

etc; it feels there is rather strong overlap between mozbuild and mozharness wrt running tests.  While I don't feel we should block either at this point in time, I do feel that it is probably a good time to begin to plan to consolidate these.  :gps, have you talked to :aki about mozharness?

+        # TODO hook up Python harness runner.
Can you elaborate on what this means?

+        if suite == 'plain':
+            target = 'mochitest-plain'
+        elif suite == 'chrome':
+            target = 'mochitest-chrome'
+        elif suite == 'browser':
+            target = 'mochitest-browser-chrome'
+        else:
+            raise Exception('None or unrecognized mochitest suite type.')

I would tend to use a dict for this vs an if-else tree

+        if suite == 'xpcshell':
+            xpcshell.run_suite()
+            return
+
+        if suite == 'mochitest-plain':
+            mochitest.run_plain_suite()
+            return
+
+        if suite == 'mochitest-chrome':
+            mochitest.run_chrome_suite()
+            return
+
+        if suite == 'mochitest-browser':
+            mochitest.run_browser_chrome_suite()
+            return

Again, I would tend to use a dict for this sort of thing

+    def run_test(self, test_file=None, debug=False):
+        """Runs an individual xpcshell test."""
+        if test_file is None:
+            raise Exception('Test file must be defined.')

Why bother having a default value?  Just force passing it.

Comment 41

[Jeff Hammel]

Comment on attachment 651961 [details] [diff] [review]
Part 5: Add building modules, v2

+import os.path

Again, `import os`

+    def update_action(self, tier, action):
+        assert tier == self.tier

Not sure what this accomplishes?

+                directories - State of directories in the tier. Dict of str to
+                    int. Keys are relative paths in build system. Values are
+                    0 for queued, 1 for in progress, and 2 for finished.

Not necessarily a fan for magical ints, but fine for now

+                relative = directory[len(self.objdir) + 1:]

I see this pattern several places in the code.  There is os.path.relpath but IIRC it is python 2.6 and up.  Or maybe just a simple helper function would

Comment 42

[Jeff Hammel]

Comment on attachment 651963 [details] [diff] [review]
Part 7: Define CompilerWarning.__cmp__, v2

+        x = cmp(self['filename'], other['filename'])
+
+        if x != 0:
+            return x
+
+        y = cmp(self['line'], other['line'])
+
+        if y != 0:
+            return y
+
+        return cmp(self['column'], other['column'])

I would probably do this differently:

keys = ['filename', 'line', 'column']
for key in keys:
   x = cmp(self[key], other[key])
   if x!= 0:
      return x
return 0

Comment 43

[Gregory Szorc [:gps]]

Attachment: Part 0: Python 3 compatibility, v1

As part of incorporating feedback for all the patches in this bug, I'm going to ensure Python 3 compatibility. Might as well cross this bridge now.

Comment 44

[Gregory Szorc [:gps]]

Attachment: Part 1: Generic config file management, v2

Now with Python 3 compatibility.

Comment 45

[Jeff Hammel]

Comment on attachment 662000 [details] [diff] [review]
Part 0: Python 3 compatibility, v1

looks fine

Comment 46

[Jeff Hammel]

Comment on attachment 662020 [details] [diff] [review]
Part 1: Generic config file management, v2

+    TYPE_STRING = 1
+    TYPE_BOOLEAN = 2
+    TYPE_POSITIVE_INTEGER = 3
+    TYPE_INTEGER = 4
+    TYPE_ABSOLUTE_PATH = 5
+    TYPE_RELATIVE_PATH = 6
+    TYPE_PATH = 7

I don't really like this pattern.  Why not use handler classes for these?

+    @classmethod
+    def _register_settings(cls):
+        raise NotImplemented('%s must implement _register_settings.' %
+            __name__)

Could abc.abstractmethod or similar be used here?  (not sure)

Also, maybe this should have a docstring that at least tells you what you're supposed to implement.

+    def register_setting(cls, section, option, type, **kwargs):
<snip/>
+        Each setting has the following optional parameters:
+
+            default -- The default value for the setting. If None (the default)
+                there is no default.
<snip/>
+        domain = kwargs.get('domain', section)
etc.  ABICT, since you have three optional keyword args, you should just add them to the function signature rather than allowing people to just pass in whatever, e.g.

def register_setting(cls, section, option, type, default=None, domain=None, choices=None):

+        value = settings.foo.bar
+        value = settings['foo']['bar']
+        value = settings.foo['bar']

I find this a bit magical, but okay.

+    def validate(self):
+        """Ensure that the current config passes validation.
+
+        This is a generator of tuples describing any validation errors. The
+        elements of the tuple are:
+
+            (bool) True if error is fatal. False if just a warning.
+            (str) Type of validation issue. Can be one of ('unknown-section',
+                'missing-required', 'type-error')
+        """
+        pass

No need for `pass` if you have a docstring

I'm going to hold off on review until I understand the reasons for the architecture a bit better.  I think the signature for register_setting should be fixed to not take **kwargs.  But more so, I would like to see all of this rearchitected so that instead of magical TYPE_* numbers, the class took handlers to register these.  If desired, I can take a stab at how I would implement this.

Comment 47

[Gregory Szorc [:gps]]

(In reply to Jeff Hammel [:jhammel] from comment #46)
> Comment on attachment 662020 [details] [diff] [review]
> Part 1: Generic config file management, v2
> 
> +    TYPE_STRING = 1
> +    TYPE_BOOLEAN = 2
> +    TYPE_POSITIVE_INTEGER = 3
> +    TYPE_INTEGER = 4
> +    TYPE_ABSOLUTE_PATH = 5
> +    TYPE_RELATIVE_PATH = 6
> +    TYPE_PATH = 7
> 
> I don't really like this pattern.  Why not use handler classes for these?

My initial reaction was "because this is Python, not Java." But, there are a few places where I switch on the type, so I think just creating very simple classes isn't too bad.

> 
> +    @classmethod
> +    def _register_settings(cls):
> +        raise NotImplemented('%s must implement _register_settings.' %
> +            __name__)
> 
> Could abc.abstractmethod or similar be used here?  (not sure)

Admittedly I'm too lazy to try it, but I'm pretty sure it won't play well with @classmethod.

Revised patch forthcoming.

Comment 48

[Gregory Szorc [:gps]]

Attachment: Part 1: Generic config file management, v3

Incorporated review feedback on all points.

Comment 49

[Jeff Hammel]

> > Could abc.abstractmethod or similar be used here?  (not sure)
> 
> Admittedly I'm too lazy to try it, but I'm pretty sure it won't play well with @classmethod.

(I'm guessing you're right but I'm also too lazy to try ;)

Comment 50

[Jeff Hammel]

Comment on attachment 662773 [details] [diff] [review]
Part 1: Generic config file management, v3

+class PathType(ConfigType):

Maybe should inherit from StringType

Since most of these directly translate to types (that is you do 

if not isinstance(value, int):
            raise TypeError()

a bunch), I would be inclined to have the type live in the class and have the base method be

+    @classmethod
+    def validate(cls, value):
+        if not isinstance(value, cls._type):
+            raise TypeError()

But this is probably more sugar than anything else

This looks good to me :)

Comment 51

[Gregory Szorc [:gps]]

Attachment: Part 2: Base module, v3

So, I had a change of heart about trying to kill mozconfig files.

In the previous patch, things that were in mozconfig files were config options in the ConfigProvider in this patch. This included configure arguments, which application to build, path to object directory, etc.

In this patch, I removed everything configure related from mozbuild. And, I integrated the base class with mozconfig files by calling mozconfig2client-mk and feeding the result into pymake for parsing and extraction. Neato.

At this juncture, I'm only interested in obtaining topobjdir. In my ideal world, topobjdir would be defined in the mozbuild config file (which will likely be mach.ini). However, we're not there yet. In the current implementation, we reimplement the logic from client.mk to load the mozconfig file to determine topobjdir. This includes calling out to config.guess if there is no mozconfig file or if it doesn't define an explicit MOZ_OBJDIR.

One will note that I am not pulling out MOZ_MAKE_FLAGS. The reason being is that make's flags are now controlled via mozbuild's run_make API. The default behavior should be good enough for most. If we need more power, we can cross that bridge later using the config files.

I also don't pull out other variables from the evaluated mozconfig file. I /think/ all the important variable exports are only relevant to configure. Configure integration with mozbuild is patch #4. That patch will be overhauled to work with mozconfig files. That may touch parts of this patch. We'll see.

jhammel gets Python parts of review. glandium gets build integration foo. There are some minor parts of client.mk I didn't port over. I guess you'll tell me if I missed anything too important.

Comment 52

[Jeff Hammel]

Comment on attachment 663247 [details] [diff] [review]
Part 2: Base module, v3

+            # We invoke pymake as a sub-process. Ideally, we would import the
+            # module and make a method call. Unfortunately, the PyMake API
+            # doesn't easily support this.

Is there a bug on file?  It would be nice to have one and have it referenced here.

+            args.insert(0, 'make')

Is this sufficient?  E.g. gmake vs make, etc.

Looks good to me

Comment 53

[Gregory Szorc [:gps]]

(In reply to Jeff Hammel [:jhammel] from comment #52)
> Comment on attachment 663247 [details] [diff] [review]

> Is there a bug on file?  It would be nice to have one and have it referenced
> here.

Bug 793260 filed.

> 
> +            args.insert(0, 'make')
> 
> Is this sufficient?  E.g. gmake vs make, etc.

Good catch. I will switch this to use the newly-imported which package. I'll also have it look for gmake as well. I'll probably expose a property that returns this path because this logic is implemented in a later patch.

Comment 54

[Gregory Szorc [:gps]]

I landed patches 0, 1, 6, and 7 as parts 1-4. Confusing, I know. So, that takes care of the fluff. Now, to get the meat landed...

https://hg.mozilla.org/mozilla-central/rev/a591d696098e
https://hg.mozilla.org/mozilla-central/rev/8ac580ee8e04
https://hg.mozilla.org/mozilla-central/rev/67835b795f5a
https://hg.mozilla.org/mozilla-central/rev/f0c89cebf913

Comment 55

[:Benjamin Peterson]

Comment on attachment 651963 [details] [diff] [review]
Part 7: Define CompilerWarning.__cmp__, v2

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

If you want to ensure Python 3 compatibility, you should define rich comparison methods instead of __cmp__.

Comment 56

[Mike Hommey [:glandium]]

Comment on attachment 663247 [details] [diff] [review]
Part 2: Base module, v3

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

::: python/mozbuild/mozbuild/base.py
@@ +71,5 @@
> +        if self._topobjdir is None:
> +            config_guess = os.path.join(self.topsrcdir, 'build', 'autoconf',
> +                'config.guess')
> +            self._topobjdir = subprocess.check_output([config_guess],
> +                cwd=self.topsrcdir).strip()

That would place the objdir in $(CONFIG_GUESS) instead of obj-$(CONFIG_GUESS), but see below.

@@ +133,5 @@
> +            if name in ('.PYMAKE', 'MAKELEVEL', 'MAKEFLAGS'):
> +                continue
> +
> +            if name == 'MOZ_OBJDIR':
> +                self._topobjdir = get_value(name)

I don't know how many people do the kind of hacks I do in their .mozconfig, but in mine, MOZ_OBJDIR is defined with $(CONFIG_GUESS) in it, and that works because $(CONFIG_GUESS) is defined in client.mk. You may want to include client.mk, which, incidentally, would also get you the right value for MOZ_OBJDIR if it's not defined in .mozconfig.

@@ +191,5 @@
> +        if filename:
> +            args.extend(['-f', filename])
> +
> +        if allow_parallel:
> +            args.append('-j%d' % (self.settings.build.threads + 2))

why +2 ?

@@ +245,5 @@
> +    def _run_command_in_srcdir(self, **args):
> +        self._run_command(cwd=self.topsrcdir, **args)
> +
> +    def _run_command_in_objdir(self, **args):
> +        self._run_command(cwd=self.topobjdir, **args)

Are these two functions needed for anything else? Because instead of changing fn depending on whether srcdir is defined in _run_make, you could change the cwd argument, and these functions would be of no use.

Comment 57

[Gregory Szorc [:gps]]

Comment on attachment 651961 [details] [diff] [review]
Part 5: Add building modules, v2

Pretty sure Vlad has no intention of looking at this...

Comment 58

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #56)
> Comment on attachment 663247 [details] [diff] [review]
> Part 2: Base module, v3
> That would place the objdir in $(CONFIG_GUESS) instead of
> obj-$(CONFIG_GUESS), but see below.

Good catch!
 
> @@ +133,5 @@
> > +            if name in ('.PYMAKE', 'MAKELEVEL', 'MAKEFLAGS'):
> > +                continue
> > +
> > +            if name == 'MOZ_OBJDIR':
> > +                self._topobjdir = get_value(name)
> 
> I don't know how many people do the kind of hacks I do in their .mozconfig,
> but in mine, MOZ_OBJDIR is defined with $(CONFIG_GUESS) in it, and that
> works because $(CONFIG_GUESS) is defined in client.mk. You may want to
> include client.mk, which, incidentally, would also get you the right value
> for MOZ_OBJDIR if it's not defined in .mozconfig.

I don't want to touch client.mk with a 10m pole. I'll just repro this simple logic.

> 
> @@ +191,5 @@
> > +        if filename:
> > +            args.extend(['-f', filename])
> > +
> > +        if allow_parallel:
> > +            args.append('-j%d' % (self.settings.build.threads + 2))
> 
> why +2 ?

Yes, that's kind of silly.

> 
> @@ +245,5 @@
> > +    def _run_command_in_srcdir(self, **args):
> > +        self._run_command(cwd=self.topsrcdir, **args)
> > +
> > +    def _run_command_in_objdir(self, **args):
> > +        self._run_command(cwd=self.topobjdir, **args)
> 
> Are these two functions needed for anything else? Because instead of
> changing fn depending on whether srcdir is defined in _run_make, you could
> change the cwd argument, and these functions would be of no use.

They are just convenience functions. They are used in later patches. I agree they aren't *that* helpful.

Comment 59

[Gregory Szorc [:gps]]

Attachment: Part 2: Base module, v4

Incorporated glandium's feedback.

Comment 60

[Mike Hommey [:glandium]]

Comment on attachment 664782 [details] [diff] [review]
Part 2: Base module, v4

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

Looks good.

It's bug-equivalent to client.mk wrt CONFIG_GUESS. I'll file a followup to improve that, but that can land as-is.

Comment 61

[Gregory Szorc [:gps]]

Base: https://hg.mozilla.org/mozilla-central/rev/1ae2d42ad234
Testing: https://hg.mozilla.org/mozilla-central/rev/9226912d54eb

Comment 62

[Gregory Szorc [:gps]]

I'm calling this done. The remaining modules (configure and building) will land in a follow-up bug.