987902 - Add a "doctor" mach command

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Gregory Szorc [:gps]]

We have a number of suggestions we like to make to users about getting the most out of their Firefox build and productivity:

* Run mercurial-setup periodically
* Run bootstrap periodically
* Case sensitive filesystem foo
* Indexing services and their impact on performance
* Insufficient memory
* Not enough CPU cores
* Slow I/O
* Filesystem settings such as atime or 8dot3name

Warning about these things on every command or build is annoying. Warnings have been backed out because of this.

Homebrew has a "doctor" command that detects common issues and tells you how to fix them (it may even prompt you to fix automatically). I think this is a good idea worth borrowing for mach.

Let's have a "doctor" command that performs common checks on the system and whatnot and helps people get the most out of their time.

Comment 1

[Mitchell Field [:Mitch]]

Attachment: Patch v1

Windows implementation of |mach doctor|. If it finds fixable problems it suggests running it as admin with --fix.
It currently doesn't do any mozconfig magic to detect srcdir and objdir; more-accurate assessments may need them passed in as arguments.

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8558461 [details] [diff] [review]
Patch v1

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

This patch made my week! I really like where you are going!

Many of these checks apply to different OSs. I'd just rename doctor_win.py to doctor.py and establish skipping for tests that aren't relevant for a certain platform. Once you plug in psutil APIs, I think you'll find that pretty much everything except 8.3 "just works" on other operating systems. I'll be more than happy to test things on OS X and Linux if you don't have access to these operating systems.

Possible follow-up idea: |mach doctor| should exit 0 if everything is good and exit 1 if not. That opens up possibilities for e.g. integrating |mach doctor| into a configure check.

::: python/mozbuild/mozbuild/doctor/doctor_win.py
@@ +14,5 @@
> +# Minimum recommended logical processors in system.
> +PROCESSORS_THRESHOLD = 4
> +
> +# Minimum recommended total system memory, in gigabytes.
> +MEMORY_THRESHOLD = 8

We may want this to be slightly less than 8 GB because some machines have things like video memory take away from system memory. They may have 8 GB installed, but only say 7.8 may be addressable to the OS.

@@ +17,5 @@
> +# Minimum recommended total system memory, in gigabytes.
> +MEMORY_THRESHOLD = 8
> +
> +# Minimum recommended free space on each disk, as a percentage.
> +FREESPACE_THRESHOLD = 10

I'm not a fan of this. 10% free on a 1 TB disk is 100 GB, which is probably 10x more than you would ever need to do Firefox development unless you are doing some real crazy stuff. Can we make this check in terms of absolutes? How about 10 GB?

@@ +74,5 @@
> +            command = 'wmic cpu get NumberOfLogicalProcessors'.split(' ')
> +            wmic_output = subprocess.check_output(command)
> +            for line in wmic_output.splitlines():
> +                if line and not line.startswith('NumberOfLogicalProcessors'):
> +                    processor_count += int(line)

The psutil package is available to mach and it contains an API to access physical process count. It even works on non-Windows machines.

@@ +75,5 @@
> +            wmic_output = subprocess.check_output(command)
> +            for line in wmic_output.splitlines():
> +                if line and not line.startswith('NumberOfLogicalProcessors'):
> +                    processor_count += int(line)
> +        except:

Nit: bareword except is evil because it catches things like KeyboardInterrupt (ctrl+c). Always use "except Exception" instead.

@@ +95,5 @@
> +
> +    @property
> +    def memory(self):
> +        try:
> +            command = 'wmic computersystem get TotalPhysicalMemory'.split(' ')

There is also a psutil API for this.

@@ +123,5 @@
> +    def storage_freespace(self):
> +        self.disks = {}
> +        results = []
> +        try:
> +            command = 'wmic logicaldisk get caption,freespace,size'.split(' ')

There's also a psutil API for this :)

@@ +174,5 @@
> +        if system8dot3 == 1:
> +            status = 'GOOD'
> +            desc = '8dot3 disabled systemwide'
> +        elif system8dot3 == 0:
> +            if (fix == True):

Nit: parens not needed in Python.

::: python/mozbuild/mozbuild/mach_commands.py
@@ +600,5 @@
> +        description='')
> +    @CommandArgument('--srcdir', default=None,
> +        help='Specify source directory.')
> +    @CommandArgument('--objdir', default=None,
> +        help='Specify object directory.')

You can easily grab these from self.topsrcdir and self.topobjdir. I don't even think arguments are necessary.

Comment 3

[Mitchell Field [:Mitch]]

Thanks for the feedback.

>Once you plug in psutil APIs

>ImportError: No module named _psutil_windows

The psutil Python extension needs to be built and it goes downhill from there.

>$ setup.py build
>running build
>running build_py
>creating build\lib.win32-2.7
>creating build\lib.win32-2.7\psutil
>copying psutil\_common.py -> build\lib.win32-2.7\psutil
>copying psutil\_compat.py -> build\lib.win32-2.7\psutil
>copying psutil\_psbsd.py -> build\lib.win32-2.7\psutil
>copying psutil\_pslinux.py -> build\lib.win32-2.7\psutil
>copying psutil\_psosx.py -> build\lib.win32-2.7\psutil
>copying psutil\_psposix.py -> build\lib.win32-2.7\psutil
>copying psutil\_pssunos.py -> build\lib.win32-2.7\psutil
>copying psutil\_pswindows.py -> build\lib.win32-2.7\psutil
>copying psutil\__init__.py -> build\lib.win32-2.7\psutil
>running build_ext
>building '_psutil_windows' extension
>error: Unable to find vcvarsall.bat

Even the "Microsoft Visual C++ Compiler for Python 2.7" (http://www.microsoft.com/en-us/download/details.aspx?id=44266), specifically for this situation, failed to build psutil.

Comment 4

[Gregory Szorc [:gps]]

Add |self._activate_virtualenv()| to your mach command handler and delay import any module importing psutil until after that call. The psutil build errors should magically go away (mach does magic behind the scenes).

https://hg.mozilla.org/mozilla-central/file/193c4c5c7ec2/tools/docs/mach_commands.py#l30

Comment 5

[Mitchell Field [:Mitch]]

Attachment: Patch v2

This makes use of psutil, and tested fine on Windows and my Fedora VM. If you could test more on Linux and OS X that would be great.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8563391 [details] [diff] [review]
Patch v2

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

This is shaping up *very* nicely!

When looking at the feedback, don't be afraid to cut features in order to get this landed. Something is better than nothing. We can always iterate on improvements later.

::: python/mozbuild/mozbuild/doctor.py
@@ +52,5 @@
> +                fixable = True
> +            if result.get('denied'):
> +                denied = True
> +        if denied:
> +            print 'run \'mach doctor --fix\' AS ADMIN to re-attempt fixing your system'

Nit: use print()

Also, mix ' and " to avoid escaping. Unlike languages like PHP and Perl, ' and " mean the same thing in Python: no interpolation either way.

@@ +171,5 @@
> +            if system8dot3 == 1:
> +                status = 'GOOD'
> +                desc = '8dot3 disabled systemwide'
> +            elif system8dot3 == 0:
> +                if self.fix == True:

if self.fix:

@@ +200,5 @@
> +                    results.append(self.check_disk_8dot3(self.srcdir))
> +                if self.objdir and self.srcdir != self.objdir:
> +                    results.append(self.check_disk_8dot3(self.objdir))
> +        else:
> +            results.append({'status':'SKIPPED'})

You should use an early return here and drop the indent from the big block above.

@@ +271,5 @@
> +
> +    def check_path_lastaccess(self, path, purpose):
> +        mount = self.getmount(path)
> +        partitions = psutil.disk_partitions()
> +        atime_opts = set(['atime', 'noatime', 'relatime', 'norelatime'])

We require Python 2.7, so you can just do {'atime', 'noative', 'relatime', 'norelatime'}.

@@ +286,5 @@
> +            if self.platform == 'linux':
> +                option = 'noatime/relatime'
> +            else:
> +                option = 'noatime'
> +            desc = '%s = %s\n%s has no explicit %s mount option' % (

I'm getting this code path on OS X resulting in:

srcdir = /Users/gps/src/firefox
/ has no explicit noatime mount option...                   UNSURE

objdir = /Users/gps/src/firefox/obj-firefox.noindex
/ has no explicit noatime mount option...                   UNSURE

Here is my partition info:

>>> psutil.disk_partitions()
[sdiskpart(device='/dev/disk1', mountpoint='/', fstype='hfs', opts='rw,exported,local,rootfs,dovolfs,journaled,multilabel')]

You may find https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man8/mount.8.html useful.

@@ +307,5 @@
> +        if self.platform != 'win':
> +            return {'status':'SKIPPED'}
> +        OSTYPE = os.environ['OSTYPE']
> +        SHELL = os.environ['SHELL']
> +        if not OSTYPE == 'msys' or '/mozilla-build/msys/bin/sh' not in SHELL:

I don't think we can rely on /mozilla-build being the install location. We'll need to do this another way.

You may want to punt on this check for the first version.

Or, there is currently work in bug 791486 to ship the next version of MozillaBuild. If you would like the environment to emit a signal we can check for, now is a good time to ask for that...

::: python/mozbuild/mozbuild/mach_commands.py
@@ +607,5 @@
> +    @CommandArgument('--fix', default=None, action='store_true',
> +        help='Attempt to fix found problems.')
> +    def doctor(self, fix=None):
> +        self._activate_virtualenv()
> +        self.virtualenv_manager.install_pip_package('psutil==2.2.1')

No need to install psutil: it comes with the virtualenv created by the build system.

Comment 7

[Mitchell Field [:Mitch]]

Attachment: Patch v3

I've classed lack of "noatime" on OS X as bad. A "fix" can be implemented later, based on the steps here:
http://blog.alutam.com/2012/04/01/optimizing-macos-x-lion-for-ssd/#noatime

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8565844 [details] [diff] [review]
Patch v3

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

This review is a bit more low-level, since most of the high-level stuff is taken care of.

There is a lot of munging of the result dicts. You may consider inventing a class and have it do more work for you. This could be punted to a follow-up.

This is getting real close. Sorry for throwing the fix prompting feedback on you this late into review. I was focused on other aspects during first review passes. This review is comprehensive and I don't think I'll find any new significant concerns.

If you would like to land something, feel free to strip fixing support and defer that to a follow-up. Putting the code behind an "if False" instead of "if self.fix" would be sufficient to get my r+.

::: python/mozbuild/mozbuild/doctor.py
@@ +31,5 @@
> +        self.fix = fix
> +        self.results = []
> +
> +    def check_all(self):
> +        checks = [

A good followup would be to use a decorator to declare check functions. That would eliminate keeping this list in sync.

@@ +41,5 @@
> +            'mozillabuild'
> +        ]
> +        for check in checks:
> +            self.report(getattr(self, check))
> +        not_good = False

Let's avoid the double inversion and flip this to "good".

@@ +62,5 @@
> +        path = os.path.realpath(mozpath.abspath(path))
> +        while path != os.path.sep:
> +            if os.path.ismount(path):
> +                return path
> +            path = mozpath.abspath(mozpath.join(path, os.pardir))

The common way of writing this is:

while path != '/':
    # test
    path = mozpath.dirname(path)

@@ +78,5 @@
> +            print('%s...\t%s\n' % (
> +                   result.get('desc', ''),
> +                   status
> +                )
> +            ).expandtabs(60)

60 seems a bit excessive. How about 40?

@@ +103,5 @@
> +            status = 'GOOD'
> +            desc = '%d logical processors detected, >=%d' % (
> +                cpu_count, PROCESSORS_THRESHOLD
> +            )
> +        return {'status':status, 'desc':desc}

Nit: put space after :

@@ +128,5 @@
> +        results = []
> +        if self.srcdir:
> +            results.append(self.check_path_freespace(self.srcdir, 'srcdir'))
> +        if self.objdir:
> +            results.append(self.check_path_freespace(self.objdir, 'objdir'))

A good follow-up would be to consolidate the results for the common case where the source dir is on the same volume as the objdir.

@@ +175,5 @@
> +        elif system8dot3 == 0:
> +            if self.fix:
> +                try:
> +                    command = 'fsutil behavior set disable8dot3 1'.split(' ')
> +                    fsutil_output = subprocess.check_output(command)

I'm not a huge fan of running commands like this without explicit user consent.

Could you please throw a y/n prompt in here? The prompt should contain info (possibly a URL) describing the impact of taking this change.

@@ +244,5 @@
> +            elif disablelastaccess == 0:
> +                if self.fix:
> +                    try:
> +                        command = 'fsutil behavior set disablelastaccess 1'.split(' ')
> +                        fsutil_output = subprocess.check_output(command)

Ditto about doing this without user consent.

@@ +310,5 @@
> +            return {'desc':'not running under MozillaBuild'}
> +        try:
> +            version_file = open(mozpath.join(MOZILLABUILD, 'VERSION'), 'r')
> +            version = version_file.readline()
> +            version_file.close()

with open(mozpath.join(MOZILLABUILD, 'VERSION'), 'r') as fh:
    version = fh.readline()

@@ +311,5 @@
> +        try:
> +            version_file = open(mozpath.join(MOZILLABUILD, 'VERSION'), 'r')
> +            version = version_file.readline()
> +            version_file.close()
> +            if len(version) == 0:

if not version:

::: python/mozbuild/mozbuild/mach_commands.py
@@ +605,5 @@
> +    @Command('doctor', category='devenv',
> +        description='')
> +    @CommandArgument('--fix', default=None, action='store_true',
> +        help='Attempt to fix found problems.')
> +    def doctor(self, fix=None):

The default value for fix should probably be False.

Comment 9

[Mitchell Field [:Mitch]]

Attachment: Patch v4

(In reply to Gregory Szorc [:gps] from comment #8)
> Comment on attachment 8565844 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 8565844 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There is a lot of munging of the result dicts. You may consider inventing a
> class and have it do more work for you. This could be punted to a follow-up.

Punted.

> If you would like to land something, feel free to strip fixing support and
> defer that to a follow-up. Putting the code behind an "if False" instead of
> "if self.fix" would be sufficient to get my r+.

Falsed.

> A good followup would be to use a decorator to declare check functions. That
> would eliminate keeping this list in sync.

Punted.

> The common way of writing this is:
> 
> while path != '/':
>     # test
>     path = mozpath.dirname(path)

On Windows that ends up with a drive letter with no slash like "c:". os.path.ismount('c:') == False.

> A good follow-up would be to consolidate the results for the common case
> where the source dir is on the same volume as the objdir.

Done.

> Could you please throw a y/n prompt in here? The prompt should contain info
> (possibly a URL) describing the impact of taking this change.

Done.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8573180 [details] [diff] [review]
Patch v4

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

This looks good for a landable version!

I'll go ahead and land this for you.

Thanks for your persistence with my review feedback. People tend to be very sensitive about mucking with system configs and I hope you understand my caution towards rolling this out. With the prompt support, it should be relatively easy to enable fix support in a follow-up.

::: python/mozbuild/mozbuild/doctor.py
@@ +39,5 @@
> +hour. Backup programs that rely on this feature may be affected.
> +https://technet.microsoft.com/en-us/library/cc785435.aspx
> +'''
> +
> +class Doctor:

class Doctor(object):

(there is such a thing as old-style and new-style classes in Python and the "object" part makes them new-style)

Comment 11

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08a5abd9d252

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/08a5abd9d252