Return a better error message if a file: URL is not found

[DiddiLeija]

Closes #10237. Print better error messages if the file/URL is not found.

[DiddiLeija]

@uranusjr I requested a review from you, because you suggested this change. Tell me if I can do something else.

[pradyunsg]

Got an example of before/after with this change?

[DiddiLeija]

Well, as we were saying on #10237, using a bad file: specifier returns a weird error message:

AttributeError: 'FileNotFoundError' object has no attribute 'read'

This is because the file object (that must be usually returned) is set to the exception raised:

pip/src/pip/_internal/network/session.py

Lines 218 to 235 in 87aee20

	try:
	stats = os.stat(pathname)
	except OSError as exc:
	resp.status_code = 404
	resp.raw = exc
	else:
	modified = email.utils.formatdate(stats.st_mtime, usegmt=True)
	content_type = mimetypes.guess_type(pathname)[0] or "text/plain"
	resp.headers = CaseInsensitiveDict(
	{
	"Content-Type": content_type,
	"Content-Length": stats.st_size,
	"Last-Modified": modified,
	}
	)
	resp.raw = open(pathname, "rb")
	resp.close = resp.raw.close

So, I am trying to set the resp.raw to a file object containing the exception to avoid that problem.

But, taking a closer look, I am not sure if the exception will be raised when set, or it will be taken as the requirements file, causing another kind of bug 🤔. Maybe we can directly raise (instead of returning a file object), or even catch the exception after returned to raise it on a "more confortable" way.

[uranusjr]

But, taking a closer look, I am not sure if the exception will be raised when set, or it will be taken as the requirements file

pip checks the response code of an HTTP request, and a 404 Not Found code will make it emit a nice response:

$ pip install https://pypi.org/project/12345/
Collecting https://pypi.org/project/12345/
  ERROR: HTTP error 404 while getting https://pypi.org/project/12345/
ERROR: Could not install requirement https://pypi.org/project/12345/ because of HTTP error 404 Client Error: Not Found for url: https://pypi.org/project/12345/ for URL https://pypi.org/project/12345/

I don’t know what the message would look like for a file: URL (because we could never reach that error due to this bug) but you can try it out and tweak the format to make sense.

[DiddiLeija]

I don’t know what the message would look like for a file: URL (because we could never reach that error due to this bug) but you can try it out and tweak the format to make sense.

Ok, I'll try it.

[DiddiLeija]

I tried to run pip install -r file:requirements.txt under this branch and got this message:

ERROR: 404 Client Error: None for url: file:///requirements.txt

It seems like pip is interpreting the message as we want. The message sounds reasonable for me, but tell me if this sounds confusing for you.

[uranusjr]

I wonder where that None part came from and if we could replace to with FileNotFoundError (which should be pretty recognisable to any Python developer) or something similar.

[DiddiLeija]

I tried a simple code (similar to the real one) to test the current format for these errors and got this:

>>>import io, os
>>># build a try/except block that fails to see the message
>>>try:
...    f = os.stat("some_imaginary_file.txt")
...except OSError as exc:
...    error_message = f"{type(exc).__name__}: {str(exc)}"
...    raw = io.BytesIO(error_message.encode("utf8"))
...
>>>raw.read()
b"FileNotFoundError: [WinError 2] El sistema no puede encontrar el archivo especificado: 'some_imaginary_file.txt'"

(I am using a mexican Windows computer with Python 3.8.2, but this example shows how the exception must look like).

On the above example, raw.read() (that represents resp.raw.read()) returns a reasonable exception for me. Considering this, I consider that this exception:

ERROR: 404 Client Error: None for url: file:///requirements.txt

could be derived from the exception we are fixing, because it looks really different. I think requests is doing something else with resp.raw on this specific case.

[uranusjr]

could be derived from the exception we are fixing, because it looks really different. I think requests is doing something else with resp.raw on this specific case.

It makes sense, because Response.raw would contain the page content, which is usually a bunch of unintelligible HTML code. We’ll probably need to dig into requests to find out where and how that exception message is formatted.

[uranusjr]

Oh actually it’s our own code (not requests), and it’s Response.reason. So we should set that.

pip/src/pip/_internal/network/utils.py

Lines 29 to 54 in 330d0aa

	def raise_for_status(resp: Response) -> None:
	http_error_msg = ""
	if isinstance(resp.reason, bytes):
	# We attempt to decode utf-8 first because some servers
	# choose to localize their reason strings. If the string
	# isn't utf-8, we fall back to iso-8859-1 for all other
	# encodings.
	try:
	reason = resp.reason.decode("utf-8")
	except UnicodeDecodeError:
	reason = resp.reason.decode("iso-8859-1")
	else:
	reason = resp.reason
	if 400 <= resp.status_code < 500:
	http_error_msg = (
	f"{resp.status_code} Client Error: {reason} for url: {resp.url}"
	)
	elif 500 <= resp.status_code < 600:
	http_error_msg = (
	f"{resp.status_code} Server Error: {reason} for url: {resp.url}"
	)
	if http_error_msg:
	raise NetworkConnectionError(http_error_msg, response=resp)

[DiddiLeija]

I set resp.reason to error_message:

https://github.com/DiddiLeija/pip/blob/filenotfound-error-message/src/pip/_internal/network/session.py#L226

[DiddiLeija]

With the newest changes, the message looks like this on my computer:

ERROR: 404 Client Error: "FileNotFoundError: [WinError 2] El sistema no puede encontrar el archivo especificado: 'file:////requirements.txt'" for url: file:///requirements.txt

Is this message reasonable for you? Or we must do something else?

[DiddiLeija]

So, what do you think about this pull request?

[xavfernandez]

Any chance to have a test for that ? :)

[DiddiLeija]

I can create a test, if you want.

[DiddiLeija]

Hmm... something went wrong. I will re-trigger the builds, to see what happens.

[DiddiLeija]

I'm a bit confused by the pre-commit error message:

Check builtin type constructor use.......................................Failed
- hook id: check-builtin-literals
- exit code: 1

Traceback (most recent call last):
  File "/home/runner/.cache/pre-commit/repob0bwwgn2/py_env-python3.9/bin/check-builtin-literals", line 8, in <module>
    sys.exit(main())
  File "/home/runner/.cache/pre-commit/repob0bwwgn2/py_env-python3.9/lib/python3.9/site-packages/pre_commit_hooks/check_builtin_literals.py", line 90, in main
    calls = check_file(
  File "/home/runner/.cache/pre-commit/repob0bwwgn2/py_env-python3.9/lib/python3.9/site-packages/pre_commit_hooks/check_builtin_literals.py", line 63, in check_file
    tree = ast.parse(f.read(), filename=filename)
  File "/opt/hostedtoolcache/Python/3.9.6/x64/lib/python3.9/ast.py", line 50, in parse
    return compile(source, filename, mode, flags,
  File "tests/functional/test_bad_url.py", line 25
    def test_filenotfound_error_message() -> None:
    ^
SyntaxError: invalid syntax

Debug Statements (Python)................................................Failed
- hook id: debug-statements
- exit code: 1

tests/functional/test_bad_url.py - Could not parse ast

	Traceback (most recent call last):
	  File "/home/runner/.cache/pre-commit/repob0bwwgn2/py_env-python3.9/lib/python3.9/site-packages/pre_commit_hooks/debug_statement_hook.py", line 55, in check_file
	    ast_obj = ast.parse(f.read(), filename=filename)
	  File "/opt/hostedtoolcache/Python/3.9.6/x64/lib/python3.9/ast.py", line 50, in parse
	    return compile(source, filename, mode, flags,
	  File "tests/functional/test_bad_url.py", line 25
	    def test_filenotfound_error_message() -> None:
	    ^
	SyntaxError: invalid syntax

flake8...................................................................Failed
- hook id: flake8
- exit code: 1

tests/functional/test_bad_url.py:1:1: E902 TokenError: EOF in multi-line statement

Any ideas to fix this?

[pfmoore]

return base + "".join(random.choice(alphabet) for _ in range(10) is missing a trailing parenthesis.

[DiddiLeija]

Now, I only have this problem with flake8:

tests/functional/test_bad_url.py:30:43: E228 missing whitespace around modulo operator

I looked around the flake8 docs, and then to the pycodestyle docs, but I didn't find information to fix this.

[pfmoore]

That line is command = ["pip", "install", "file:%s"%file]. The error is "missing whitespace around modulo operator". The modulo operator is the %, so I'd assume it wants command = ["pip", "install", "file:%s" % file]. Also, why are you using %-formatting rather than format() or f-strings?

Are you able to run the lint commands locally? It would be better to fix the various lint errors before updating the PR, as it's a lot more costly triggering a new CI run for every typo 🙁

[pradyunsg]

@uranusjr You added this to the 21.3 milestone. Do you think this is a release blocker, or is that more an indicator of when you'd like this to be done by?

[uranusjr]

Not a release blocker, I just thought we'd want to review this and include it if it's ready.

[pradyunsg]

Whomever merges this, please squash merge this one.

[DiddiLeija]

Thanks @pradyunsg! I was sleeping when you pushed the changes (I woke up late today...), so thanks for doing it! :)

[DiddiLeija]

Thanks @pradyunsg! :D