Integrate Flake8 linting with SCons (PR #9958)

Code contributors regularly have to deal with ill-defined and inconsistently enforced code style requirements. Code reviewers spend much of their time reporting minor issues, time that would be better spent looking for architectural problems / product issues / logic errors.

In this commit we introduce automated checking of python style. The diff from new PR's will be tested for compliance with Flake8. The NVDA Python code already contains several inconsistent styles, so rather than try to match it I have tried to configure Flake8 to use the default style guidelines as much as possible.

Added two new SCons build targets:
- `lint`
  - creates a unified diff with `git diff -U0 $(git merge-base <baseBranch>)`
    - A helper script is used to generate this diff (`tests\lint\genDiff.py`)
  - The diff is piped to `flake8` to perform the linting.
  - The output is printed to stdout and also to `tests/lint/current.lint`
- `lintInstall`
  - required by `lint`.
  - Uses pip to install dependencies from a `requirements.txt` file.

AppVeyor changes:
- Adds a new script for tests phase of build
- Mostly does what SCons does, does not need to worry about getting working tree / uncommit changes into the diff.
- In order to preserve the availability of artifacts, these are uploaded from a `on_finish` phase rather than `artifacts` phase.
  - This acts like a "finally" block, and happens regardless of whether the build passes or fails.  
  - The installer artifact is often used to test if a change fixes an issue before the PR is polished off / reviewed. It also can help reviewers to test a change locally without having to build the branch.
- A message is sent when there are linting errors.
- A failed lint will still halt the build, system tests are not run.

Closes #5918

appveyor.yml

@@ -23,6 +23,8 @@ environment:
 init:
  - ps: |
      # iex ((new-object net.webclient).DownloadString('https://raw.githubusercontent.com/appveyor/ci/master/scripts/enable-rdp.ps1'))
+     $pythonVersion = (py --version)
+     echo $pythonVersion
      if ($env:APPVEYOR_REPO_TAG_NAME -and $env:APPVEYOR_REPO_TAG_NAME.StartsWith("release-")) {
       # Strip "release-" prefix.
       $version = $env:APPVEYOR_REPO_TAG_NAME.Substring(8)
@@ -121,10 +123,11 @@ build_script:
 
 before_test:
  # install required packages
- - py -m pip install -r tests/system/requirements.txt
+ - py -m pip install -r tests/system/requirements.txt -r tests/lint/lintInstall/requirements.txt
  - mkdir testOutput
  - mkdir testOutput\unit
  - mkdir testOutput\system
+ - mkdir testOutput\lint
  - ps: |
      $errorCode=0
      $nvdaLauncherFile=".\output\nvda"
@@ -157,6 +160,28 @@ test_script:
      $wc = New-Object 'System.Net.WebClient'
      $wc.UploadFile("https://ci.appveyor.com/api/testresults/junit/$($env:APPVEYOR_JOB_ID)", $unitTestsXml)
      if($errorCode -ne 0) { $host.SetShouldExit($errorCode) }
+ - ps: |
+    if($env:APPVEYOR_PULL_REQUEST_NUMBER) {
+      $lintOutput = (Resolve-Path .\testOutput\lint\)
+      $lintSource = (Resolve-Path .\tests\lint\)
+      git fetch -q origin $env:APPVEYOR_REPO_BRANCH
+      $prDiff = "$lintOutput\prDiff.patch"
+      git diff -U0 FETCH_HEAD...HEAD > $prDiff
+      $flake8Config = "$lintSource\flake8.ini"
+      $flake8Output = "$lintOutput\PR-Flake8.txt"
+      type "$prDiff" | py -m flake8 --diff --output-file="$flake8Output" --tee --config="$flake8Config"
+      if($LastExitCode -ne 0) {
+       $errorCode=$LastExitCode
+       Add-AppveyorMessage "PR introduces Flake8 errors"
+      }
+      Push-AppveyorArtifact $flake8Output
+      $junitXML = "$lintOutput\PR-Flake8.xml"
+      py "$lintSource\createJunitReport.py" "$flake8Output" "$junitXML"
+      Push-AppveyorArtifact $junitXML
+      $wc = New-Object 'System.Net.WebClient'
+      $wc.UploadFile("https://ci.appveyor.com/api/testresults/junit/$($env:APPVEYOR_JOB_ID)", $junitXML)
+      if($errorCode -ne 0) { $host.SetShouldExit($errorCode) }
+    }
  - ps: |
      $testOutput = (Resolve-Path .\testOutput\)
      $systemTestOutput = (Resolve-Path "$testOutput\system")
@@ -169,9 +194,10 @@ test_script:
      $wc.UploadFile("https://ci.appveyor.com/api/testresults/junit/$($env:APPVEYOR_JOB_ID)", (Resolve-Path "$systemTestOutput\systemTests.xml"))
      if($errorCode -ne 0) { $host.SetShouldExit($errorCode) }
 
-artifacts:
- - path: output\*
- - path: output\*\*
+on_finish:
+ - ps: |
+      Get-ChildItem output\*   | % { Push-AppveyorArtifact $_.FullName -FileName $_.Name }
+      Get-ChildItem output\*\* | % { Push-AppveyorArtifact $_.FullName -FileName $_.Name }
 
 deploy_script:
  - ps: |
@@ -180,9 +206,9 @@ deploy_script:
       # Notify our server.
       $exe = Get-ChildItem -Name output\*.exe
       $hash = (Get-FileHash "output\$exe" -Algorithm SHA1).Hash.ToLower()
-      $apiVersion = (python -c "import sys; sys.path.append('source'); from addonAPIVersion import CURRENT; print('{}.{}.{}'.format(*CURRENT))")
+      $apiVersion = (py -c "import sys; sys.path.append('source'); from addonAPIVersion import CURRENT; print('{}.{}.{}'.format(*CURRENT))")
       echo apiversion: $apiVersion
-      $apiCompatTo = (python -c "import sys; sys.path.append('source'); from addonAPIVersion import BACK_COMPAT_TO; print('{}.{}.{}'.format(*BACK_COMPAT_TO))")
+      $apiCompatTo = (py -c "import sys; sys.path.append('source'); from addonAPIVersion import BACK_COMPAT_TO; print('{}.{}.{}'.format(*BACK_COMPAT_TO))")
       echo apiBackCompatTo: $apiCompatTo
       $data = @{
        jobId=$env:APPVEYOR_JOB_ID;

readme.md

@@ -91,7 +91,13 @@ Additionally, the following build time dependencies are included in Git submodul
 * [Boost Optional (stand-alone header)](https://github.com/akrzemi1/Optional), from commit [3922965](https://github.com/akrzemi1/Optional/commit/3922965396fc455c6b1770374b9b4111799588a9)
 
 ### Other Dependencies
-These dependencies are not included in Git submodules, but aren't needed by most people.
+To lint using Flake 8 locally using our SCons integration, some dependencies are installed (automatically) via pip.
+Although this [must be run manually](#linting-your-changes), developers may wish to first configure a Python Virtual Environment to ensure their general install is not affected.
+* Flake8
+* Flake8-tabs
+
+
+The following dependencies aren't needed by most people, and are not included in Git submodules:
 
 * To generate developer documentation for nvdaHelper: [Doxygen Windows installer](http://www.doxygen.nl/download.html), version 1.8.15:
 
@@ -155,6 +161,8 @@ scons dist
 
 The build will be created in the dist directory.
 
+### Building the installer
+
 To create a launcher archive (one executable allowing for installation or portable dist generation), type:
 
 ```
@@ -163,6 +171,8 @@ scons launcher
 
 The archive will be placed in the output directory.
 
+### Building the developer documentation
+
 To generate the NVDA developer guide, type:
 
 ```
@@ -180,6 +190,7 @@ scons devDocs_nvdaHelper
 
 The documentation will be placed in the `devDocs\nvdaHelper` folder in the output directory.
 
+### Generate debug symbols archive
 To generate an archive of debug symbols for the various dll/exe binaries, type:
 
 ```
@@ -188,12 +199,14 @@ scons symbolsArchive
 
 The archive will be placed in the output directory.
 
+### Generate translation template
 To generate a gettext translation template (for translators), type:
 
 ```
 scons pot
 ```
 
+### Customising the build
 Optionally, the build can be customised by providing variables on the command line:
 
 * version: The version of this build.
@@ -216,15 +229,15 @@ scons launcher version=test1
 ## Running Automated Tests
 If you make a change to the NVDA code, you should run NVDA's automated tests.
 These tests help to ensure that code changes do not unintentionally break functionality that was previously working.
-Currently, NVDA has two kinds of automated testing: unit tests and translatable string checks.
 
-To run the tests, first change directory to the root of the NVDA source distribution as above.
+To run the tests (unit tests, translatable string checks), first change directory to the root of the NVDA source distribution as above.
 Then, run:
 
 ```
 scons tests
 ```
 
+### Unit tests
 To run only specific unit tests, specify them using the `unitTests` variable on the command line.
 The tests should be provided as a comma separated list.
 Each test should be specified as a Python module, class or method relative to the `tests\unit` directory.
@@ -234,12 +247,25 @@ For example, to run only methods in the `TestMove` and `TestSelection` classes i
 scons tests unitTests=test_cursorManager.TestMove,test_cursorManager.TestSelection
 ```
 
+### Translatable string checks
 To run only the translatable string checks (which check that all translatable strings have translator comments), run:
 
 ```
 scons checkPot
 ```
 
+### Linting your changes
+In order to ensure your changes comply with NVDA's coding style you can run the Flake8 linter locally.
+Running via SCons will use Flake8 to inspect only the differences between your working directory and the specified `base` branch.
+If you create a Pull Request, the `base` branch you use here should be the same as the target you would use for a Pull Request. In most cases it will be `origin/master`.
+```
+scons lint base=origin/master
+```
+
+To be warned about linting errors faster, you may wish to integrate Flake8 other development tools you are using.
+For more details, see `tests/lint/readme.md`
+
+### System Tests
 You may also use scons to run the system tests, though this will still rely on having set up the dependencies (see `tests/system/readme.md`).
 
 ```

sconstruct

@@ -76,6 +76,17 @@ vars.Add(ListVariable("nvdaHelperDebugFlags", "a list of debugging features you
 vars.Add(EnumVariable('nvdaHelperLogLevel','The level of logging you wish to see, lower is more verbose','15',allowed_values=[str(x) for x in range(60)]))
 if "tests" in COMMAND_LINE_TARGETS:
 	vars.Add("unitTests", "A list of unit tests to run", "")
+if "lint" in COMMAND_LINE_TARGETS:
+	vars.Add(  # pass through variable that lint is requested
+		"doLint",
+		"internal use",
+		True
+	)
+	vars.Add(
+		"base",
+		"Lint is done only on a diff, specify the ref to use as base for the diff.",
+		None
+	)
 if "systemTests" in COMMAND_LINE_TARGETS:
 	vars.Add("filter", "A filter for the name of the system test(s) to run. Wildcards accepted.", "")
 

tests/lint/.gitignore

@@ -0,0 +1,2 @@
+current.diff
+current.lint

tests/lint/createJunitReport.py

@@ -0,0 +1,65 @@
+# -*- coding: UTF-8 -*-
+# A part of NonVisual Desktop Access (NVDA)
+# This file is covered by the GNU General Public License.
+# See the file COPYING for more details.
+# Copyright (C) 2019 NV Access Limited
+
+import os
+from sys import argv
+
+NO_ERROR = r'''<?xml version="1.0" encoding="UTF-8"?>
+<testsuite name="flake8" tests="1" errors="0" failures="0" skip="0">
+<testcase classname="flake8.lint" name="flake8_diff_lint" time="1.00">
+</testcase>
+</testsuite>
+'''
+
+# With Error:
+WE_PRE = r'''<?xml version="1.0" encoding="UTF-8"?>
+<testsuite name="flake8" tests="1" errors="1" failures="0" skip="0">
+<testcase classname="flake8.lint" name="flake8_diff_lint" time="1.00">
+<error type="lintError" message="Linting errors occurred">
+<![CDATA[
+'''
+WE_POST = r'''
+]]>
+</error>
+</testcase>
+</testsuite>
+'''
+
+
+def makeJunitXML(inFileName, outFileName):
+	with open(inFileName, 'rt', encoding='UTF-8') as flake8In:
+		errorText = flake8In.read()
+	if len(errorText) > 0:
+		# make "with error" xml content
+		outContents = f'{WE_PRE}{errorText}{WE_POST}'
+	else:
+		# make "no error" xml content
+		outContents = NO_ERROR
+
+	with open(outFileName, 'wt', encoding='UTF-8') as out:
+		out.write(outContents)
+
+
+def main():
+	try:
+		if len(argv) != 3:
+			raise RuntimeError(
+				f"{argv[0]} expects two arguments: flake8_output_file_name junit_file_name"
+			)
+		scriptName, flake8OutputFileName, junitFileName = argv
+		if not os.path.isfile(flake8OutputFileName):
+			raise RuntimeError(
+				f"Flake8_output_file does not exist at {flake8OutputFileName}"
+			)
+		makeJunitXML(flake8OutputFileName, junitFileName)
+	except Exception as e:
+		print(e)
+		raise e
+
+
+if __name__ == "__main__":
+	# execute only if run as a script
+	main()

tests/lint/flake8.ini

@@ -0,0 +1,35 @@
+[flake8]
+
+# Plugins
+use-flake8-tabs = True
+use-pycodestyle-indent = True
+
+# Reporting
+statistics = True
+doctests = True
+show-source = True
+
+# Options
+max-complexity = 15
+max-line-length = 110
+hang-closing = True
+
+ignore =
+	W191, # indentation contains tabs
+	E126, # continuation line over-indented for hanging indent
+	E133, # closing bracket is missing indentation
+	W503, # line break before binary operator. As opposed to W504 (line break after binary operator) which we want to check for.
+
+builtins = # inform flake8 about functions we consider built-in.
+	_, # translation lookup
+	pgettext, # translation lookup
+
+exclude = # don't bother looking in the following subdirectories / files.
+	.git,
+	__pycache__,
+	.tox,
+	build,
+	output,
+	include,
+	miscDeps,
+	source/louis,