Beef up how `qmk doctor` works. (#7375)

* Beef up how `qmk doctor` works.

* improve the `git submodule status` parsing. h/t @erovia

* Fix whitespace and imports

* yapf

* Add documentation for the new doctor functionality

* Replace type_unchanged() with str()

* remove unused modules

* Update lib/python/qmk/cli/doctor.py

Co-Authored-By: Erovia <Erovia@users.noreply.github.com>

Co-authored-by: Erovia <Erovia@users.noreply.github.com>

docs/cli.md

@@ -141,14 +141,28 @@ qmk docs [-p PORT]
 
 ## `qmk doctor`
 
-This command examines your environment and alerts you to potential build or flash problems.
+This command examines your environment and alerts you to potential build or flash problems. It can fix many of them if you want it to.
 
 **Usage**:
 
 ```
-qmk doctor
+qmk doctor [-y] [-n]
 ```
 
+**Examples**:
+
+Check your environment for problems and prompt to fix them:
+
+    qmk doctor
+
+Check your environment and automatically fix any problems found:
+
+    qmk doctor -y
+
+Check your environment and report problems only:
+
+    qmk doctor -n
+
 ## `qmk json-keymap`
 
 Creates a keymap.c from a QMK Configurator export.

lib/python/qmk/cli/doctor.py

@@ -1,14 +1,18 @@
-"""QMK Python Doctor
+"""QMK Doctor
 
-Check up for QMK environment.
+Check out the user's QMK environment and make sure it's ready to compile.
 """
-import os
 import platform
 import shutil
 import subprocess
-import glob
+from pathlib import Path
 
 from milc import cli
+from qmk import submodules
+from qmk.questions import yesno
+
+ESSENTIAL_BINARIES = ['dfu-programmer', 'avrdude', 'dfu-util', 'avr-gcc', 'arm-none-eabi-gcc', 'bin/qmk']
+ESSENTIAL_SUBMODULES = ['lib/chibios', 'lib/lufa']
 
 
 def _udev_rule(vid, pid=None):
@@ -20,45 +24,42 @@ def _udev_rule(vid, pid=None):
         return 'SUBSYSTEMS=="usb", ATTRS{idVendor}=="%s", MODE:="0666"' % vid
 
 
-@cli.subcommand('Basic QMK environment checks')
+def check_binaries():
-def doctor(cli):
+    """Iterates through ESSENTIAL_BINARIES and tests them.
-    """Basic QMK environment checks.
-
-    This is currently very simple, it just checks that all the expected binaries are on your system.
-
-    TODO(unclaimed):
-        * [ ] Compile a trivial program with each compiler
     """
-    cli.log.info('QMK Doctor is checking your environment.')
-
-    # Make sure the basic CLI tools we need are available and can be executed.
-    binaries = ['dfu-programmer', 'avrdude', 'dfu-util', 'avr-gcc', 'arm-none-eabi-gcc', 'bin/qmk']
     ok = True
 
-    for binary in binaries:
+    for binary in ESSENTIAL_BINARIES:
-        res = shutil.which(binary)
+        if not is_executable(binary):
-        if res is None:
-            cli.log.error("{fg_red}QMK can't find %s in your path.", binary)
-            ok = False
-        else:
-            check = subprocess.run([binary, '--version'], stdout=subprocess.PIPE, stderr=subprocess.PIPE, timeout=5)
-            if check.returncode in [0, 1]:
-                cli.log.info('Found {fg_cyan}%s', binary)
-            else:
-                cli.log.error("{fg_red}Can't run `%s --version`", binary)
             ok = False
 
-    # Determine our OS and run platform specific tests
+    return ok
-    OS = platform.system()  # noqa (N806), uppercase name is ok in this instance
 
-    if OS == "Darwin":
-        cli.log.info("Detected {fg_cyan}macOS.")
 
-    elif OS == "Linux":
+def check_submodules():
-        cli.log.info("Detected {fg_cyan}Linux.")
+    """Iterates through all submodules to make sure they're cloned and up to date.
-        # Checking for udev rules
+    """
-        udev_dir = "/etc/udev/rules.d/"
+    ok = True
-        # These are the recommended udev rules
+
+    for submodule in submodules.status().values():
+        if submodule['status'] is None:
+            if submodule['name'] in ESSENTIAL_SUBMODULES:
+                cli.log.error('Submodule %s has not yet been cloned!', submodule['name'])
+                ok = False
+            else:
+                cli.log.warn('Submodule %s is not available.', submodule['name'])
+        elif not submodule['status']:
+            if submodule['name'] in ESSENTIAL_SUBMODULES:
+                cli.log.warn('Submodule %s is not up to date!')
+
+    return ok
+
+
+def check_udev_rules():
+    """Make sure the udev rules look good.
+    """
+    ok = True
+    udev_dir = Path("/etc/udev/rules.d/")
     desired_rules = {
         'dfu': {_udev_rule("03eb", "2ff4"), _udev_rule("03eb", "2ffb"), _udev_rule("03eb", "2ff0")},
         'tmk': {_udev_rule("feed")},
@@ -67,12 +68,13 @@ def doctor(cli):
         'caterina': {'ATTRS{idVendor}=="2a03", ENV{ID_MM_DEVICE_IGNORE}="1"', 'ATTRS{idVendor}=="2341", ENV{ID_MM_DEVICE_IGNORE}="1"'},
     }
 
-        if os.path.exists(udev_dir):
+    if udev_dir.exists():
-            udev_rules = [rule for rule in glob.iglob(os.path.join(udev_dir, "*.rules")) if os.path.isfile(rule)]
+        udev_rules = [str(rule_file) for rule_file in udev_dir.glob('*.rules')]
-            # Collect all rules from the config files
         current_rules = set()
-            for rule in udev_rules:
+
-                with open(rule, "r") as fd:
+        # Collect all rules from the config files
+        for rule_file in udev_rules:
+            with open(rule_file, "r") as fd:
                 for line in fd.readlines():
                     line = line.strip()
                     if not line.startswith("#") and len(line):
@@ -83,22 +85,133 @@ def doctor(cli):
             if not rules.issubset(current_rules):
                 # If the rules for catalina are not present, check if ModemManager is running
                 if bootloader == "caterina":
-                        if shutil.which("systemctl"):
+                    if check_modem_manager():
-                            mm_check = subprocess.run(["systemctl", "--quiet", "is-active", "ModemManager.service"], timeout=10)
-                            if mm_check.returncode == 0:
                         ok = False
                         cli.log.warn("{bg_yellow}Detected ModemManager without udev rules. Please either disable it or set the appropriate udev rules if you are using a Pro Micro.")
-                        else:
-                            cli.log.warn("Can't find systemctl to check for ModemManager.")
                 else:
                     cli.log.warn("{bg_yellow}Missing udev rules for '%s' boards. You'll need to use `sudo` in order to flash them.", bootloader)
 
+    return ok
+
+
+def check_modem_manager():
+    """Returns True if ModemManager is running.
+    """
+    if shutil.which("systemctl"):
+        mm_check = subprocess.run(["systemctl", "--quiet", "is-active", "ModemManager.service"], timeout=10)
+        if mm_check.returncode == 0:
+            return True
+
     else:
-        cli.log.info("Assuming {fg_cyan}Windows.")
+        cli.log.warn("Can't find systemctl to check for ModemManager.")
+
+
+def is_executable(command):
+    """Returns True if command exists and can be executed.
+    """
+    # Make sure the command is in the path.
+    res = shutil.which(command)
+    if res is None:
+        cli.log.error("{fg_red}Can't find %s in your path.", command)
+        return False
+
+    # Make sure the command can be executed
+    check = subprocess.run([command, '--version'], stdout=subprocess.PIPE, stderr=subprocess.PIPE, timeout=5)
+    if check.returncode in [0, 1]:  # Older versions of dfu-programmer exit 1
+        cli.log.debug('Found {fg_cyan}%s', command)
+        return True
+
+    cli.log.error("{fg_red}Can't run `%s --version`", command)
+    return False
+
+
+def os_test_linux():
+    """Run the Linux specific tests.
+    """
+    cli.log.info("Detected {fg_cyan}Linux.")
+    ok = True
+
+    if not check_udev_rules():
+        ok = False
+
+    return ok
+
+
+def os_test_macos():
+    """Run the Mac specific tests.
+    """
+    cli.log.info("Detected {fg_cyan}macOS.")
+
+    return True
+
+
+def os_test_windows():
+    """Run the Windows specific tests.
+    """
+    cli.log.info("Detected {fg_cyan}Windows.")
+
+    return True
+
+
+@cli.argument('-y', '--yes', action='store_true', arg_only=True, help='Answer yes to all questions.')
+@cli.argument('-n', '--no', action='store_true', arg_only=True, help='Answer no to all questions.')
+@cli.subcommand('Basic QMK environment checks')
+def doctor(cli):
+    """Basic QMK environment checks.
+
+    This is currently very simple, it just checks that all the expected binaries are on your system.
+
+    TODO(unclaimed):
+        * [ ] Compile a trivial program with each compiler
+    """
+    cli.log.info('QMK Doctor is checking your environment.')
+    ok = True
+
+    # Determine our OS and run platform specific tests
+    OS = platform.system()  # noqa (N806), uppercase name is ok in this instance
+
+    if OS == 'Darwin':
+        if not os_test_macos():
+            ok = False
+    elif OS == 'Linux':
+        if not os_test_linux():
+            ok = False
+    elif OS == 'Windows':
+        if not os_test_windows():
+            ok = False
+    else:
+        cli.log.error('Unsupported OS detected: %s', OS)
+        ok = False
+
+    # Make sure the basic CLI tools we need are available and can be executed.
+    bin_ok = check_binaries()
+
+    if not bin_ok:
+        if yesno('Would you like to install dependencies?', default=True):
+            subprocess.run(['util/qmk_install.sh'])
+            bin_ok = check_binaries()
+
+    if bin_ok:
+        cli.log.info('All dependencies are installed.')
+    else:
+        ok = False
+
+    # Check out the QMK submodules
+    sub_ok = check_submodules()
+
+    if sub_ok:
+        cli.log.info('Submodules are up to date.')
+    else:
+        if yesno('Would you like to clone the submodules?', default=True):
+            submodules.update()
+            sub_ok = check_submodules()
+
+        if not sub_ok:
+            ok = False
 
     # Report a summary of our findings to the user
     if ok:
         cli.log.info('{fg_green}QMK is ready to go')
     else:
         cli.log.info('{fg_yellow}Problems detected, please fix these problems before proceeding.')
-        # FIXME(skullydazed): Link to a document about troubleshooting, or discord or something
+        # FIXME(skullydazed/unclaimed): Link to a document about troubleshooting, or discord or something

lib/python/qmk/questions.py

@@ -0,0 +1,97 @@
+"""Functions to collect user input.
+"""
+
+from milc import cli, format_ansi
+
+
+def yesno(prompt, *args, default=None, **kwargs):
+    """Displays prompt to the user and gets a yes or no response.
+
+    Returns True for a yes and False for a no.
+
+    If you add `--yes` and `--no` arguments to your program the user can answer questions by passing command line flags.
+
+        @add_argument('-y', '--yes', action='store_true', arg_only=True, help='Answer yes to all questions.')
+        @add_argument('-n', '--no', action='store_true', arg_only=True, help='Answer no to all questions.')
+
+    Arguments:
+        prompt
+            The prompt to present to the user. Can include ANSI and format strings like milc's `cli.print()`.
+
+        default
+            Whether to default to a Yes or No when the user presses enter.
+
+            None- force the user to enter Y or N
+
+            True- Default to yes
+
+            False- Default to no
+    """
+    if not args and kwargs:
+        args = kwargs
+
+    if 'no' in cli.args and cli.args.no:
+        return False
+
+    if 'yes' in cli.args and cli.args.yes:
+        return True
+
+    if default is not None:
+        if default:
+            prompt = prompt + ' [Y/n] '
+        else:
+            prompt = prompt + ' [y/N] '
+
+    while True:
+        print()
+        answer = input(format_ansi(prompt % args))
+        print()
+
+        if not answer and prompt is not None:
+            return default
+
+        elif answer.lower() in ['y', 'yes']:
+            return True
+
+        elif answer.lower() in ['n', 'no']:
+            return False
+
+
+def question(prompt, *args, default=None, confirm=False, answer_type=str, **kwargs):
+    """Prompt the user to answer a question with a free-form input.
+
+        prompt
+            The prompt to present to the user. Can include ANSI and format strings like milc's `cli.print()`.
+
+        default
+            The value to return when the user doesn't enter any value. Use None to prompt until they enter a value.
+
+        answer_type
+            Specify a type function for the answer. Will re-prompt the user if the function raises any errors. Common choices here include int, float, and decimal.Decimal.
+    """
+    if not args and kwargs:
+        args = kwargs
+
+    if default is not None:
+        prompt = '%s [%s] ' % (prompt, default)
+
+    while True:
+        print()
+        answer = input(format_ansi(prompt % args))
+        print()
+
+        if answer:
+            if confirm:
+                if yesno('Is the answer "%s" correct?', answer, default=True):
+                    try:
+                        return answer_type(answer)
+                    except Exception as e:
+                        cli.log.error('Could not convert answer (%s) to type %s: %s', answer, answer_type.__name__, str(e))
+            else:
+                try:
+                    return answer_type(answer)
+                except Exception as e:
+                    cli.log.error('Could not convert answer (%s) to type %s: %s', answer, answer_type.__name__, str(e))
+
+        elif default is not None:
+            return default

lib/python/qmk/submodules.py

@@ -0,0 +1,71 @@
+"""Functions for working with QMK's submodules.
+"""
+
+import subprocess
+
+
+def status():
+    """Returns a dictionary of submodules.
+
+    Each entry is a dict of the form:
+
+        {
+            'name': 'submodule_name',
+            'status': None/False/True,
+            'githash': '<sha-1 hash for the submodule>
+        }
+
+    status is None when the submodule doesn't exist, False when it's out of date, and True when it's current
+    """
+    submodules = {}
+    git_cmd = subprocess.run(['git', 'submodule', 'status'], stdout=subprocess.PIPE, stderr=subprocess.PIPE, timeout=30, universal_newlines=True)
+
+    for line in git_cmd.stdout.split('\n'):
+        if not line:
+            continue
+
+        status = line[0]
+        githash, submodule = line[1:].split()[:2]
+        submodules[submodule] = {'name': submodule, 'githash': githash}
+
+        if status == '-':
+            submodules[submodule]['status'] = None
+        elif status == '+':
+            submodules[submodule]['status'] = False
+        elif status == ' ':
+            submodules[submodule]['status'] = True
+        else:
+            raise ValueError('Unknown `git submodule status` sha-1 prefix character: "%s"' % status)
+
+    return submodules
+
+
+def update(submodules=None):
+    """Update the submodules.
+
+        submodules
+            A string containing a single submodule or a list of submodules.
+    """
+    git_sync_cmd = ['git', 'submodule', 'sync']
+    git_update_cmd = ['git', 'submodule', 'update', '--init']
+
+    if submodules is None:
+        # Update everything
+        git_sync_cmd.append('--recursive')
+        git_update_cmd.append('--recursive')
+        subprocess.run(git_sync_cmd, check=True)
+        subprocess.run(git_update_cmd, check=True)
+
+    else:
+        if isinstance(submodules, str):
+            # Update only a single submodule
+            git_sync_cmd.append(submodules)
+            git_update_cmd.append(submodules)
+            subprocess.run(git_sync_cmd, check=True)
+            subprocess.run(git_update_cmd, check=True)
+
+        else:
+            # Update submodules in a list
+            for submodule in submodules:
+                subprocess.run(git_sync_cmd + [submodule], check=True)
+                subprocess.run(git_update_cmd + [submodule], check=True)