Closed Bug 1660338 Opened 4 years ago Closed 4 years ago

black python linter is unhappy with virtualenvs for some reason

Categories

(Developer Infrastructure :: Lint and Formatting, defect, P3)

Tracking

(firefox84 fixed)

RESOLVED FIXED
84 Branch
Tracking Status
firefox84 --- fixed

People

(Reporter: Gijs, Assigned: andi)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

$ ./mach lint -l black tools/lint/hooks.py --fix
Traceback (most recent call last):
  File "path/to/mozilla-central/python/mozlint/mozlint/roller.py", line 60, in _run_worker
    res = func(paths, config, **lintargs) or []
  File "path/to/mozilla-central/python/mozlint/mozlint/types.py", line 54, in __call__
    return self._lint(paths, config, **lintargs)
  File "path/to/mozilla-central/python/mozlint/mozlint/types.py", line 143, in _lint
    return func(files, config, **lintargs)
  File "path/to/mozilla-central/tools/lint/python/black.py", line 129, in lint
    return run_black(config, files, fix=fix, log=lintargs["log"])
  File "path/to/mozilla-central/tools/lint/python/black.py", line 116, in run_black
    log.debug("Black version {}".format(get_black_version(binary)))
  File "path/to/mozilla-central/tools/lint/python/black.py", line 43, in get_black_version
    output = subprocess.check_output(
  File "/usr/local/Cellar/python@3.8/3.8.5/Frameworks/Python.framework/Versions/3.8/lib/python3.8/subprocess.py", line 411, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/usr/local/Cellar/python@3.8/3.8.5/Frameworks/Python.framework/Versions/3.8/lib/python3.8/subprocess.py", line 489, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/usr/local/Cellar/python@3.8/3.8.5/Frameworks/Python.framework/Versions/3.8/lib/python3.8/subprocess.py", line 854, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/local/Cellar/python@3.8/3.8.5/Frameworks/Python.framework/Versions/3.8/lib/python3.8/subprocess.py", line 1702, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: '$HOME/.mozbuild/_virtualenvs/mach/bin/../bin/black'
A failure occurred in the black linter.
✖ 1 problem (0 errors, 0 warnings, 1 failure)
Blocks: mach-busted

The severity field is not set for this bug.
:ahal, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(ahal)

Fwiw, I'm unable to reproduce on Linux (WSL). Perhaps it's a MacOS issue? I noticed a couple black related patches landing recently, so there's a chance it got fixed since this was filed. Are you still able to reproduce?

I also CC'ed Sylvestre and Ricky who may have more context.

Severity: -- → S3
Flags: needinfo?(ahal)
Priority: -- → P3

Ah, I saw something like this in CI, and I fixed it in CI with https://phabricator.services.mozilla.com/D87904.

A LOT of changes have happened in virtualenvs over the past 19 days, so first of all I would check whether you're still encountering the issue.

If it is, it's because the lint codebase uses pip to install black in the virtualenv you're using, but if pip determines that black is already installed, then pip won't do anything. Then the lint codebase attempts to locate black in the virtualenv directory -- but if pip didn't install it, it won't be there.

The "correct" solution to the problem would be to call into black with python -m black which is guaranteed to work in that case, rather than attempting to locate a binary that may not exist.

I still see this on central rev d4e11195e398 , from yesterday.

However, running pip3 uninstall black and then re-running the command yields the same error.

Is there anything else I can do/check to help here?

Flags: needinfo?(rstewart)

Does this patch fix it?

diff --git a/tools/lint/python/black.py b/tools/lint/python/black.py
index 7a78f5b44b77..0712052a924a 100644
--- a/tools/lint/python/black.py
+++ b/tools/lint/python/black.py
@@ -35,20 +35,21 @@ else:
     bindir = os.path.join(sys.prefix, "bin")
 
 
-def get_black_version(binary):
+def get_black_version(cmd):
     """
-    Returns found binary's version
+    Returns the version of black. cmd is a list of the form
+    ["python", "-m", "black"].
     """
     try:
         output = subprocess.check_output(
-            [binary, "--version"],
+            cmd + ["--version"],
             stderr=subprocess.STDOUT,
             universal_newlines=True,
         )
     except subprocess.CalledProcessError as e:
         output = e.output
 
-    return re.match(r"black, version (.*)$", output)[1]
+    return re.search(r", version (.*)$", output)[1]
 
 
 def parse_issues(config, output, paths, *, log):
@@ -113,11 +114,11 @@ def setup(root, **lintargs):
 
 
 def run_black(config, paths, fix=None, *, log):
-    binary = os.path.join(bindir, "black")
+    cmd = [os.path.join(bindir, "python"), "-m", "black"]
 
-    log.debug("Black version {}".format(get_black_version(binary)))
+    log.debug("Black version {}".format(get_black_version(cmd)))
 
-    cmd_args = [binary]
+    cmd_args = list(cmd)
     if not fix:
         cmd_args.append("--check")
     base_command = cmd_args + paths
Flags: needinfo?(rstewart) → needinfo?(gijskruitbosch+bugs)

Apologies for the delay. With that I get:

Traceback (most recent call last):
  File "python/mozlint/mozlint/roller.py", line 60, in _run_worker
    res = func(paths, config, **lintargs) or []
  File "python/mozlint/mozlint/types.py", line 54, in __call__
    return self._lint(paths, config, **lintargs)
  File "python/mozlint/mozlint/types.py", line 143, in _lint
    return func(files, config, **lintargs)
  File "tools/lint/python/black.py", line 132, in lint
    return run_black(config, files, fix=fix, log=lintargs["log"])
  File "tools/lint/python/black.py", line 119, in run_black
    log.debug("Black version {}".format(get_black_version(cmd)))
  File "tools/lint/python/black.py", line 52, in get_black_version
    return re.search(r", version (.*)$", output)[1]
TypeError: 'NoneType' object is not subscriptable

If I modify the end of get_black_version to:

    versionmatch = re.search(r", version (.*)$", output)
    return versionmatch[1] if versionmatch else None

then it no longer errors out, ie I get:

✖ 0 problems (0 errors, 0 warnings)

though tbh I don't understand how it would then work... and a naive attempt to introduce issues into a python file (really long line) doesn't seem to be able to trigger actual issues...

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(rstewart)

If I print e.output in the catch clause I still get:

$HOME/.mozbuild/_virtualenvs/mach/bin/../bin/python: No module named black

I'm confused about why the virtualenv it's hitting is the in-~/.mozbuild one. It should be using the $objdir/_virtualenvs/init_py3 virtualenv. Is there anything funny about your environment I should be aware of that would be causing this?

Flags: needinfo?(rstewart)

If it wasn't clear: 1. I can't reproduce that at HEAD, and I've verified that running mach lint on my machine uses the in-objdir virtualenv as I mentioned. and 2. This is the cause of the issue, so if we determine why your mach lint is doing this weird thing, that'll fix the problem.

Assigning to Andi after a conversation on chat.m.o.

We've isolated the issue to how mach lint subprocesses into sub-jobs. We use a ProcessPoolExecutor which may start new processes that end up loading black.py multiple times. When they do, the variable binpath will be instantiated in those subprocesses to an incorrect value (as the changes that activate_this.py makes to sys.prefix don't persist in subprocesses). This results in the behavior we see here, namely, trying to open the black binary at the incorrect path.

Assignee: nobody → bpostelnicu

If you're looking for a good place to pass down a "virtualenv_path" or similar key, I'd do it here:
https://searchfox.org/mozilla-central/source/tools/lint/mach_commands.py#83

This way it stays out of mozlint core, and linters can find it in the lintargs dict. There's precedent too as we set other build related configuration (maybe the virtualenv can even be derived from the build config we already pass in).

I pulled today, and this issue broke my lint command. I'm on macOS.

If anyone needs a workaround, this worked on my machine:

 ~/.mozbuild/_virtualenvs/mach/bin/pip install -r tools/lint/python/black_requirements.txt
 ~/.mozbuild/_virtualenvs/mach/bin/pip install -r tools/lint/python/flake8_requirements.txt

The logic the black and flake8 linters were using to find the location of the appropriate binaries for linting was wrong in certain cases given how mach lint uses subprocesses to batch work. Instead, we allow the option to override the old janky behavior with a known-good path.

Pushed by ahalberstadt@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cc3064995922 Pass down location of `virtualenv` `bin` directory from `mach lint` invocation down to linters r=ahal
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: