CLI: Udev related fixes and improvements (#10736)

2 files changed, 118 insertions, 93 deletions

diff --git a/lib/python/qmk/cli/doctor.py b/lib/python/qmk/cli/doctor.py
index caa98a71c..a5eda555f 100755
--- a/lib/python/qmk/cli/doctor.py
+++ b/lib/python/qmk/cli/doctor.py
@@ -7,6 +7,7 @@ import re
 import shutil
 import subprocess
 from pathlib import Path
+from enum import Enum
 
 from milc import cli
 from qmk import submodules
@@ -14,6 +15,13 @@ from qmk.constants import QMK_FIRMWARE
 from qmk.questions import yesno
 from qmk.commands import run
 
+
+class CheckStatus(Enum):
+    OK = 1
+    WARNING = 2
+    ERROR = 3
+
+
 ESSENTIAL_BINARIES = {
     'dfu-programmer': {},
     'avrdude': {},
@@ -33,9 +41,12 @@ def _udev_rule(vid, pid=None, *args):
     """
     rule = ""
     if pid:
-        rule = 'SUBSYSTEMS=="usb", ATTRS{idVendor}=="%s", ATTRS{idProduct}=="%s", TAG+="uaccess", RUN{builtin}+="uaccess"' % (vid, pid)
+        rule = 'SUBSYSTEMS=="usb", ATTRS{idVendor}=="%s", ATTRS{idProduct}=="%s", TAG+="uaccess"' % (
+            vid,
+            pid,
+        )
     else:
-        rule = 'SUBSYSTEMS=="usb", ATTRS{idVendor}=="%s", TAG+="uaccess", RUN{builtin}+="uaccess"' % vid
+        rule = 'SUBSYSTEMS=="usb", ATTRS{idVendor}=="%s", TAG+="uaccess"' % vid
     if args:
         rule = ', '.join([rule, *args])
     return rule
@@ -69,24 +80,25 @@ def check_arm_gcc_version():
         version_number = ESSENTIAL_BINARIES['arm-none-eabi-gcc']['output'].strip()
         cli.log.info('Found arm-none-eabi-gcc version %s', version_number)
 
-    return True  # Right now all known arm versions are ok
+    return CheckStatus.OK  # Right now all known arm versions are ok
 
 
 def check_avr_gcc_version():
     """Returns True if the avr-gcc version is not known to cause problems.
     """
+    rc = CheckStatus.ERROR
     if 'output' in ESSENTIAL_BINARIES['avr-gcc']:
         version_number = ESSENTIAL_BINARIES['avr-gcc']['output'].strip()
 
+        cli.log.info('Found avr-gcc version %s', version_number)
+        rc = CheckStatus.OK
+
         parsed_version = parse_gcc_version(version_number)
         if parsed_version['major'] > 8:
-            cli.log.error('We do not recommend avr-gcc newer than 8. Downgrading to 8.x is recommended.')
-            return False
+            cli.log.warning('{fg_yellow}We do not recommend avr-gcc newer than 8. Downgrading to 8.x is recommended.')
+            rc = CheckStatus.WARNING
 
-        cli.log.info('Found avr-gcc version %s', version_number)
-        return True
-
-    return False
+    return rc
 
 
 def check_avrdude_version():
@@ -95,7 +107,7 @@ def check_avrdude_version():
         version_number = last_line.split()[2][:-1]
         cli.log.info('Found avrdude version %s', version_number)
 
-    return True
+    return CheckStatus.OK
 
 
 def check_dfu_util_version():
@@ -104,7 +116,7 @@ def check_dfu_util_version():
         version_number = first_line.split()[1]
         cli.log.info('Found dfu-util version %s', version_number)
 
-    return True
+    return CheckStatus.OK
 
 
 def check_dfu_programmer_version():
@@ -113,7 +125,7 @@ def check_dfu_programmer_version():
         version_number = first_line.split()[1]
         cli.log.info('Found dfu-programmer version %s', version_number)
 
-    return True
+    return CheckStatus.OK
 
 
 def check_binaries():
@@ -131,58 +143,56 @@ def check_binaries():
 def check_submodules():
     """Iterates through all submodules to make sure they're cloned and up to date.
     """
-    ok = True
-
     for submodule in submodules.status().values():
         if submodule['status'] is None:
             cli.log.error('Submodule %s has not yet been cloned!', submodule['name'])
-            ok = False
+            return CheckStatus.ERROR
         elif not submodule['status']:
-            cli.log.error('Submodule %s is not up to date!', submodule['name'])
-            ok = False
+            cli.log.warning('Submodule %s is not up to date!', submodule['name'])
+            return CheckStatus.WARNING
 
-    return ok
+    return CheckStatus.OK
 
 
 def check_udev_rules():
     """Make sure the udev rules look good.
     """
-    ok = True
+    rc = CheckStatus.OK
     udev_dir = Path("/etc/udev/rules.d/")
     desired_rules = {
         'atmel-dfu': {
-            _udev_rule("03EB", "2FEF"),  # ATmega16U2
-            _udev_rule("03EB", "2FF0"),  # ATmega32U2
-            _udev_rule("03EB", "2FF3"),  # ATmega16U4
-            _udev_rule("03EB", "2FF4"),  # ATmega32U4
-            _udev_rule("03EB", "2FF9"),  # AT90USB64
-            _udev_rule("03EB", "2FFB")  # AT90USB128
+            _udev_rule("03eb", "2fef"),  # ATmega16U2
+            _udev_rule("03eb", "2ff0"),  # ATmega32U2
+            _udev_rule("03eb", "2ff3"),  # ATmega16U4
+            _udev_rule("03eb", "2ff4"),  # ATmega32U4
+            _udev_rule("03eb", "2ff9"),  # AT90USB64
+            _udev_rule("03eb", "2ffb")  # AT90USB128
         },
-        'kiibohd': {_udev_rule("1C11", "B007")},
+        'kiibohd': {_udev_rule("1c11", "b007")},
         'stm32': {
-            _udev_rule("1EAF", "0003"),  # STM32duino
-            _udev_rule("0483", "DF11")  # STM32 DFU
+            _udev_rule("1eaf", "0003"),  # STM32duino
+            _udev_rule("0483", "df11")  # STM32 DFU
         },
-        'bootloadhid': {_udev_rule("16C0", "05DF")},
-        'usbasploader': {_udev_rule("16C0", "05DC")},
-        'massdrop': {_udev_rule("03EB", "6124", 'ENV{ID_MM_DEVICE_IGNORE}="1"')},
+        'bootloadhid': {_udev_rule("16c0", "05df")},
+        'usbasploader': {_udev_rule("16c0", "05dc")},
+        'massdrop': {_udev_rule("03eb", "6124", 'ENV{ID_MM_DEVICE_IGNORE}="1"')},
         'caterina': {
             # Spark Fun Electronics
-            _udev_rule("1B4F", "9203", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # Pro Micro 3V3/8MHz
-            _udev_rule("1B4F", "9205", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # Pro Micro 5V/16MHz
-            _udev_rule("1B4F", "9207", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # LilyPad 3V3/8MHz (and some Pro Micro clones)
-            # Pololu Electronics
-            _udev_rule("1FFB", "0101", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # A-Star 32U4
+            _udev_rule("1b4f", "9203", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # Pro Micro 3V3/8MHz
+            _udev_rule("1b4f", "9205", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # Pro Micro 5V/16MHz
+            _udev_rule("1b4f", "9207", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # LilyPad 3V3/8MHz (and some Pro Micro clones)
+            # Pololu EleCTRONICS
+            _udev_rule("1ffb", "0101", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # A-Star 32U4
             # Arduino SA
             _udev_rule("2341", "0036", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # Leonardo
             _udev_rule("2341", "0037", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # Micro
-            # Adafruit Industries LLC
-            _udev_rule("239A", "000C", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # Feather 32U4
-            _udev_rule("239A", "000D", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # ItsyBitsy 32U4 3V3/8MHz
-            _udev_rule("239A", "000E", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # ItsyBitsy 32U4 5V/16MHz
-            # dog hunter AG
-            _udev_rule("2A03", "0036", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # Leonardo
-            _udev_rule("2A03", "0037", 'ENV{ID_MM_DEVICE_IGNORE}="1"')  # Micro
+            # Adafruit INDUSTRIES llC
+            _udev_rule("239a", "000c", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # Feather 32U4
+            _udev_rule("239a", "000d", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # ItsyBitsy 32U4 3V3/8MHz
+            _udev_rule("239a", "000e", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # ItsyBitsy 32U4 5V/16MHz
+            # dog hunter ag
+            _udev_rule("2a03", "0036", 'ENV{ID_MM_DEVICE_IGNORE}="1"'),  # Leonardo
+            _udev_rule("2a03", "0037", 'ENV{ID_MM_DEVICE_IGNORE}="1"')  # Micro
         }
     }
 
@@ -209,31 +219,43 @@ def check_udev_rules():
 
         # Check if the desired rules are among the currently present rules
         for bootloader, rules in desired_rules.items():
-            # For caterina, check if ModemManager is running
-            if bootloader == "caterina":
-                if check_modem_manager():
-                    ok = False
-                    cli.log.warn("{bg_yellow}Detected ModemManager without the necessary udev rules. Please either disable it or set the appropriate udev rules if you are using a Pro Micro.")
             if not rules.issubset(current_rules):
                 deprecated_rule = deprecated_rules.get(bootloader)
                 if deprecated_rule and deprecated_rule.issubset(current_rules):
-                    cli.log.warn("{bg_yellow}Found old, deprecated udev rules for '%s' boards. The new rules on https://docs.qmk.fm/#/faq_build?id=linux-udev-rules offer better security with the same functionality.", bootloader)
+                    cli.log.warning("{fg_yellow}Found old, deprecated udev rules for '%s' boards. The new rules on https://docs.qmk.fm/#/faq_build?id=linux-udev-rules offer better security with the same functionality.", bootloader)
                 else:
-                    cli.log.warn("{bg_yellow}Missing udev rules for '%s' boards. See https://docs.qmk.fm/#/faq_build?id=linux-udev-rules for more details.", bootloader)
+                    # For caterina, check if ModemManager is running
+                    if bootloader == "caterina":
+                        if check_modem_manager():
+                            rc = CheckStatus.WARNING
+                            cli.log.warning("{fg_yellow}Detected ModemManager without the necessary udev rules. Please either disable it or set the appropriate udev rules if you are using a Pro Micro.")
+                    rc = CheckStatus.WARNING
+                    cli.log.warning("{fg_yellow}Missing or outdated udev rules for '%s' boards. Run 'sudo cp %s/util/udev/50-qmk.rules /etc/udev/rules.d/'.", bootloader, QMK_FIRMWARE)
 
-    return ok
+    else:
+        cli.log.warning("{fg_yellow}'%s' does not exist. Skipping udev rule checking...", udev_dir)
+
+    return rc
+
+
+def check_systemd():
+    """Check if it's a systemd system
+    """
+    return bool(shutil.which("systemctl"))
 
 
 def check_modem_manager():
     """Returns True if ModemManager is running.
+
     """
-    if shutil.which("systemctl"):
+    if check_systemd():
         mm_check = run(["systemctl", "--quiet", "is-active", "ModemManager.service"], timeout=10)
         if mm_check.returncode == 0:
             return True
-
     else:
-        cli.log.warn("Can't find systemctl to check for ModemManager.")
+        """(TODO): Add check for non-systemd systems
+        """
+    return False
 
 
 def is_executable(command):
@@ -263,12 +285,8 @@ 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
+    return check_udev_rules()
 
 
 def os_test_macos():
@@ -276,7 +294,7 @@ def os_test_macos():
     """
     cli.log.info("Detected {fg_cyan}macOS.")
 
-    return True
+    return CheckStatus.OK
 
 
 def os_test_windows():
@@ -284,7 +302,7 @@ def os_test_windows():
     """
     cli.log.info("Detected {fg_cyan}Windows.")
 
-    return True
+    return CheckStatus.OK
 
 
 @cli.argument('-y', '--yes', action='store_true', arg_only=True, help='Answer yes to all questions.')
@@ -299,23 +317,20 @@ def doctor(cli):
         * [ ] Compile a trivial program with each compiler
     """
     cli.log.info('QMK Doctor is checking your environment.')
-    ok = True
+    status = CheckStatus.OK
 
     # Determine our OS and run platform specific tests
     platform_id = platform.platform().lower()
 
     if 'darwin' in platform_id or 'macos' in platform_id:
-        if not os_test_macos():
-            ok = False
+        status = os_test_macos()
     elif 'linux' in platform_id:
-        if not os_test_linux():
-            ok = False
+        status = os_test_linux()
     elif 'windows' in platform_id:
-        if not os_test_windows():
-            ok = False
+        status = os_test_windows()
     else:
-        cli.log.error('Unsupported OS detected: %s', platform_id)
-        ok = False
+        cli.log.warning('Unsupported OS detected: %s', platform_id)
+        status = CheckStatus.WARNING
 
     cli.log.info('QMK home: {fg_cyan}%s', QMK_FIRMWARE)
 
@@ -330,31 +345,41 @@ def doctor(cli):
     if bin_ok:
         cli.log.info('All dependencies are installed.')
     else:
-        ok = False
+        status = CheckStatus.ERROR
 
     # Make sure the tools are at the correct version
+    ver_ok = []
     for check in (check_arm_gcc_version, check_avr_gcc_version, check_avrdude_version, check_dfu_util_version, check_dfu_programmer_version):
-        if not check():
-            ok = False
+        ver_ok.append(check())
+
+    if CheckStatus.ERROR in ver_ok:
+        status = CheckStatus.ERROR
+    elif CheckStatus.WARNING in ver_ok and status == CheckStatus.OK:
+        status = CheckStatus.WARNING
 
     # Check out the QMK submodules
     sub_ok = check_submodules()
 
-    if sub_ok:
+    if sub_ok == CheckStatus.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
+        if CheckStatus.ERROR in sub_ok:
+            status = CheckStatus.ERROR
+        elif CheckStatus.WARNING in sub_ok and status == CheckStatus.OK:
+            status = CheckStatus.WARNING
 
     # Report a summary of our findings to the user
-    if ok:
+    if status == CheckStatus.OK:
         cli.log.info('{fg_green}QMK is ready to go')
+        return 0
+    elif status == CheckStatus.WARNING:
+        cli.log.info('{fg_yellow}QMK is ready to go, but minor problems were found')
+        return 1
     else:
-        cli.log.info('{fg_yellow}Problems detected, please fix these problems before proceeding.')
-        # FIXME(skullydazed/unclaimed): Link to a document about troubleshooting, or discord or something
-
-    return ok
+        cli.log.info('{fg_red}Major problems detected, please fix these problems before proceeding.')
+        cli.log.info('{fg_blue}Check out the FAQ (https://docs.qmk.fm/#/faq_build) or join the QMK Discord (https://discord.gg/Uq7gcHh) for help.')
+        return 2
diff --git a/lib/python/qmk/tests/test_cli_commands.py b/lib/python/qmk/tests/test_cli_commands.py
index df5f047da..dd0c572a7 100644
--- a/lib/python/qmk/tests/test_cli_commands.py
+++ b/lib/python/qmk/tests/test_cli_commands.py
@@ -13,14 +13,14 @@ def check_subcommand(command, *args):
     return result
 
 
-def check_returncode(result, expected=0):
+def check_returncode(result, expected=[0]):
     """Print stdout if `result.returncode` does not match `expected`.
     """
-    if result.returncode != expected:
+    if result.returncode not in expected:
         print('`%s` stdout:' % ' '.join(result.args))
         print(result.stdout)
         print('returncode:', result.returncode)
-    assert result.returncode == expected
+    assert result.returncode in expected
 
 
 def test_cformat():
@@ -45,7 +45,7 @@ def test_flash():
 
 def test_flash_bootloaders():
     result = check_subcommand('flash', '-b')
-    check_returncode(result, 1)
+    check_returncode(result, [1])
 
 
 def test_config():
@@ -62,7 +62,7 @@ def test_kle2json():
 
 def test_doctor():
     result = check_subcommand('doctor', '-n')
-    check_returncode(result)
+    check_returncode(result, [0, 1])
     assert 'QMK Doctor is checking your environment.' in result.stdout
     assert 'QMK is ready to go' in result.stdout
 
@@ -89,43 +89,43 @@ def test_list_keyboards():
 
 def test_list_keymaps():
     result = check_subcommand('list-keymaps', '-kb', 'handwired/onekey/pytest')
-    check_returncode(result, 0)
+    check_returncode(result)
     assert 'default' and 'test' in result.stdout
 
 
 def test_list_keymaps_long():
     result = check_subcommand('list-keymaps', '--keyboard', 'handwired/onekey/pytest')
-    check_returncode(result, 0)
+    check_returncode(result)
     assert 'default' and 'test' in result.stdout
 
 
 def test_list_keymaps_kb_only():
     result = check_subcommand('list-keymaps', '-kb', 'niu_mini')
-    check_returncode(result, 0)
+    check_returncode(result)
     assert 'default' and 'via' in result.stdout
 
 
 def test_list_keymaps_vendor_kb():
     result = check_subcommand('list-keymaps', '-kb', 'ai03/lunar')
-    check_returncode(result, 0)
+    check_returncode(result)
     assert 'default' and 'via' in result.stdout
 
 
 def test_list_keymaps_vendor_kb_rev():
     result = check_subcommand('list-keymaps', '-kb', 'kbdfans/kbd67/mkiirgb/v2')
-    check_returncode(result, 0)
+    check_returncode(result)
     assert 'default' and 'via' in result.stdout
 
 
 def test_list_keymaps_no_keyboard_found():
     result = check_subcommand('list-keymaps', '-kb', 'asdfghjkl')
-    check_returncode(result, 1)
+    check_returncode(result, [1])
     assert 'does not exist' in result.stdout
 
 
 def test_json2c():
     result = check_subcommand('json2c', 'keyboards/handwired/onekey/keymaps/default_json/keymap.json')
-    check_returncode(result, 0)
+    check_returncode(result)
     assert result.stdout == '#include QMK_KEYBOARD_H\nconst uint16_t PROGMEM keymaps[][MATRIX_ROWS][MATRIX_COLS] = {\t[0] = LAYOUT_ortho_1x1(KC_A)};\n\n'