Bug 1803510 - Set up isort as a separate linter. r=linter-reviewers,ahal DONTBUILD

This makes it possible to use the resolve all configs option and set up
different first party modules for different directories.

The downside is that configuration options (e.g. profile = black) have to
be repeated in every .isort.cfg file, because isort never merges configuration
files.

Differential Revision: https://phabricator.services.mozilla.com/D163567

tools/lint/flake8.yml

@@ -9,7 +9,7 @@ flake8:
         - '**/.flake8'
         - 'tools/lint/python/flake8*'
     # Rules that should result in warnings rather than errors.
-    warning-rules: ['I001', 'I003', 'I004', 'I005']
+    warning-rules: []
     type: external
     payload: python.flake8:lint
     setup: python.flake8:setup

tools/lint/isort.yml

@@ -0,0 +1,14 @@
+---
+isort:
+    description: Sort python imports
+    level: warning
+    # Excludes should be added to topsrcdir/.flake8.
+    exclude: []
+    extensions: ['configure', 'py']
+    support-files:
+        - '**/.flake8'
+        - '**/.isort.cfg'
+        - 'tools/lint/python/isort*'
+    type: external
+    payload: python.isort:lint
+    setup: python.isort:setup

tools/lint/mach_commands.py

@@ -23,7 +23,7 @@ if os.path.exists(thunderbird_excludes):
 
 GLOBAL_EXCLUDES = ["**/node_modules", "tools/lint/test/files", ".hg", ".git"]
 
-VALID_FORMATTERS = {"black", "clang-format", "rustfmt"}
+VALID_FORMATTERS = {"black", "clang-format", "rustfmt", "isort"}
 VALID_ANDROID_FORMATTERS = {"android-format"}
 
 # Code-review bot must index issues from the whole codebase when pushing

tools/lint/python/flake8.py

@@ -104,20 +104,11 @@ def lint(paths, config, **lintargs):
             "--recursive",
         ]
 
-        isort_cmd = [
-            os.path.join(virtualenv_bin_path or default_bindir(), "isort"),
-        ]
-
         if config.get("exclude"):
             fix_cmd.extend(["--exclude", ",".join(config["exclude"])])
-            isort_cmd.append("--filter-files")
-            for glob in config.get("exclude"):
-                isort_cmd.extend(["--skip", glob])
 
         subprocess.call(fix_cmd + paths)
 
-        subprocess.call(isort_cmd + paths)
-
         results = run(paths, config, **lintargs)
 
         fixed = fixed - len(results)

tools/lint/python/flake8_requirements.in

@@ -1,6 +1,4 @@
 flake8==5.0.4
-flake8-isort==4.2.0
-isort==5.10.1
 zipp==0.5
 autopep8==1.7.0
 typing-extensions==3.10.0.2

tools/lint/python/flake8_requirements.txt

@@ -11,19 +11,7 @@ autopep8==1.7.0 \
 flake8==5.0.4 \
     --hash=sha256:6fbe320aad8d6b95cec8b8e47bc933004678dc63095be98528b7bdd2a9f510db \
     --hash=sha256:7a1cf6b73744f5806ab95e526f6f0d8c01c66d7bbe349562d22dfca20610b248
-    # via
-    #   -r tools/lint/python/flake8_requirements.in
-    #   flake8-isort
-flake8-isort==4.2.0 \
-    --hash=sha256:26571500cd54976bbc0cf1006ffbcd1a68dd102f816b7a1051b219616ba9fee0 \
-    --hash=sha256:5b87630fb3719bf4c1833fd11e0d9534f43efdeba524863e15d8f14a7ef6adbf
     # via -r tools/lint/python/flake8_requirements.in
-isort==5.10.1 \
-    --hash=sha256:6f62d78e2f89b4500b080fe3a81690850cd254227f27f75c3a0c491a1f351ba7 \
-    --hash=sha256:e8443a5e7a020e9d7f97f1d7d9cd17c88bcb3bc7e218bf9cf5095fe550be2951
-    # via
-    #   -r tools/lint/python/flake8_requirements.in
-    #   flake8-isort
 mccabe==0.7.0 \
     --hash=sha256:348e0240c33b60bbdf4e523192ef919f28cb2c3d7d5c7794f74009290f236325 \
     --hash=sha256:6c2d30ab6be0e4a46919781807b4f0d834ebdd6c6e3dca0bda5a15f863427b6e

tools/lint/python/isort.py

@@ -0,0 +1,139 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+import configparser
+import os
+import platform
+import re
+import signal
+import subprocess
+import sys
+
+import mozpack.path as mozpath
+from mozlint import result
+from mozlint.pathutils import expand_exclusions
+from mozprocess import ProcessHandler
+
+here = os.path.abspath(os.path.dirname(__file__))
+ISORT_REQUIREMENTS_PATH = os.path.join(here, "isort_requirements.txt")
+
+ISORT_INSTALL_ERROR = """
+Unable to install correct version of isort
+Try to install it manually with:
+    $ pip install -U --require-hashes -r {}
+""".strip().format(
+    ISORT_REQUIREMENTS_PATH
+)
+
+
+def default_bindir():
+    # We use sys.prefix to find executables as that gets modified with
+    # virtualenv's activate_this.py, whereas sys.executable doesn't.
+    if platform.system() == "Windows":
+        return os.path.join(sys.prefix, "Scripts")
+    else:
+        return os.path.join(sys.prefix, "bin")
+
+
+def parse_issues(config, output, *, log):
+    would_sort = re.compile(
+        "^ERROR: (.*?) Imports are incorrectly sorted and/or formatted.$", re.I
+    )
+    sorted = re.compile("^Fixing (.*)$", re.I)
+    results = []
+    for line in output:
+        line = line.decode("utf-8")
+
+        match = would_sort.match(line)
+        if match:
+            res = {"path": match.group(1)}
+            results.append(result.from_config(config, **res))
+            continue
+
+        match = sorted.match(line)
+        if match:
+            res = {"path": match.group(1), "message": "sorted"}
+            results.append(result.from_config(config, **res))
+            continue
+
+        log.debug("Unhandled line", line)
+    return results
+
+
+class IsortProcess(ProcessHandler):
+    def __init__(self, config, *args, **kwargs):
+        self.config = config
+        kwargs["stream"] = False
+        ProcessHandler.__init__(self, *args, **kwargs)
+
+    def run(self, *args, **kwargs):
+        orig = signal.signal(signal.SIGINT, signal.SIG_IGN)
+        ProcessHandler.run(self, *args, **kwargs)
+        signal.signal(signal.SIGINT, orig)
+
+
+def run_process(config, cmd):
+    proc = IsortProcess(config, cmd)
+    proc.run()
+    try:
+        proc.wait()
+    except KeyboardInterrupt:
+        proc.kill()
+
+    return proc.output
+
+
+def setup(root, **lintargs):
+    virtualenv_manager = lintargs["virtualenv_manager"]
+    try:
+        virtualenv_manager.install_pip_requirements(ISORT_REQUIREMENTS_PATH, quiet=True)
+    except subprocess.CalledProcessError:
+        print(ISORT_INSTALL_ERROR)
+        return 1
+
+
+def lint(paths, config, **lintargs):
+    from isort import __version__ as isort_version
+
+    binary = os.path.join(
+        lintargs.get("virtualenv_bin_path") or default_bindir(), "isort"
+    )
+
+    log = lintargs["log"]
+    root = lintargs["root"]
+
+    log.debug("isort version {}".format(isort_version))
+
+    cmd_args = [
+        binary,
+        "--resolve-all-configs",
+        "--config-root",
+        root,
+    ]
+    if not lintargs.get("fix"):
+        cmd_args.append("--check-only")
+
+    # We merge exclusion rules from .flake8 to avoid having to repeat the same exclusions twice.
+    flake8_config_path = os.path.join(root, ".flake8")
+    flake8_config = configparser.ConfigParser()
+    flake8_config.read(flake8_config_path)
+    config["exclude"].extend(
+        mozpath.normpath(p.strip())
+        for p in flake8_config.get("flake8", "exclude").split(",")
+    )
+
+    paths = list(expand_exclusions(paths, config, lintargs["root"]))
+    if len(paths) == 0:
+        return {"results": [], "fixed": 0}
+
+    base_command = cmd_args + paths
+    log.debug("Command: {}".format(" ".join(base_command)))
+
+    output = run_process(config, base_command)
+
+    results = parse_issues(config, output, log=log)
+
+    fixed = sum(1 for issue in results if issue.message == "sorted")
+
+    return {"results": results, "fixed": fixed}

tools/lint/python/isort_requirements.in

@@ -0,0 +1 @@
+isort==5.10.1

tools/lint/python/isort_requirements.txt

@@ -0,0 +1,10 @@
+#
+# This file is autogenerated by pip-compile with python 3.10
+# To update, run:
+#
+#    pip-compile --generate-hashes --output-file=tools/lint/python/isort_requirements.txt tools/lint/python/isort_requirements.in
+#
+isort==5.10.1 \
+    --hash=sha256:6f62d78e2f89b4500b080fe3a81690850cd254227f27f75c3a0c491a1f351ba7 \
+    --hash=sha256:e8443a5e7a020e9d7f97f1d7d9cd17c88bcb3bc7e218bf9cf5095fe550be2951
+    # via -r tools/lint/python/isort_requirements.in

tools/lint/test/files/isort/.flake8

@@ -0,0 +1,4 @@
+[flake8]
+max-line-length = 100
+exclude =
+    subdir/exclude,

tools/lint/test/files/isort/bad.py

@@ -0,0 +1,8 @@
+import prova
+import collections
+
+
+def foobar():
+    c = collections.Counter()
+    prova.ciao(c)
+

tools/lint/test/files/isort/subdir/exclude/bad.py

@@ -0,0 +1,9 @@
+import collections
+
+import prova
+
+
+def foobar():
+    c = collections.Counter()
+    prova.ciao(c)
+

tools/lint/test/python.ini

@@ -29,3 +29,5 @@ requirements = tools/lint/rst/requirements.txt
 [test_shellcheck.py]
 [test_trojan_source.py]
 [test_yaml.py]
+[test_isort.py]
+requirements = tools/lint/python/isort_requirements.txt

tools/lint/test/test_flake8.py

@@ -39,12 +39,12 @@ def foobar():
 
     path = create_temp_file(contents, name="bad.py")
     results = lint([path])
-    assert len(results) == 3
+    assert len(results) == 2
 
     # Make sure the missing blank line is fixed, but the unused import isn't.
     results = lint([path], fix=True)
     assert len(results) == 1
-    assert fixed == 2
+    assert fixed == 1
 
     fixed = 0
 
@@ -53,7 +53,7 @@ def foobar():
     results = lint([path], fix=True)
     # There should now be two files with 2 combined errors
     assert len(results) == 2
-    assert fixed == 2
+    assert fixed == 1
     assert all(r.rule != "E501" for r in results)
 
 
@@ -113,25 +113,5 @@ def test_lint_uses_custom_extensions(lint, paths):
     assert len(lint(paths("ext/bad.configure"))) == 1
 
 
-def test_lint_isort(lint, create_temp_file):
-    contents = """
-import prova
-import collections
-
-
-def foobar():
-    c = collections.Counter()
-    prova.ciao(c)
-""".lstrip()
-
-    path = create_temp_file(contents, name="bad.py")
-    results = lint([path])
-    assert len(results) == 2
-    assert results[0].rule == "I003"
-    assert results[0].level == "warning"
-    assert results[1].rule == "I001"
-    assert results[1].level == "warning"
-
-
 if __name__ == "__main__":
     mozunit.main()

tools/lint/test/test_isort.py

@@ -0,0 +1,114 @@
+# -*- coding: utf-8 -*-
+
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+import os
+
+import mozunit
+
+LINTER = "isort"
+fixed = 0
+
+
+def test_lint_fix(lint, create_temp_file):
+    contents = """
+import prova
+import collections
+
+
+def foobar():
+    c = collections.Counter()
+    prova.ciao(c)
+""".lstrip()
+
+    path = create_temp_file(contents, name="bad.py")
+    results = lint([path])
+    assert len(results) == 1
+    assert results[0].level == "warning"
+
+    lint([path], fix=True)
+    assert fixed == 1
+
+
+def test_lint_excluded_file(lint, paths, config):
+    # Second file is excluded from .flake8 config.
+    files = paths("bad.py", "subdir/exclude/bad.py", "subdir/exclude/exclude_subdir")
+    results = lint(files, config)
+    assert len(results) == 1
+
+    # First file is globally excluded, second one is from .flake8 config.
+    files = paths("bad.py", "subdir/exclude/bad.py", "subdir/exclude/exclude_subdir")
+    config["exclude"] = paths("bad.py")
+    results = lint(files, config)
+    assert len(results) == 0
+
+    # Make sure excludes also apply when running from a different cwd.
+    cwd = paths("subdir")[0]
+    os.chdir(cwd)
+
+    results = lint(paths("subdir/exclude"))
+    assert len(results) == 0
+
+
+def test_lint_uses_all_configs(lint, paths, tmpdir):
+    myself = tmpdir.join("myself")
+    myself.mkdir()
+
+    flake8_path = tmpdir.join(".flake8")
+    flake8_path.write(
+        """
+[flake8]
+exclude =
+""".lstrip()
+    )
+
+    py_path = myself.join("good.py")
+    py_path.write(
+        """
+import os
+
+from myself import something_else
+from third_party import something
+
+
+def ciao():
+    pass
+""".lstrip()
+    )
+
+    results = lint([py_path.strpath])
+    assert len(results) == 0
+
+    isort_cfg_path = myself.join(".isort.cfg")
+    isort_cfg_path.write(
+        """
+[settings]
+known_first_party = myself
+""".lstrip()
+    )
+
+    results = lint([py_path.strpath], root=tmpdir.strpath)
+    assert len(results) == 1
+
+    py_path.write(
+        """
+import os
+
+from third_party import something
+
+from myself import something_else
+
+
+def ciao():
+    pass
+""".lstrip()
+    )
+
+    results = lint([py_path.strpath], root=tmpdir.strpath)
+    assert len(results) == 0
+
+
+if __name__ == "__main__":
+    mozunit.main()