Add black formatter check

[eloquence]

Resolves #568

• Adds black formatter with 100 character line length per our coding convention
• Adjusts flake8 settings accordingly
• Reinstates dev-requirements.in and dev-requirements.txt for managing these dependencies
• Generalizes and improves upon the auto-discovery logic for Python files in this repo
• Adds a venv Makefile target for convenience, following @rmol's example in Add black and isort securedrop-sdk#124
• Stops using the machine type executor in CI for lint jobs and running linters in Docker locally

Status

Ready for review

Test plan

Preparatory steps

• Check out this branch in your development environment
• Prepare a virtualenv using make venv and activate it using source .venv/bin/activate
• To test revision exclusion, ensure you are running at least git version 2.23.x.

flake8 and black targets

• Run make black.
  • Observe the output "All done! 35 files would be left unchanged."
  • Observe that the exit code is 0 (echo $?)
• Run make flake8
  • Observe that there is no output
  • Observe that the exit code is 0 (echo $?)
• Modify a Python file in this repository that ends with .py and introduce an issue that would be flagged by both flake8 and black, e.g., an E251 violation
• Modify a Python file in this repository that has no file extension (e.g., in the scripts directory) and introduce a similar issue.
• Re-run make flake8 and make black
  • Observe that the issues you introduced are reported for both files.
  • Observe a that the exit code is non-zero (echo $?) for both commands.
• Undo your modifications.

I would recommend additional investigation of the auto-discovery logic introduced in scripts/lint-all. We want to make sure that it covers all Python files in this repository, including likely future additions.

Excluding formatter revisions

• Run the command git blame scripts/validate_config.py
• Observe that several lines are attributed to commit 782a04e, which made formatting changes.
• Run the command git blame --ignore-revs-file .git-blame-ignore-revs scripts/validate_config.py
• Observe that these same lines are attributed to their original authors, and that commit 782a04e is excluded.

[eloquence]

OK, recapping some of the implementation choices here, after discussion with @conorsch:

• As with flake8, black autodiscovery will miss a few files. Much of this is due to our liberal creation of standalone scripts without the .py file extension.
• There seems to be limited benefit in adding .py everywhere.
• That said, the current approach to run flake8 is a bit clunky:
  • We rely on a custom script, scripts/flake8-linting, that will apt-get install the file command to use as an additional discovery method (i.e. we always run flake8 twice on most files)
  • We always run it in a Docker container (my understanding is that this is why we're using the machine type in CI, because we can't run Docker inside Docker).
  • We don't control the version of flake8 that is installed via a requirements file.

I intend to re-organize this as follows:

• Reinstate dev-requirements.in/dev-requirements.txt and specify flake8 and black
• Generalize scripts/flake8-linting to work with any linter, and do not install file but throw an error if it is not present on the local system.
• Switch CI to a suitable Docker image; run black and flake8 in virtualenv via their respective Make targets.
• Ensure that a single discovery method is sufficient, and if so, squash into a single pass.
• Combine both black and flake8 into a single lint job.

Comments, clarifications or objections welcome, now or later. :) If there's an alternative to file, such as a Python module we can use instead, that would seem to be more elegant and more cross-platform-friendly.

[eloquence]

(i.e. we always run flake8 twice on most files)

This was necessary because the auto-discovery method used missed some Python files that were classified as text/plain, such as launcher/sdw_updater_gui/strings.py . I've made this logic more inclusive so only a single pass should be required.

The changes above are now implemented, except for

throw an error if file is not present on the local system.

In addition, black in CI only runs once, globally, instead of running again for the launcher tests. I've also verified that git blame --ignore-revs-file works as intended.

I think this is ready for someone else to take a look. Leaving as draft PR until I add a formal test plan.

[conorsch]

Looking food to me, @eloquence. Confirming that files are currently clean:

$ make black
All done! ✨ 🍰 ✨
35 files would be left unchanged.

and after intentionally breaking formatting in a file, the same check exits non-zero:

$ make black
would reformat /home/user/securedrop-workstation/tests/test_vms_platform.py
Oh no! 💥 💔 💥
1 file would be reformatted, 34 files would be left unchanged.
make: *** [Makefile:20: black] Error 123
$ echo $?
2

A bit more guidance around testing, particularly the git ignore refs logic mentioned in freedomofpress/securedrop-sdk#124, would be helpful. Otherwise, looks ready for final review to my eye!

[eloquence]

throw an error if file is not present on the local system.

This is now also done, and I've added a detailed test plan. Calling this officially ready for review, thanks for taking a first pass @conorsch :-)

[conorsch]

Looks great! Confirmed full test plan.

In order to run the blame checks on Debian Stable, I had to enable buster-backports to get a new enough version of git. Then it worked beautifully. Note that git config blame.ignoreRevsFile .git-blame-ignore-revs (cribbed from freedomofpress/securedrop-sdk#124) will make the change stick around in the local repo config. I've added that to my ~/.gitconfig, in an attempt to apply the change everywhere.