Opened 4 months ago

Last modified 4 months ago

#35308 assigned Cleanup/optimization

FileNotFoundError escapes from run_formatters()

Reported by: Jacob Walls Owned by: Jeetu Singh
Component: Core (Management commands) Version: dev
Severity: Normal Keywords: black, code formatting
Cc: Jeetu Singh Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Jacob Walls)

A couple coworkers of mine have now run into this issue described on the Django forum.

In essence, it's not safe to assume that just because shutil.which("black") returns a path that subprocess.run() can execute that path.

Some complications with shutil.which(..., path=None):

  • Reads a path variable from one or two fallbacks (either os.environ() or os.defpath)
  • Behavior is different on Windows
  • Behavior is different on Windows with Python 3.12.0
  • Behavior is different on Windows with Python 3.12.1

My impression of the feature was that it was aiming at a frictionless experience -- format if you have a working black install, don't if you don't.

Suggesting we should catch FileNotFoundError (at least) and at most log out an explanation why the file wasn't formatted. Users who only have a copy of black in abandoned environments are unlikely to care about their files not being formatted. :-)

Change History (7)

comment:1 by Jacob Walls, 4 months ago

Description: modified (diff)

comment:2 by Mariusz Felisiak, 4 months ago

I cannot reproduce it on Linux. Is it Windows-specific issue? Did you manage to reproduce it? Is not this an issue in Python instead?

comment:3 by Jacob Walls, 4 months ago

My colleagues were developing on Macs. I just mentioned the Windows information when seeing it in the python docs.

This isn't a solid reproducer, but I managed to hack together an example that at least fails to launch. Something else must be responsible for escaping the FileNotFoundError, since the call to subprocess.run does not include check=True. (I'm not saying this is how my colleagues reproduced it they had black installs from in abandoned environments, unsure what versions of pip, black, homebrew etc were used):

~ % chmod 777 /opt/homebrew/bin/black
~ % chmod 644 /opt/homebrew/bin/black
~ % chmod +u /opt/homebrew/bin/black
~ % chmod -u /opt/homebrew/bin/black
~ % chmod +u /opt/homebrew/bin/black
~ % chmod +x /opt/homebrew/bin/black
~ % python3.12                       
Python 3.12.0 (v3.12.0:0fb18b02c8, Oct  2 2023, 09:45:56) [Clang 13.0.0 (clang-1300.0.29.30)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import shutil
>>> import subprocess
>>> subprocess.run(shutil.which('black'))
/opt/homebrew/Cellar/python@3.11/3.11.5/Frameworks/Python.framework/Versions/3.11/Resources/Python.app/Contents/MacOS/Python: can't open file '/opt/homebrew/bin/black': [Errno 13] Permission denied
CompletedProcess(args='/opt/homebrew/bin/black', returncode=2)

comment:4 by Natalia Bidart, 4 months ago

Component: UncategorizedCore (Management commands)
Keywords: code formatting added
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization
Version: 4.2dev

I can see value in handling these two exceptions (even in Linux):

  • PermissionError: somehow the path is not executable or can not be read by the user executing the formatting.
  • FileNotFoundError: black path disappeared between it was calculated by shutil.which and it was executed by subprocess.run (disk full, version upgrade, etc).

As long as we log the fact that we were about to run black but we couldn't, I think this is a welcomed improvement. Accepting on that basis (follow up of #33476).

comment:5 by Mariusz Felisiak, 4 months ago

I'm torn. Applying formatters should be smooth, on the other hand, hiding errors can make it more difficult to debug/notice potential issues in black installations.

comment:6 by Natalia Bidart, 4 months ago

I agree regarding visibility of errors, that's why I accepted the ticket as long as errors are "shown" somehow.

comment:7 by Jeetu Singh, 4 months ago

Cc: Jeetu Singh added
Owner: changed from nobody to Jeetu Singh
Status: newassigned
Note: See TracTickets for help on using tickets.
Back to Top