Bug 751795 (mach)

Alternative front-end for build system

RESOLVED FIXED in mozilla18

Status

enhancement
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: gps, Assigned: gps)

Tracking

(Blocks 3 bugs)

Trunk
mozilla18
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 12 obsolete attachments)

4.72 KB, patch
k0scist
: review+
Details | Diff | Splinter Review
16.96 KB, patch
k0scist
: review+
Ms2ger
: feedback+
Details | Diff | Splinter Review
2.53 KB, patch
k0scist
: review+
Details | Diff | Splinter Review
2.36 KB, patch
k0scist
: review+
Details | Diff | Splinter Review
6.38 KB, patch
k0scist
: review+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
Posted patch Proof of concept (obsolete) — Splinter Review
Our build system has a usability problem. Makefiles just aren't intuitive to use. e.g.

 $ make -f client.mk help
 > make: *** No rule to make target `help'.  Stop.

Thanks, make, that wasn't helpful.

Bug 749314 is a good start, but it is far from perfect. Even if we implemented convenient targets, we still have a number of problems stemming from the fact that makefiles are effectively DSLs for shell scripts. By sticking with solely makefiles as the driver for our build system, we're being held back by the lacking feature set and usability of make/shell. We can do better.

I propose that we provide a tool in the root directory of the repository which serves as a gateway to common actions. The big difference from client.mk or the Makefile in the objdir is that it is written in a more modern, user-friendly, and batteries-included language. Since large parts of the build system are written in it already, I say we go with Python.

The attached patch provides such a tool. It is currently lacking many features and is far from perfect. But, I already find it to be useful for my needs. For example, I can run a single xpcshell test from the srcdir via:

 $ ./mach xpcshell-test services/common/test/unit/test_load_modules.js

("mach" is German for "do")

The tool knows where the objdir dir is (by looking in a mozconfig-like file). Furthermore, it invokes the xpcshell test runner as a native Python method call! Best of all, since I'm in the source dir, shell tab-complete just works! Compare with:

 $ SOLO_FILE=test_load_modules.js make -C objdir/services/common/test xpcshell-test

The character counts aren't that different. But, the make version requires you to know more pieces of information. This is painful, especially for newcomers. Furthermore, in order to discover this command, you need to visit the right web page. Compare with:

 $ ./mach --help

>usage: mach action [arguments]
>
>This program is your main control point for the Mozilla build
>system.
>
>To perform an action, specify it as the first argument. Here are some common
>actions:
>
>  mach build         Build the source tree.
>  mach settings      Launch a wizard to guide you through build setup.
>  mach help          Show full help.
>  mach xpcshell-test Run an xpcshell test or test suite.
>
>To see more help for a specific action, run:
>
>  mach action --help
>
>e.g. mach build --help


 $ ./mach xpcshell-test --help

>usage: mach xpcshell-test [-h] [--debug] test_file
>
>positional arguments:
>  test_file    Path to test file to run.
>
>optional arguments:
>  -h, --help   show this help message and exit
>  --debug, -d  Run test in debugger.

A build system that helps me - yay!

Anyway, having an alternative driver (and I want to stress that that would be the nature of such a tool in the beginning - I'm not advocating replacing client.mk just yet) for the build system has all kinds of other advantages:

* Could serve as the "virtualenv" environment provider for Python (bug 661908)
* Would emphasize Python as the build system programming language rather than Makefiles. I believe this would result in a more readable, testable, and debuggable build system.
* Better documentation. You add flags to the tool and docs get generated automatically. This translates to easy feature discovery.
* Easier to extend. Do you know where in the build system is the proper place to add new functionality? I don't either. With this tool, you just provide a new action to a subsystem and you have full control over what to do.
* Easier to develop. I put this together on Caltrain over 2 trips between SF and MV. I've spent the same amount of time refactoring existing makefiles and still didn't produce a finished patch. And, I'm not incompetent with make: our build system is just that complicated.
* Easier to review. I think there are more people around comfortable in Python than there are with makefiles.
* gmake vs pymake? Let the tool decide for you based on your environment!
* A clean slate. You know those things you've always wanted to add to the build system but couldn't for historical reasons? Go right ahead.

On the patch itself, I'm only requesting feedback. I want to know "do you want this feature?" and "is there anything seriously wrong with the patch?" If you don't want this feature, my response is going to be "why do you hate developers?" Seriously, that will be my question. I believe that our current build system is hardly intuitive (especially for project newcomers) to the point it drives people away because it is so difficult. A tool like this would go far to bridge that gap.

Assuming people like the idea, what are my plans you ask? Well, I'd like to get the tool with a minimum viable feature set checked in. Maybe we initially just provide convenience for initial environment configuration, invoking `make -f client.mk`, and common post-build actions, like running tests. Maybe we just focus on test execution. That would be good enough for me.

Once it is checked in, others can staple on features. And, there is surely no limit to features. Want to provide actions to pull source from version control? I'd be cool with that! How about configuring your version control system to emit the proper patch format. People need to do this anyway, so why not make it part of the setup wizard? Posting patches directly to bugzilla - Why not! Starting a Try build (with a built-in wizard to configure all the options) - I'd use that! Launch an MXR search from the console - sweet! Yes, people have provided these tools today. But, they aren't included and people just don't know about the productivity they are missing out on! In essence, I want a batteries included development environment for Mozilla projects. I think providing one is one of the best ways we can attract and retain technical project contributors.

Some comments on using:

* Output is pretty silent by default. Try adding -v | --verbose: ./mach -v build
* The xpcshell test runner doesn't yet detect manifest files. I'll make that work later.

Anyway, some comments on the patch:

* There are vast features not implemented. Settings management is one. I'd like to have a settings wizard that guides you through the initial configuration. "What application do you want to build?" "Debug or release?" etc.
* Most of the features are incomplete. As the patch name implies, it is only a proof of concept. Things like hooking up the settings file to the arguments to configure are missing. The code is also missing a lot of polish.
* I put all the Python in a pylib/ directory. I'm not sure where else to put it. build/ maybe? But, IMO the functionality could be pertinent beyond just "build" activities. tools/? I've been advocating for a while that we establish a unified Python "site-packages" directory for all the in-tree Python. This is my attempt to sneak in a de facto standard location [with the hope that eventually existing Python like testing/mozbase get migrated there].
* The process execution logic doesn't work on Windows. It needs shell wrapping and non-blocking I/O that doesn't use fcntl. Maybe I could reuse mozprocess?
* It requires Python 2.7. It makes no sense to write Python targeting anything lower.
* Is "mozbuild" already taken? I'm all for using a better name.

Assuming I get f+ on the patch, I'd appreciate feedback on issues that would prevent r+ and landing.

If you find yourself typing a long reply arguing the merits of this idea, perhaps it would just be best to reply "let's take this to a mailing list." The only reason I didn't start there is because I have a patch and I want eyes on it ;)
Attachment #620925 - Flags: feedback?(ted.mielczarek)
Um, yeah. Excellent idea!

Having a tool that helps developers get started and perform typical tasks in the tree w/o having to remember the make voodoo would be awesome. And I agree, it could even go further and replace some make rules here and there, like you have demonstrated with xpcshell tests. That could lead to leaner Makefiles, for instance, which would already be a win for builds.
(Assignee)

Comment 2

7 years ago
(In reply to Philipp von Weitershausen [:philikon] from comment #1)
> And I agree, it could even go further and replace some make rules here
> and there, like you have demonstrated with xpcshell tests. That could
> lead to leaner Makefiles, for instance, which would already be a win for
> builds.

While that is true, I want to emphasize that the initial goal is to supplement - not replace - existing functionality. There are practical reasons why we don't want to move things out of makefiles just yet (Python 2.7 isn't deployed everywhere, we don't want to force people to break old habits, etc).

If a transition were to occur, we could always have the old makefile targets call ./mach with the appropriate arguments to preserve backwards-compatible behavior. This would probably be a net win since the complex logic would be implemented in Python not make.

Of course, this transition could apply to other things as well. How about having mach wrap the compiler? Now we're talking...
(CC'ing some people who might be interested in the simplifying people's first contribution aspects of this; particularly the setup wizard, patch-creation help, send-to-try suggestions in comment 0)
I'm sold on the concept. I'd just like to drop a pointer to http://hg.mozilla.org/users/josh_joshmatthews.net/mozconfig-creator which could be integrated in the initial setup.
(In reply to Gregory Szorc [:gps] from comment #0)
> The tool knows where the objdir dir is (by looking in a mozconfig-like
> file).

How would this interact with multiple object directories per source tree? I have frequently  used debug and release builds plus other applications off one source tree.
(Assignee)

Comment 6

7 years ago
(In reply to Mark Banner (:standard8) from comment #5)
> (In reply to Gregory Szorc [:gps] from comment #0)
> > The tool knows where the objdir dir is (by looking in a mozconfig-like
> > file).
> 
> How would this interact with multiple object directories per source tree? I
> have frequently  used debug and release builds plus other applications off
> one source tree.

Currently, you can provide a non-default location for the settings file via a command-line argument (./mach --settings=/path/to/file) or via an environment variable (MOZ_BUILD_CONFIG=/path/to/file ./mach).

We may want to adopt the popular convention of supporting settings merging from parent configs (e.g. ~/.machconfig). But, that would be a later feature, IMO.

I also think that more important settings (like the objdir) should be configurable via arguments or environment variables. This would allow you to share the same settings file across multiple objdirs and/or trees. The run-time settings exist as a Python class instance. So, it's just a matter a munging that at run-time. The rest of the execution environment doesn't care where the value came from.
(Assignee)

Comment 7

7 years ago
Posted patch Proof of concept (obsolete) — Splinter Review
Make Ted's eyes bleed less.

For those playing at home, the latest version will always be available at https://hg.mozilla.org/users/gszorc_mozilla.com/mq-sc/raw-file/default/mozbuild
Attachment #620925 - Attachment is obsolete: true
Attachment #620925 - Flags: feedback?(ted.mielczarek)
Attachment #621185 - Flags: feedback?(ted.mielczarek)
(Assignee)

Comment 8

7 years ago
It has been suggested through other mediums that this should integrate more with mozharness. I agree (on principle at least). However, I'm not sure what that will mean. Perhaps mach is a frontend to mozharness. Perhaps mach uses some of the mozharness modules (like mozprocess). Perhaps core bits of mach are rolled into mozharness. If someone who knows the landscape better could chime in, I'll listen. Perhaps ping me on IRC?

Comment 9

7 years ago
(In reply to Ed Morley [:edmorley] from comment #3)
> (CC'ing some people who might be interested in the simplifying people's
> first contribution aspects of this; particularly the setup wizard,
> patch-creation help, send-to-try suggestions in comment 0)

Thanks -- always excited to hear about how we can help make things easier for new contributors.

We're right in the middle of putting some conversion metrics in place that will allow us to see if changes like this help contributors move through the process of contributing.  

It would be great to figure out how this fits in with that so we can measure the effectiveness and give you information that could help you optimize things more.  For more details see:

https://wiki.mozilla.org/Contribute/Conversion_points

jdm, could you help me figure out how this maps to the coding contribution path we're putting together and could you flag if this applies to other functional areas as well?  The details of where this fits and how people will use it are a little over my head :)
(Assignee)

Comment 10

7 years ago
Posted patch Latest iteration (obsolete) — Splinter Review
Lots of code cleanup and new features:

* Integration with mozprocess (no more hacky process launching code - yay!)
* Compiler warning parsing. It only works with Clang so far. And, I've only tested with a SVN HEAD build. We write a JSON file at the end of the build with all the compiler warnings. I've provided basic mach sub-commands for extracting metadata from the report. We could probably mark bug 756804 as a dupe of this if this lands. There are 3047 warnings for a clobber build. 1343 -Wdelete-non-virtual-dtor, with ~950 in parser/html, ugh.
* Bypass client.mk. That's kinda the point of mach. I don't have all the dependencies worked out yet, which is unfortunate. e.g. if you run `mach build` without doing `mach configure` things are busted. This can be fixed.
* Now possible to run xpcshell tests via `mach xpcshell-test file|directory` and it just works.
* Color terminal output (when your terminal supports it)
* More config options. Support for custom CC, CFLAGS, etc
* Misc bug squashing and code refactoring
* PyMake automagically used over gmake on Windows. Sadly, I have to invoke PyMake as a Python process because its APIs are lacking. That could be fixed if there is a will.
* Started framework for self-testing test suite.

I'll give it a little more love before asking for formal review. I'm not looking for perfection here, just something that's a step in the right direction.
Attachment #621185 - Attachment is obsolete: true
Attachment #621185 - Flags: feedback?(ted.mielczarek)
Attachment #633403 - Flags: feedback?(ted.mielczarek)
(Assignee)

Comment 11

7 years ago
philikon wanted mochitest support, so I added it. It can be found in my HG patch queue repository, which is linked in comment #7. You'll also need to apply the 2 mozbuild-colorama patches.

I had to switch terminal colorization from blessings to colorama because blessings doesn't work on Windows :( I think I'll keep blessings in the tree because there are some nice abilities it provides [and I'd be OK if Windows didn't have the window dressing].
(Assignee)

Comment 12

7 years ago
Posted patch mozbuild, v1 (obsolete) — Splinter Review
After a bit of polish this weekend, I think I'm happy with the current state and am requesting review for tree inclusion.

Again, I don't think we need to strive for perfection here. And, I don't claim this tool attains it. What this tool does provide is a solid offering of convenience on a platform that is easily extensible and ready for others to start building.

This patch is self-standing: everything required to run it is in the patch or already in the tree. (I ripped out colorize and decided to just make the terminal window dressing conditional on blessings, which means no Windows support.)

Open Issues:

* Only tested on OS X on my machine, which has lots of zaniness, such as LLVM/Clang SVN HEAD. Linux probably works. Windows has a high chance of bustage. I'll verify it when I get into the office Monday and have access to a Windows machine.
* Bits are hacky. There is no real dependency tracking, mochitests are invoked through make, not as a native Python method, etc. At this juncture, I don't care.
* Compiler warning parsing only works on a new'ish version of Clang (not what is currently part of latest 10.7 XCode). I guess the format changed recently. I'll staple on older Clang, GCC, and MSVC compiler warning parsing in a later iteration.
* Needs more unit tests (what doesn't). They will be written over time and when the tool is actually not early alpha.

FWIW, pyflakes output is clean. pylint reports a number of violations with the default settings, mostly around missing docstrings and short variable names in short function bodies. I could probably clean out most of them with a few hours of work.
Assignee: nobody → gps
Attachment #633403 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #633403 - Flags: feedback?(ted.mielczarek)
Attachment #633963 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 13

7 years ago
Forgot to include notes for reviewer!

1) Read README.rst first. I've documented a lot of the developer-centric things. If you have any questions after reading that, the README needs updated.
2) Look at Python outside of mozbuild.cli first. This is all the backend code that actually does stuff. The bugs due to my ineptitude with the build system will be on display here.
3) mozbuild.cli is just the frontend to the backend pieces. It probably doesn't need as rigorous of a review. I imagine the architecture of these bits will change over time (more Python magic).
(Assignee)

Comment 14

7 years ago
Comment on attachment 633963 [details] [diff] [review]
mozbuild, v1

Windows is busted hard in this patch. I personally want Windows to work from day 0.

Required changes will involve process execution. There's lot of shell magic I need to perform. I don't look forward to it :/
Attachment #633963 - Flags: review?(ted.mielczarek)

Comment 15

7 years ago
(In reply to Gregory Szorc [:gps] from comment #14)
> Comment on attachment 633963 [details] [diff] [review]
> mozbuild, v1
> 
> Windows is busted hard in this patch. I personally want Windows to work from
> day 0.
> 
> Required changes will involve process execution. There's lot of shell magic
> I need to perform. I don't look forward to it :/

I haven't yet had time to review the patch in detail so I don't know what you're hitting. But, it might help to take a look at mozprocess - there is a version in the m-c tree at http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/mozprocess/processhandler.py It may be overkill for you but it does showcase a means of using windows C APIs to handle process management. Not sure if that is helpful or not. Also remember that everyone on windows is running inside an msys shell which is its own form of hell (because python won't realize the difference).

Comment 16

7 years ago
(In reply to Clint Talbert ( :ctalbert ) from comment #15)
> (In reply to Gregory Szorc [:gps] from comment #14)
> > Comment on attachment 633963 [details] [diff] [review]
> I haven't yet had time to review the patch in detail so I don't know what
> you're hitting. But, it might help to take a look at mozprocess - there is a
> version in the m-c tree at
> http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/
> mozprocess/processhandler.py 

LOL, next time I'll look at the patch first. I think we would definitely be interested in such a tool. I don't think the makefiles are going away anytime soon, but anything we can do to lower the barrier to entry is worthwhile. I like what you have so far.
(Assignee)

Comment 17

7 years ago
(In reply to Clint Talbert ( :ctalbert ) from comment #15)
> I haven't yet had time to review the patch in detail so I don't know what
> you're hitting. But, it might help to take a look at mozprocess - there is a
> version in the m-c tree at
> http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/
> mozprocess/processhandler.py It may be overkill for you but it does showcase
> a means of using windows C APIs to handle process management. Not sure if
> that is helpful or not. Also remember that everyone on windows is running
> inside an msys shell which is its own form of hell (because python won't
> realize the difference).

I switched to mozprocess a few major revisions ago. It provides exactly what I need for process execution!

The issues I'm running into are related to shells in Windows. Basically I need to steal PyMake's code for shell detection and have my "run process" function take an argument that says "require a UNIX environment" that when present proxies the command through sh.exe. On top of that, I have a nice healthy dose of path normalization and expectations of existing tools like PyMake and build system sanity checking. It is a lovely mess. Or, as I call it, a multi-beer problem.
(Assignee)

Updated

7 years ago
Blocks: 756804
(Assignee)

Comment 18

7 years ago
Posted patch mozbuild, v2 (obsolete) — Splinter Review
Got Windows working, so asking for review.

Other changes since last patch:

* Support for parsing MSVC compiler warnings
* Compiler warnings stored in a single database, not per-run databases.
* Implement autoconf/configure dependency tracking
* Don't emit terminal control characters when not acting as a TTY (e.g. allow output to be piped or redirected)
* Progress indicator when building. It is hot. Only works when Python has the ncurses module, which is not on Windows, sadly. Can add in a future release.
Attachment #633963 - Attachment is obsolete: true
Attachment #635460 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 19

7 years ago
Started dev doc at https://developer.mozilla.org/En/Developer_Guide/mach. My intent is for the README to have the main source of info. I personally don't like fragmenting the docs nor having the docs live too far away from the source. I will also throw something up on my personal blog (syndicated through planet.m.o) to announce this (after it lands on m-c).
(In reply to Gregory Szorc [:gps] from comment #18)
> * Progress indicator when building. It is hot. Only works when Python has
> the ncurses module, which is not on Windows, sadly. Can add in a future
> release.

Can I help with this? I found this: http://www.lfd.uci.edu/~gohlke/pythonlibs/#curses for curses on Windows. I can also try other things to make it work on Windows if you direct me.
(Assignee)

Updated

7 years ago
Blocks: 767434
(Assignee)

Comment 21

7 years ago
(In reply to Burak Yiğit Kaya [:BYK] from comment #20)
> (In reply to Gregory Szorc [:gps] from comment #18)
> > * Progress indicator when building. It is hot. Only works when Python has
> > the ncurses module, which is not on Windows, sadly. Can add in a future
> > release.
> 
> Can I help with this? I found this:
> http://www.lfd.uci.edu/~gohlke/pythonlibs/#curses for curses on Windows. I
> can also try other things to make it work on Windows if you direct me.

Please help! I've filed bug 767434 to track this since I don't want to scope creep any more.
(In reply to Gregory Szorc [:gps] from comment #8)
> It has been suggested through other mediums that this should integrate more
> with mozharness. I agree (on principle at least). However, I'm not sure what
> that will mean. Perhaps mach is a frontend to mozharness. Perhaps mach uses
> some of the mozharness modules (like mozprocess). Perhaps core bits of mach
> are rolled into mozharness. If someone who knows the landscape better could
> chime in, I'll listen. Perhaps ping me on IRC?

Mozharness != mozbase.
Mozharness is a generic script harness.
Mozbase is, aiui, specifically oriented towards testing the browser.

Mozprocess is part of mozbase.

http://hg.mozilla.org/build/mozharness
https://github.com/mozilla/mozbase
Comment on attachment 635460 [details] [diff] [review]
mozbuild, v2

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

::: build/pylib/mozbuild/mozbuild/cli/terminal.py
@@ +69,5 @@
> +
> +class BuildTierFooter(TerminalFooter):
> +    # TODO grab these from build system properly.
> +    TIERS = ['base', 'nspr', 'js', 'platform', 'app']
> +    ACTIONS = ['default', 'export', 'libs', 'tools']

Can't you use the module you created for those constants?

::: build/pylib/mozbuild/mozbuild/logger.py
@@ +84,5 @@
> +
> +        if s.startswith('TEST-PASS'):
> +            result = self.terminal.green(s[0:9]) + s[9:]
> +        elif s.startswith('TEST-UNEXPECTED'):
> +            result = self.terminal.red(s[0:20]) + s[21:]

TEST-KNOWN-FAIL?
(Assignee)

Comment 24

7 years ago
(In reply to :Ms2ger from comment #23)

Thanks for the drive-by! I think both issues are suitable for follow-up patches, especially the coloring one.

As for coloring, that is all just a giant, temporary hack. In my utopian future world, the test driver can invoke a callback with the results of an individual test. When the driver is invoked via the command line (e.g. |make xpcshell-tests|), that callback prints results to stdout (as it does today). If mach calls the test driver, it does its own formatting, complete with cleaner output and colorization. We could even add progress bars here if we wanted.

If anyone wants to start working on those changes to the test harnesses, please do!
(Assignee)

Updated

7 years ago
Blocks: 769394
(Assignee)

Comment 25

7 years ago
Posted patch mozbuild, v3 (obsolete) — Splinter Review
My powers of bit rot are now approaching :Unfocused's levels: I have bit rotted myself with bug 762837. This patch is just a 2 line delta from the previous. It defines the testing-only JS modules path when invoking the xpcshell test runner.
Attachment #635460 - Attachment is obsolete: true
Attachment #635460 - Flags: review?(ted.mielczarek)
Attachment #638455 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

7 years ago
Blocks: 729098
(Assignee)

Updated

7 years ago
Blocks: 773398
(Assignee)

Updated

7 years ago
Blocks: 773913
(Assignee)

Updated

7 years ago
Blocks: 774032
(Assignee)

Updated

7 years ago
Blocks: machfeatures
(Assignee)

Updated

7 years ago
No longer blocks: 729098
(Assignee)

Updated

7 years ago
No longer blocks: 767434
(Assignee)

Updated

7 years ago
No longer blocks: 773398
(Assignee)

Updated

7 years ago
No longer blocks: 773913
(Assignee)

Updated

7 years ago
No longer blocks: 774032
(Assignee)

Updated

7 years ago
No longer blocks: 756804
(Assignee)

Comment 26

7 years ago
Ted, et al: I created bug 774108 to track follow-up work and features for the new frontend. If you have any review comments like "this isn't adequate," it may be covered by a follow-up bug tracked in that umbrella.

All the interested bug lurkers may want to have a look at all the new feature requests I filed. If you have any new suggestions, please file a bug blocking 774108 and add [mach] to the whiteboard. If you can help with implementation, please do!
(Assignee)

Comment 27

7 years ago
Comment on attachment 638455 [details] [diff] [review]
mozbuild, v3

Jeff/Philipp: any chance I could get a drive-by from you? I'm trying to take the burden of this review off of just Ted and I think having a few f+ might help.

Please note I have filed a number of follow-up bugs to address obvious known shortcomings. If you have a "this is obviously awful" reaction, first check the tree of bug 774108 to see if I've captured it.

The invitation for feedback extends to anyone who is interested.
Attachment #638455 - Flags: feedback?(philipp)
Attachment #638455 - Flags: feedback?(jhammel)

Comment 28

7 years ago
AFAIK, you don't have to import os.path on any python 2.4+ on any OS. Importing os will allow access to os.path
(Assignee)

Comment 29

7 years ago
Posted patch mozbuild, v4 (obsolete) — Splinter Review
Mostly style fixes. Things in build/pylib/mozbuild no longer raise any pyflakes violations \o/.

I've also been thinking about splitting this patch up into first the distinctive backend and frontend bits. I would further divide that by functionality and get targeted reviews for each. Reviewer: please offer guidance for next steps.
Attachment #638455 - Attachment is obsolete: true
Attachment #638455 - Flags: review?(ted.mielczarek)
Attachment #638455 - Flags: feedback?(philipp)
Attachment #638455 - Flags: feedback?(jhammel)
Attachment #644474 - Flags: review?(ted.mielczarek)
Attachment #644474 - Flags: feedback?(philipp)
Attachment #644474 - Flags: feedback?(jhammel)
(Assignee)

Comment 30

7 years ago
Comment on attachment 644474 [details] [diff] [review]
mozbuild, v4

I'm going to split this patch up and get reviews from the component pieces. Once some of the core modules are landed, I'll return to this bug for frontend work.
Attachment #644474 - Flags: review?(ted.mielczarek)
Attachment #644474 - Flags: feedback?(philipp)
Attachment #644474 - Flags: feedback?(jhammel)
(Assignee)

Updated

7 years ago
Depends on: 777231
(Assignee)

Updated

7 years ago
Depends on: 777233
(Assignee)

Comment 31

7 years ago
For anyone on the review loop, http://gregoryszorc.com/blog/2012/07/25/mozilla-build-system-plan-of-attack/ describes the relationship between mozbuild, mach, etc.
(Assignee)

Updated

7 years ago
Depends on: 780329
(Assignee)

Comment 32

7 years ago
Posted patch mach, v1 (obsolete) — Splinter Review
All the mozbuild patches are separated out into bug 780329. This just contains the code for mach itself. Since the last patch, I rebased on top of changes to mozbuild. I also moved files into python/mach. There is now clearly a wall between the backend (mozbuild + everything else) and the frontend (mach).
Attachment #644474 - Attachment is obsolete: true
Attachment #649414 - Flags: review?(vladimir)
Comment on attachment 649414 [details] [diff] [review]
mach, v1

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

::: mach
@@ +17,5 @@
> +        platform.python_version() )
> +    sys.exit(1)
> +
> +# Ideally we could load build/virtualenv/packages.txt. But, we need a
> +# supet set of what's in there, so we go with a one-off for now.

superset

::: python/mach/mach/build.py
@@ +12,5 @@
> +    def __init__(self, config):
> +        Base.__init__(self, config)
> +
> +    def build(self):
> +        """Builds the tree."""

I don't find "the tree" clear, but that might just be me.

::: python/mach/mach/main.py
@@ +36,5 @@
> +
> +  %(prog)s help          Show full help.
> +  %(prog)s build         Build the source tree.
> +  %(prog)s test          Run a test suite.
> +  %(prog)s xpcshell-test Run xpcshell test(s).

It seems strange to special-case xpcshell here

@@ +62,5 @@
> +        if len(argv) == 0:
> +            parser.usage = Mach.USAGE
> +            parser.print_usage()
> +            return 0
> +        elif argv[0] == 'help':

No else after return, please

@@ +163,5 @@
> +            p = args.settings_file
> +        elif 'MACH_SETTINGS_FILE' in os.environ:
> +            p = os.environ['MACH_SETTINGS_FILE']
> +
> +        return p

I think I'd write this as

if args.settings_file:
  return args.settings_file

if 'MACH_SETTINGS_FILE' in os.environ:
  return os.environ['MACH_SETTINGS_FILE']

return os.path.join(self.cwd, 'mach.ini')

::: python/mach/mach/terminal.py
@@ +72,5 @@
> +
> +class BuildTierFooter(TerminalFooter):
> +    # TODO grab these from build system properly.
> +    TIERS = ['base', 'nspr', 'js', 'platform', 'app']
> +    ACTIONS = ['default', 'export', 'libs', 'tools']

From python/mozbuild/mozbuild/building/tiers.py, right?

::: python/mach/mach/testing.py
@@ +59,5 @@
> +            metavar='TEST',
> +            help='Test to run. Can be specified as a single JS file, a '
> +                 'directory, or omitted. If omitted, all the tests are '
> +                 'executed.')
> +        mochitest_plain.set_defaults(cls=Testing, method='run_mochitest_plain')

So you can do

./mach mochitest-plain
./mach mochitest-plain --test_file=all
./mach test --suite=mochitest-plain

to do the same thing, right? Just the first seems sufficient to me.
(Assignee)

Comment 34

7 years ago
Posted patch mach, v2 (obsolete) — Splinter Review
Mostly rebased on refactored settings foo. I also snuck in some new actions for managing settings files. We have 'settings-list' which prints lines containing all the configurable settings along with a short description. We also have 'settings-create' which prints a settings file with in-line comments on what each setting does. It is pretty helpful.
Attachment #649414 - Attachment is obsolete: true
Attachment #649414 - Flags: review?(vladimir)
Attachment #651964 - Flags: review?(vladimir)
Blocks: 766646
(Assignee)

Comment 35

7 years ago
Looks like Scott Johnson did something similar: http://www.glasstowerstudios.com/trac/JMozTools/

If only some hero could have found time in the past 5 months to review these patches, perhaps Scott wouldn't have wasted time duplicating effort.

If *anyone* out there wants to take *any* of the reviews on this and bug 780329, please do.
Comment on attachment 651964 [details] [diff] [review]
mach, v2

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

I assume you pyflakes'd all this?

::: mach
@@ +2,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/.
> +
> +import os.path

I hear this should be "import os"

@@ +14,5 @@
> +
> +if major != '2' or minor != '7':
> +    print 'Python 2.7 is required to run mach. You are running %s.' % (
> +        platform.python_version() )
> +    sys.exit(1)

Do you know if it would be hard to make this py3-ready?

@@ +17,5 @@
> +        platform.python_version() )
> +    sys.exit(1)
> +
> +# Ideally we could load build/virtualenv/packages.txt. But, we need a
> +# supet set of what's in there, so we go with a one-off for now.

"superset"; though I'm wondering why it needs to be. (I.e., why we can't extend packages.txt.)

@@ +38,5 @@
> +    import mach.main
> +except ImportError:
> +    SEARCH_PATHS.reverse()
> +    for path in SEARCH_PATHS:
> +        sys.path.insert(0, os.path.join(our_dir, path))

sys.path[0:0] = [os.path.join(our_dir, path) for path in reversed(SEARCH_PATHS)]?

Maybe I'm just too fond of list comprehensions.

::: python/mach/mach/main.py
@@ +1,3 @@
> +# 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/.

Nit: the canonical header has 'file,' on the third line

@@ +6,5 @@
> +# (mach). It is packaged as a module because everything is a library.
> +
> +import argparse
> +import logging
> +import os.path

id.

@@ +13,5 @@
> +from mozbuild import config
> +from mozbuild.logger import LoggingManager
> +
> +# Import sub-command modules
> +# TODO do this via auto-discovery. Update README once this is done.

Bug #

@@ +36,5 @@
> +
> +  %(prog)s help          Show full help.
> +  %(prog)s build         Build the source tree.
> +  %(prog)s test          Run a test suite.
> +  %(prog)s xpcshell-test Run xpcshell test(s).

I don't like having both of those. (But I notice I said that before.)

@@ +46,5 @@
> +e.g. %(prog)s build --help
> +"""
> +
> +    def __init__(self, cwd):
> +        assert(os.path.isdir(cwd))

assert os.path.isdir(cwd)

(No parens)

@@ +55,5 @@
> +
> +        self.log_manager.register_structured_logger(self.logger)
> +
> +    def run(self, argv):
> +        """Runs the build tool with arguments specified."""

This is a pretty big function, and it does a lot of things. It would be nice if it were split up a bit.

@@ +59,5 @@
> +        """Runs the build tool with arguments specified."""
> +        parser = self.get_argument_parser()
> +
> +        if len(argv) == 0:
> +            parser.usage = Mach.USAGE

Don't we always want to do this?

@@ +61,5 @@
> +
> +        if len(argv) == 0:
> +            parser.usage = Mach.USAGE
> +            parser.print_usage()
> +            return 0

We don't always return something from this function, but when we do, we return 0. Should we return a value?

@@ +62,5 @@
> +        if len(argv) == 0:
> +            parser.usage = Mach.USAGE
> +            parser.print_usage()
> +            return 0
> +        elif argv[0] == 'help':

No else-after-return, please.

@@ +76,5 @@
> +            self.log_manager.add_json_handler(args.logfile)
> +
> +        log_level = logging.INFO
> +        if args.verbose:
> +            log_level = logging.DEBUG

log_level = logging.INFO if not args.verbose else logging.DEBUG

@@ +81,5 @@
> +
> +        self.log_manager.add_terminal_logging(level=log_level,
> +                write_interval=args.log_interval)
> +
> +        conf = config.BuildConfig(log_manager=self.log_manager)

BuildConfig has a log_manager argument?

@@ +89,5 @@
> +                print 'Settings path exists but is not a file: %s' % \
> +                    settings_file
> +                print 'Please delete the specified path or try again with ' \
> +                    'a new path'
> +                sys.exit(1)

So, earlier we returned, now we're sys.exiting, and below we're raising exceptions... Can we pick one type of control flow, please?

@@ +93,5 @@
> +                sys.exit(1)
> +
> +            conf.load_file(settings_file)
> +            self.log(logging.WARNING, 'config_load', {'file': settings_file},
> +                 'Loaded existing config file: {file}')

5-space indentation?

@@ +103,5 @@
> +                self.log(logging.WARNING, 'config_save',
> +                    {'file': settings_file}, 'Saved config file to {file}')
> +
> +        if args.objdir:
> +            raise Exception('TODO implement custom object directories.')

But you can put an objdir path in your mach.ini, I assume?

@@ +156,5 @@
> +          2) Environment variable MOZ_BUILD_CONFIG
> +          3) Default path
> +        """
> +
> +        p = os.path.join(self.cwd, 'mach.ini')

Did you see comment 33?

@@ +161,5 @@
> +
> +        if args.settings_file:
> +            p = args.settings_file
> +        elif 'MACH_SETTINGS_FILE' in os.environ:
> +            p = os.environ['MACH_SETTINGS_FILE']

Do we have anybody using this envvar?

@@ +178,5 @@
> +            dest='no_save_settings', action='store_true', default=False,
> +            help='When automatically generating settings, do not save the '
> +                'file.')
> +        settings_group.add_argument('--objdir', dest='objdir',
> +            metavar='FILENAME', help='Object directory to use. (ADVANCED).')

But we don't support this, right? Might be worth removing the option or adding a warning.

@@ +191,5 @@
> +        logging_group.add_argument('--log-interval', dest='log_interval',
> +            action='store_true', default=False,
> +            help='Prefix log line with interval from last message rather '
> +                'than relative time. Note that this is NOT execution time '
> +                'if there are parallel operations.')

(What's it good for then, if you don't mind me asking?)
(Assignee)

Comment 37

7 years ago
Posted patch Part 1: mach kernel, v1 (obsolete) — Splinter Review
This is the core of mach, the "kernel" if you will (sorry, I can't resist).

It does little by default. It's just the core logic. There are areas for improvement. These will be follow-up bugs.

Some of the TODOs will be addressed in subsequent patches on this bug. Others will be bugs (that haven't been filed yet).

I consider the sys.path hackery in the main driver program the biggest hack. Long term we'll need a better solution to how the Python virtualenv is populated. This involves modifying configure and sign-off from build system people. We'll cross that bridge later. For now, I'm comfortable with the hack. You should be too.

The terminal footer code isn't used yet. But, it's simple enough to review. It should basically be a rubber stamp.

Will have additional patches up shortly.
Attachment #663564 - Flags: review?(jhammel)
Attachment #663564 - Flags: feedback?(Ms2ger)
(Assignee)

Comment 38

7 years ago
Posted patch Part 2: Settings module, v1 (obsolete) — Splinter Review
Add a "settings" module to mach. This module provides functionality dealing with the mach settings. Currently it doesn't do much. There is only one setting being registered anywhere and even that setting is not being used. Yet, settings will be an important aspect of mach (they will be used to configure all the extra features that will land). So, this needs to land sometime.
Attachment #663567 - Flags: review?(jhammel)
(Assignee)

Comment 39

7 years ago
Provides commands for running tests. The set of tests it can run are what's exposed by mozbuild currently. Eventually we'll add more test suites. These are easy enough to do. If someone wants to throw up a patch, please do.

Also, mozbuild test running is only temporary until mozharness lands. Then, we should switch to that.
Attachment #663572 - Flags: review?(jhammel)
Attachment #663572 - Flags: feedback?(Ms2ger)
(Assignee)

Comment 40

7 years ago
This is everything that's left. I'm not asking for review yet because the mozbuild patches these talk to haven't gotten r+ yet.

Parts 1 through 3 are dependent on patches in bug 780329 that have already been granted review. So, once I get review on parts 1-3 on this bug and on bug 793061, mach can land in the tree. It will only support running tests. But, it's better than nothing. And, it unblocks new features from landing.
Attachment #651964 - Attachment is obsolete: true
Attachment #651964 - Flags: review?(vladimir)
(Assignee)

Comment 41

7 years ago
Incorporated some review feedback from Ms2ger. Also added some Python 3 support. However, pymake won't load in Python 3 so if you try to run mach with Python 3, you get an ImportError.
Attachment #663564 - Attachment is obsolete: true
Attachment #663564 - Flags: review?(jhammel)
Attachment #663564 - Flags: feedback?(Ms2ger)
Attachment #663581 - Flags: review?(jhammel)
Attachment #663581 - Flags: feedback?(Ms2ger)
(Assignee)

Comment 42

7 years ago
Python 3 compatibility.
Attachment #663567 - Attachment is obsolete: true
Attachment #663567 - Flags: review?(jhammel)
Attachment #663582 - Flags: review?(jhammel)
Attachment #663572 - Attachment is patch: true
Comment on attachment 663581 [details] [diff] [review]
Part 1: mach kernel, v2

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

I'm lovin' it

::: python/mach/README.rst
@@ +20,5 @@
> +the *mach* package which inherits from *ArgumentProvider*.
> +
> +Currently, you also need to hook up some plumbing in
> +*mach.main.Mach*. In the future, we hope to have automatic detection
> +of submodules.

Maybe add a bug number here?

@@ +99,5 @@
> +the mach CLI driver starts. Therefore, there is potential for *import bloat*.
> +
> +We want the CLI driver to load quickly. So, please delay load external modules
> +until they are actually required. In other words, don't use a global
> +*import* when you can import from inside a specific command's handler.

Nice readme!

::: python/mach/mach/main.py
@@ +31,5 @@
> +]
> +
> +# Settings for argument parser that don't get proxied to sub-module. i.e. these
> +# are things consumed by the driver itself.
> +IGNORED_ARGUMENTS = [

Nit: not sure about 'ignored'; maybe 'consumed' or 'handled'?

@@ +60,5 @@
> +  %(prog)s <command> --help
> +"""
> +
> +    def __init__(self, cwd):
> +        assert(os.path.isdir(cwd))

I tend to prefer |assert expr| over |assert(expr)|

@@ +129,5 @@
> +        self.logger.log(level, format_str,
> +            extra={'action': action, 'params': params})
> +
> +    def load_settings(self, args):
> +        """Determine what settings files apply and load them.

s/what/which/ sounds better to me. (Disclaimer: I'm not a native speaker)
Attachment #663581 - Flags: feedback?(Ms2ger) → feedback+
> s/what/which/ sounds better to me. (Disclaimer: I'm not a native speaker)

Native speaker concurs :)
Comment on attachment 663582 [details] [diff] [review]
Part 2: Settings module, v2

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

A bit of documentation about the goals of this module would be nice
Comment on attachment 663572 [details] [diff] [review]
Part 3: Testing module, v1

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

::: python/mach/mach/testing.py
@@ +41,5 @@
> +        xpcshell = self._spawn(XPCShellRunner)
> +        xpcshell.run_test(**params)
> +
> +    @staticmethod
> +    def populate_argparse(parser):

I still think this should just support

./mach mochitest-plain
./mach mochitest-plain --test-file=TEST

and the equivalents for other harnesses.
Comment on attachment 663575 [details] [diff] [review]
Part 4: Support for building the tree, v1

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

::: python/mach/mach/terminal.py
@@ +70,5 @@
>      def clear(self):
>          raise Exception('clear() must be implemented.')
>  
>      def draw(self):
>          raise Exception('draw() must be implemented.')

Oh, so TerminalFooter is supposed to be an abstract base class? How about marking those @abstract or however that's written in Python again?

@@ +88,5 @@
> +
> +    def clear(self):
> +        self._clear_lines(1)
> +
> +    def draw(self):

http://people.mozilla.org/~gszorc/mach00.png looks really nice, though I wonder about people who use a dark background for their terminal.

@@ +106,5 @@
> +        for action in self.ACTIONS:
> +            if action == self.action:
> +                parts.extend([self.t.yellow(action), ' '])
> +            else:
> +                parts.extend([action, ' '])

These two loops are essentially the same; how about a helper method?

@@ +118,5 @@
> +            total += 1
> +
> +            if state['start_time'] is None:
> +                pass
> +            elif state['start_time'] and state['finish_time'] is None:

Do you need the first half of the condition here?

@@ +120,5 @@
> +            if state['start_time'] is None:
> +                pass
> +            elif state['start_time'] and state['finish_time'] is None:
> +                in_progress += 1
> +                names.add(name)

Do you need in_progress, or can you use len(names)?

@@ +135,5 @@
> +            if in_progress > 0:
> +                parts.extend(['(', self.t.magenta(' '.join(names)), ')'])
> +
> +        line = ''.join(parts)
> +        self.fh.write(line)

Dunno if you need the 'line' variable

@@ +226,5 @@
> +
> +        if action in ('directory_start', 'directory_finish'):
> +            self.draw_directory_update(action, directory,
> +                    build.directories[directory])
> +            return

No refresh here?

Comment 48

7 years ago
Comment on attachment 663572 [details] [diff] [review]
Part 3: Testing module, v1

+        from mozbuild.testing.suite import Suite
...
+        from mozbuild.testing.mochitest import MochitestRunner

etc.  Why not have these as top-level imports?

I would also be inclined to have a single function for run_mochitest that run_mochitest_* can call

Other that that, looks fine
Attachment #663572 - Flags: review?(jhammel) → review+

Comment 49

7 years ago
Comment on attachment 663581 [details] [diff] [review]
Part 1: mach kernel, v2

It would sure be nice if .hgignore and .gitignore could be kept in sync without manual effort, though I don't know how.  

diff --git a/mach b/mach
new file mode 100755
--- /dev/null
+++ b/mach

In general, I would prefer `mach.py` for the name.  

+# TODO We should integrate with the in-tree virtualenv configuration better.

Yes plz.

+Currently, you also need to hook up some plumbing in
+*mach.main.Mach*. In the future, we hope to have automatic detection
+of submodules.

Yes plz.

+is using the \*\*kwargs notation to call the defined method.

I would prefer "...the ``**kwargs`` notation..."

wfm
Attachment #663581 - Flags: review?(jhammel) → review+

Comment 50

7 years ago
Comment on attachment 663582 [details] [diff] [review]
Part 2: Settings module, v2

+    def list(self):

A docstring would be appreciated and 'write' might be a better name.
Attachment #663582 - Flags: review?(jhammel) → review+
(Assignee)

Comment 51

7 years ago
(In reply to :Ms2ger from comment #43)
 
> ::: python/mach/README.rst
> @@ +20,5 @@
> > +the *mach* package which inherits from *ArgumentProvider*.
> > +
> > +Currently, you also need to hook up some plumbing in
> > +*mach.main.Mach*. In the future, we hope to have automatic detection
> > +of submodules.
> 
> Maybe add a bug number here?

jhammel: what's that Python packaging feature you were telling me about that could be useful here? I /think/ you were saying there's some kind of hook we can leverage in setup.py/distutils/site.py that we could leverage here to facilitate autodiscovery? Of course, that would assume we are using setup.py. I recently changed our virtualenv population script to create raw .pth files because setup.py was too heavyweight for most of what we were doing...
(Assignee)

Comment 52

7 years ago
(In reply to :Ms2ger from comment #46)
> ::: python/mach/mach/testing.py
> @@ +41,5 @@
> > +        xpcshell = self._spawn(XPCShellRunner)
> > +        xpcshell.run_test(**params)
> > +
> > +    @staticmethod
> > +    def populate_argparse(parser):
> 
> I still think this should just support
> 
> ./mach mochitest-plain
> ./mach mochitest-plain --test-file=TEST
> 
> and the equivalents for other harnesses.

I think positional arguments are better. They are less typing, after all.

My eventual goal is to expose a single "test" subcommand that accepts either special values (xpcshell-tests, mochitest-plain) and/or paths to test files or directories. Mach will figure out what kind of test you specified by the path. We might be able to do this today, but it's a bit hacky since it relies on heuristics and some rather ugly Makefile traversal + parsing. Once bug 784841 matures, test files will be more easily extractable from the build config, making mapping paths to test suites much, much easier.
(In reply to Gregory Szorc [:gps] from comment #52)
> (In reply to :Ms2ger from comment #46)
> > ::: python/mach/mach/testing.py
> > @@ +41,5 @@
> > > +        xpcshell = self._spawn(XPCShellRunner)
> > > +        xpcshell.run_test(**params)
> > > +
> > > +    @staticmethod
> > > +    def populate_argparse(parser):
> > 
> > I still think this should just support
> > 
> > ./mach mochitest-plain
> > ./mach mochitest-plain --test-file=TEST
> > 
> > and the equivalents for other harnesses.
> 
> I think positional arguments are better. They are less typing, after all.

Agreed. I only used --test-file because I thought that's what the patch currently supports.

> My eventual goal is to expose a single "test" subcommand that accepts either
> special values (xpcshell-tests, mochitest-plain) and/or paths to test files
> or directories. Mach will figure out what kind of test you specified by the
> path. We might be able to do this today, but it's a bit hacky since it
> relies on heuristics and some rather ugly Makefile traversal + parsing. Once
> bug 784841 matures, test files will be more easily extractable from the
> build config, making mapping paths to test suites much, much easier.

That sounds so nice I couldn't have imagined it myself... Stockholm syndrome, I guess :)

However, to make the change, Maybe we should have only

./mach SUITE
./mach SUITE PATH

for now, and move to

./mach SUITE
./mach PATH

when we can. (And keep ./mach SUITE PATH supported for a bit, I guess.) I don't think we should support --suite / --test-path / --test-path=all now, if they're not part of your eventual goal. I suspect people will jump onto mach as soon as it lands, and they will probably discover and that using those features, and will need to change their muscle memory twice instead of once. (I know my proposal still causes that for individual tests, but at least omitting the SUITE part shouldn't hurt too much.)
(Assignee)

Comment 54

7 years ago
(In reply to :Ms2ger from comment #53)
> However, to make the change, Maybe we should have only
> 
> ./mach SUITE
> ./mach SUITE PATH
> 
> for now, and move to
> 
> ./mach SUITE
> ./mach PATH
> 
> when we can. (And keep ./mach SUITE PATH supported for a bit, I guess.)

./mach SUITE PATH *is* how it is implemented today! Admittedly, it's a little hard to follow in the argparse code. Essentially we register a separate sub-parser (a sub-command) for each test suite. Each sub-parser has an *optional* "test_file" argument with a special default value of "all." You know it is optional because the name of the argument is not prefixed with a '-' and nargs='?'.
(Assignee)

Updated

7 years ago
Blocks: 794506
(Assignee)

Updated

7 years ago
Blocks: 794509
(Assignee)

Comment 55

7 years ago
(In reply to Jeff Hammel [:jhammel] from comment #48)
> Comment on attachment 663572 [details] [diff] [review]
> Part 3: Testing module, v1
> 
> +        from mozbuild.testing.suite import Suite
> ...
> +        from mozbuild.testing.mochitest import MochitestRunner
> 
> etc.  Why not have these as top-level imports?

This avoids import bloat issues. When the mach driver is invoked, we don't want it importing the world and things it probably won't use. Lazy loading may reduce start-up lag.

(In reply to Jeff Hammel [:jhammel] from comment #49)
> Comment on attachment 663581 [details] [diff] [review]
> Part 1: mach kernel, v2

> 
> diff --git a/mach b/mach
> new file mode 100755
> --- /dev/null
> +++ b/mach
> 
> In general, I would prefer `mach.py` for the name.  

I am going to override you on this one. The .py does nothing. The shebang in the file is all that matters. People shouldn't care that mach is implemented in Python. If nothing else, it's 3 less characters of typing :)
 
> +# TODO We should integrate with the in-tree virtualenv configuration better.
> 
> Yes plz.

Bug 794506.

> 
> +Currently, you also need to hook up some plumbing in
> +*mach.main.Mach*. In the future, we hope to have automatic detection
> +of submodules.
> 
> Yes plz.

Bug 794509.
(Assignee)

Comment 56

7 years ago
Kernel, settings, and testing landed \o/

https://hg.mozilla.org/mozilla-central/rev/4621dc706abc
https://hg.mozilla.org/mozilla-central/rev/c9294c9df7c1
https://hg.mozilla.org/mozilla-central/rev/2359243ee2b1

Will leave open to track landing the building integration.

The big news is the floodgates are open. We now have a logical and obvious place to discover in-tree developer tools. I expect great things. I hope a lot of bugs under bug 774108 start moving now.
Whiteboard: [leave open]
(Assignee)

Comment 57

7 years ago
Adding some simple APIs to mozbuild to control how make is executed. I like control.
Attachment #665050 - Flags: review?(jhammel)
(Assignee)

Comment 58

7 years ago
Robust tree building code is waiting on patches in bug 780329. These probably won't land for at least a few weeks. In the mean time, I'd like to land a "build" mach command so it's there and so mach is a little more valuable.

The big win of this command is compiler warnings recording. We already have the code in mozbuild to parse compiler warnings from build output. We might as well put it to use!

This patch lacks all of the contentious build system integration issues holding up bug 780329. It also doesn't contain the (somewhat complicated) terminal interaction code. This patch is small. It's simple. It works. I don't think there's a good reason to not check this in.

See the commit message for notes about DRY violations and the temporary nature of this code.

If this lands, I figure I'll close this bug and bug 780329 and open up new ones to track the robust build system integration as a follow-up.
Attachment #663575 - Attachment is obsolete: true
Attachment #665055 - Flags: review?(jhammel)
(Assignee)

Comment 59

7 years ago
I can haz bugzilla component \o/
Assignee: gps → nobody
Component: Build Config → mach
Assignee: nobody → gps
Attachment #663572 - Flags: feedback?(Ms2ger)

Comment 60

7 years ago
(In reply to Gregory Szorc [:gps] from comment #51)
> (In reply to :Ms2ger from comment #43)
>  
> > ::: python/mach/README.rst
> > @@ +20,5 @@
> > > +the *mach* package which inherits from *ArgumentProvider*.
> > > +
> > > +Currently, you also need to hook up some plumbing in
> > > +*mach.main.Mach*. In the future, we hope to have automatic detection
> > > +of submodules.
> > 
> > Maybe add a bug number here?
> 
> jhammel: what's that Python packaging feature you were telling me about that
> could be useful here? I /think/ you were saying there's some kind of hook we
> can leverage in setup.py/distutils/site.py that we could leverage here to
> facilitate autodiscovery? Of course, that would assume we are using
> setup.py. I recently changed our virtualenv population script to create raw
> .pth files because setup.py was too heavyweight for most of what we were
> doing...

Yes, this is a setuptools feature call entry points that requires the setup.py actually be run: http://packages.python.org/distribute/setuptools.html#entry-points

Comment 61

7 years ago
(In reply to :Ms2ger from comment #53)
> (In reply to Gregory Szorc [:gps] from comment #52)
> > (In reply to :Ms2ger from comment #46)
> > > ::: python/mach/mach/testing.py
> > > @@ +41,5 @@
> > > > +        xpcshell = self._spawn(XPCShellRunner)
> > > > +        xpcshell.run_test(**params)
> > > > +
> > > > +    @staticmethod
> > > > +    def populate_argparse(parser):
> > > 
> > > I still think this should just support
> > > 
> > > ./mach mochitest-plain
> > > ./mach mochitest-plain --test-file=TEST
> > > 
> > > and the equivalents for other harnesses.
> > 
> > I think positional arguments are better. They are less typing, after all.
> 
> Agreed. I only used --test-file because I thought that's what the patch
> currently supports.
> 
> > My eventual goal is to expose a single "test" subcommand that accepts either
> > special values (xpcshell-tests, mochitest-plain) and/or paths to test files
> > or directories. Mach will figure out what kind of test you specified by the
> > path. We might be able to do this today, but it's a bit hacky since it
> > relies on heuristics and some rather ugly Makefile traversal + parsing. Once
> > bug 784841 matures, test files will be more easily extractable from the
> > build config, making mapping paths to test suites much, much easier.
> 
> That sounds so nice I couldn't have imagined it myself... Stockholm
> syndrome, I guess :)
> 
> However, to make the change, Maybe we should have only
> 
> ./mach SUITE
> ./mach SUITE PATH
> 
> for now, and move to
> 
> ./mach SUITE
> ./mach PATH
> 
> when we can. (And keep ./mach SUITE PATH supported for a bit, I guess.) I
> don't think we should support --suite / --test-path / --test-path=all now,
> if they're not part of your eventual goal. I suspect people will jump onto
> mach as soon as it lands, and they will probably discover and that using
> those features, and will need to change their muscle memory twice instead of
> once. (I know my proposal still causes that for individual tests, but at
> least omitting the SUITE part shouldn't hurt too much.)

I am for 

./mach SUITE 
./mach SUITE PATH 

for the forseeable future until we have something in-tree, e.g. universal manifests, that tells what harness (and potentially arguments) are needed for each path.  The alternative

./mach PATH

seems too magical otherwise </IMHO>

Updated

7 years ago
Attachment #665050 - Flags: review?(jhammel) → review+

Comment 62

7 years ago
Comment on attachment 665055 [details] [diff] [review]
Part 4b: Minimal build and compiler warning commands, v1

lgtm
Attachment #665055 - Flags: review?(jhammel) → review+
(Assignee)

Comment 63

7 years ago
https://hg.mozilla.org/mozilla-central/rev/cc0a9e9c7370
https://hg.mozilla.org/mozilla-central/rev/4a3292507308

And with crude build support landed, I'm going to mark this resolved. I will file a follow-up to integrate the "real" build support (the thing with progress indicators, coloring, performance data, etc). But, that should just be a drop-in replacement.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla18
Alias: mach

Updated

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