[Django] #35308: FileNotFoundError escapes from run_formatters()

[Django]

#35308: FileNotFoundError escapes from run_formatters()
-----------------------------------------+------------------------
Reporter: Jacob Walls | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 4.2
Severity: Normal | Keywords: black
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
Two members of my team have now run into
[https://forum.djangoproject.com/t/black-library-error/20897/3 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 [https://docs.python.org/3/library/shutil.html#shutil.which
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. :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/35308>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35308: FileNotFoundError escapes from run_formatters()

-------------------------------+--------------------------------------

Reporter: Jacob Walls | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 4.2

Severity: Normal | Resolution:

Keywords: black | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------+--------------------------------------
Description changed by Jacob Walls:

Old description:

> Two members of my team have now run into
> [https://forum.djangoproject.com/t/black-library-error/20897/3 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 [https://docs.python.org/3/library/shutil.html#shutil.which
> 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. :-)

New description:

A couple coworkers of mine have now run into

[https://forum.djangoproject.com/t/black-library-error/20897/3 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 [https://docs.python.org/3/library/shutil.html#shutil.which
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. :-)

--

--
Ticket URL: <https://code.djangoproject.com/ticket/35308#comment:1>

[Django]

#35308: FileNotFoundError escapes from run_formatters()
-------------------------------+--------------------------------------
Reporter: Jacob Walls | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 4.2
Severity: Normal | Resolution:
Keywords: black | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------

Comment (by Mariusz Felisiak):

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?
--
Ticket URL: <https://code.djangoproject.com/ticket/35308#comment:2>

[Django]

#35308: FileNotFoundError escapes from run_formatters()
-------------------------------+--------------------------------------
Reporter: Jacob Walls | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 4.2
Severity: Normal | Resolution:
Keywords: black | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------

Comment (by Jacob Walls):

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/pyt...@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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35308#comment:3>

[Django]

#35308: FileNotFoundError escapes from run_formatters()

-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: nobody

Type: | Status: new
Cleanup/optimization |
Component: Core (Management | Version: dev
commands) |
Severity: Normal | Resolution:
Keywords: black, code | Triage Stage: Accepted
formatting |

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Natalia Bidart):

* component: Uncategorized => Core (Management commands)
* keywords: black => black, code formatting
* stage: Unreviewed => Accepted
* type: Bug => Cleanup/optimization
* version: 4.2 => dev

Comment:

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).
--
Ticket URL: <https://code.djangoproject.com/ticket/35308#comment:4>

[Django]

#35308: FileNotFoundError escapes from run_formatters()
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Core (Management | Version: dev
commands) |
Severity: Normal | Resolution:
Keywords: black, code | Triage Stage: Accepted
formatting |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak):

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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35308#comment:5>

[Django]

#35308: FileNotFoundError escapes from run_formatters()
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Core (Management | Version: dev
commands) |
Severity: Normal | Resolution:
Keywords: black, code | Triage Stage: Accepted
formatting |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Natalia Bidart):

I agree regarding visibility of errors, that's why I accepted the ticket
as long as errors are "shown" somehow.
--
Ticket URL: <https://code.djangoproject.com/ticket/35308#comment:6>

[Django]

#35308: FileNotFoundError escapes from run_formatters()
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: Jeetu
Type: | Singh
Cleanup/optimization | Status: assigned

Component: Core (Management | Version: dev
commands) |
Severity: Normal | Resolution:
Keywords: black, code | Triage Stage: Accepted
formatting |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jeetu Singh):

* cc: Jeetu Singh (added)
* owner: nobody => Jeetu Singh
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/35308#comment:7>