Use pylint (via pantsbuild)

[cognifloyd]

Background

This is another part of introducing pants, as discussed in various TSC meetings.

Related PRs can be found in:

• pantsbuild milestone (which includes PRs since Initialize pantsbuild #5713)
• pantsbuild label (which also includes preparatory PRs that came before adding pantsbuild)

Overview of this PR

This configures pants to use pylint when running the lint goal. Configuring pylint is multi-faceted. We have to add config in pants.toml (like the pylint version to use and adding a new pylint_plugins resolve), generating lockfiles for pylint and pylint_plugins, and adding skip_pylint=True to a lot of BUILD files.

I recommend reviewing this PR by stepping through the commits.

This adds a new resolve, pylint_plugins, for the code under pylint_plugins/. This is separate from our default st2 resolve. I would just re-use the pylint resolve and lockfile (for the pylint tool), but pants does not support reusing a tool's config like that yet (There is discussion in the pants community on how to make this work better. We'll see how that goes.). So, we have to duplicate the version requirements in two places, and have two lockfiles that have practically the same contents.

Relevant Pants documentation

• ./pants lint goal
• ./pants generate-lockfiles goal
• Python Overview
  • Python Linters and Formatters
  • pylint backend
• Python 3rd Party Dependencies
  • Python Lockfiles
  • Manually Generating Lockfiles

68f1921 record pylint version requirements

First, this commit records the pylint version in pants.toml. This version is for the pylint resolve, which is used whenever pants is running pylint. It also adds the setuptools dep, because pylint fails to report that it uses pkg_resources from setuptools, and setuptools is NOT installed by default with pants+PEX.

st2/pants.toml

Lines 147 to 151 in 68f1921

	[pylint]
	version = "pylint~=2.8.2"
	extra_requirements = [
	"setuptools", # includes pkg_resources
	]

Second, this commit records the pylint version in pylint_plugins/BUILD. This version is for the pylint_plugins resolve, which is used whenever pants is running linters/tests/etc on the code in pylint_plugins/. This mirrors pants.toml by also including the same setuptools requirement.

st2/pylint_plugins/BUILD

Lines 7 to 15 in 68f1921

	python_requirement(
	name="pylint",
	requirements=[
	# This must be the same as [pylint].version in pants.toml
	"pylint~=2.8.2",
	# other requirements in [pylint].extra_requirements in pants.toml
	"setuptools",
	],
	)

Third, this commit adds an astroid dependency in pylint_plugins/BUILD, because that is actually the only third party dependency of our pylint_plugins/ code. But, we do not define a version for astroid; instead, the version of astroid gets constrained because we also require pylint, specifying the same as in pants.toml. pylint has narrow support for which versions of astroid it can use, so trying to maintain a version of both pylint and astroid proved irritating. Instead, we rely on the lockfile to keep things consistent in-between upgrading pylint.

st2/pylint_plugins/BUILD

Lines 17 to 21 in 68f1921

	python_requirement(
	name="astroid",
	# The version of astroid is constrained by the pylint version above
	requirements=["astroid"],
	)

395c869 add new resolve for pylint_plugins/ deps

First, this commit defines the pylint_plugins resolve by setting the path to the lockfile (which will be generated in a later commit).

st2/pants.toml

Lines 97 to 99 in 395c869

	[python.resolves]
	st2 = "lockfiles/st2.lock"
	pylint_plugins = "lockfiles/pylint_plugins.lock"

Second, this commit changes the __defaults__ for the pylint_plugins/ dir so that everything in that dir uses the new resolve:

st2/pylint_plugins/BUILD

Lines 1 to 5 in 395c869

	__defaults__(
	all=dict(
	resolve="pylint_plugins",
	)
	)

Third, this commit drops an implicit dependency on the root conftest.py file. Code in the pylint_plugins resolve cannot import code in the st2 resolve. pants will error if something tries to do that. (Plus, this renames the target name from test_utils0 to test_utils for clarity (the original name was generated by ./pants tailor).

st2/pylint_plugins/BUILD

Lines 9 to 14 in 395c869

	python_tests(
	name="tests",
	dependencies=[
	"!//conftest.py:test_utils",
	],
	)

9ac8365 enable pylint via pants

This registers the pylint backend in pants.toml, and tells pants where our .pylintrc file is, and where our pylint plugins are.

Effectively this covers how the Makefile runs pylint with args: --rcfile=./lint-configs/python/.pylintrc --load-plugins=pylint_plugins.api_models --load-plugins=pylint_plugins.db_models

st2/Makefile

Line 346 in 9ac8365

. $(VIRTUALENV_DIR)/bin/activate ; pylint -j $(PYLINT_CONCURRENCY) -E --rcfile=./lint-configs/python/.pylintrc --load-plugins=pylint_plugins.api_models --load-plugins=pylint_plugins.db_models $$component/$$component || exit 1; \

e085f52 Generate pylint* lockfiles

This generates both lockfiles: lockfiles/pylint.lock and lockfiles/pylint_plugins.lock. These files are exactly the same, except for the name of the resolve (pylint vs pylint_plugins). Each lockfile is 385 lines long. So, 770 of the lines in this PR are auto-generated.

add skip_pylint=True metadata to BUILD files

The next three commits add skip_pylint=True in a variety of contexts where running pylint doesn't work well. This runs pylint on a lot more of our code than the Makefile does, but skips files as needed. Maybe in the future we can enable more of our code.

In particular this skips running pylint on:

• tests
• intentionally invalid python file
• st2*/bin dirs (because the script names in bin ended up shadowing the modules they were importing and the - in other script names was problematic. see: Add way to disable module caching for specific files pylint-dev/pylint#2095)

There are two ways we add this metadata. We either add skip_pylint=True to individual targets, or we can skip entire directories with __defaults__:

st2/st2actions/tests/BUILD

Lines 1 to 5 in 4fd2f06

	__defaults__(
	all=dict(
	skip_pylint=True,
	)
	)

f4cb99f fix pants dep inference for pylint_plugins

It turns out, we have to use different pylint args when pants runs pylint vs when the legacy Makefile runs it. pants adds source_roots to PYTHONPATH, so pylint_plugins/ is on the PYTHONPATH which makes api_plugins and api_models top-level modules. When it is not on the PYTHONPATH, as in the Makefile, pylint_plugins is the top-level module. So, we had to just an import in the python test, and the args in `pants.toml.

4c7888d use pylint -E under pants

The Makefile uses -E when running pylint:

st2/Makefile

Line 346 in 4c7888d

. $(VIRTUALENV_DIR)/bin/activate ; pylint -j $(PYLINT_CONCURRENCY) -E --rcfile=./lint-configs/python/.pylintrc --load-plugins=pylint_plugins.api_models --load-plugins=pylint_plugins.db_models $$component/$$component || exit 1; \

I had to look up what that meant. Running pylint without that generates a lot of warnings. It turns out -E is short for --errors-only. So, I used --errors-only instead of -E to remind myself of what it does in the future. Hopefully we can drop this at some point.

st2/pants.toml

Lines 161 to 162 in 4c7888d

	# match the current Makefile usage with -E (TODO: drop this)
	"--errors-only",

[Eric-Arellano]

Exciting!

[amanda11]

LGTM

[rush-skills]

LGTM