Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #8541 by getting version from -dumpversion then --version as fa… #8583

Merged
merged 4 commits into from
Apr 4, 2020

Conversation

StreakInTheSky
Copy link
Contributor

@StreakInTheSky StreakInTheSky commented Mar 28, 2020

Description

When running qmk setup or qmk doctor would get error as referenced in #8541. The problem likely came from a change in the formatting of avr-gcc --version. Instead I used the flag -dumpversion and made it fallback to --version if it returned an error. Changed the cli.log string and the version checking to match the new output values.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@fauxpark
Copy link
Member

I think it might be clearer to simply add a version_arg to the dictionaries in ESSENTIAL_BINARIES and use those instead.

@fauxpark
Copy link
Member

The following is working well on MSYS and macOS:

diff --git a/lib/python/qmk/cli/doctor.py b/lib/python/qmk/cli/doctor.py
index 9b81c8508..18fe5a9b9 100755
--- a/lib/python/qmk/cli/doctor.py
+++ b/lib/python/qmk/cli/doctor.py
@@ -15,8 +15,8 @@ ESSENTIAL_BINARIES = {
     'dfu-programmer': {},
     'avrdude': {},
     'dfu-util': {},
-    'avr-gcc': {},
-    'arm-none-eabi-gcc': {},
+    'avr-gcc': {'version_arg': '-dumpversion'},
+    'arm-none-eabi-gcc': {'version_arg': '-dumpversion'},
     'bin/qmk': {},
 }
 ESSENTIAL_SUBMODULES = ['lib/chibios', 'lib/lufa']
@@ -35,9 +35,7 @@ def check_arm_gcc_version():
     """Returns True if the arm-none-eabi-gcc version is not known to cause problems.
     """
     if 'output' in ESSENTIAL_BINARIES['arm-none-eabi-gcc']:
-        first_line = ESSENTIAL_BINARIES['arm-none-eabi-gcc']['output'].split('\n')[0]
-        second_half = first_line.split(')', 1)[1].strip()
-        version_number = second_half.split()[0]
+        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
@@ -47,8 +45,7 @@ def check_avr_gcc_version():
     """Returns True if the avr-gcc version is not known to cause problems.
     """
     if 'output' in ESSENTIAL_BINARIES['avr-gcc']:
-        first_line = ESSENTIAL_BINARIES['avr-gcc']['output'].split('\n')[0]
-        version_number = first_line.split()[2]
+        version_number = ESSENTIAL_BINARIES['avr-gcc']['output'].strip()
 
         major, minor, rest = version_number.split('.', 2)
         if int(major) > 8:
@@ -153,7 +150,8 @@ def is_executable(command):
         return False
 
     # Make sure the command can be executed
-    check = subprocess.run([command, '--version'], stdout=subprocess.PIPE, stderr=subprocess.PIPE, timeout=5, universal_newlines=True)
+    version_arg = ESSENTIAL_BINARIES[command].get('version_arg', '--version')
+    check = subprocess.run([command, version_arg], stdout=subprocess.PIPE, stderr=subprocess.PIPE, timeout=5, universal_newlines=True)
     ESSENTIAL_BINARIES[command]['output'] = check.stdout
 
     if check.returncode in [0, 1]:  # Older versions of dfu-programmer exit 1

lib/python/qmk/cli/doctor.py Outdated Show resolved Hide resolved
@fauxpark fauxpark requested a review from a team March 31, 2020 02:05
@fauxpark fauxpark added cli qmk cli command python bug labels Mar 31, 2020
lib/python/qmk/cli/doctor.py Show resolved Hide resolved
@@ -154,14 +151,16 @@ def is_executable(command):
return False

# Make sure the command can be executed
check = run([command, '--version'], stdout=subprocess.PIPE, stderr=subprocess.PIPE, timeout=5, universal_newlines=True)
version_arg = ESSENTIAL_BINARIES[command].get('version_arg', '--version')
check = run([command, version_arg], stdout=subprocess.PIPE, stderr=subprocess.PIPE, timeout=5, universal_newlines=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
check = run([command, version_arg], stdout=subprocess.PIPE, stderr=subprocess.PIPE, timeout=5, universal_newlines=True)
check = run([command, version_arg], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, timeout=5, universal_newlines=True)

Since @fauxpark found out avrdude prints the help to stderr.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And dfu-programmer.

@skullydazed skullydazed merged commit 2f15cb2 into qmk:master Apr 4, 2020
@StreakInTheSky StreakInTheSky deleted the fix-doctor-version-checking branch September 3, 2021 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cli qmk cli command python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants