misleading error "Failed to load Noxfile noxfile.py, no such file exists."

[weatherfrog]

Describe the bug
If a FileNotFoundError occurs somewhere on module-level within noxfile.py, nox will display a misleading error:

misleading error "Failed to load Noxfile noxfile.py, no such file exists."

The problem is here: https://github.com/theacodes/nox/blob/main/nox/tasks.py#L109
This except clause is harmful. Something like this would solve the problem:

if not Path(global_config.noxfile).exists():
    logger.error(
        f"Failed to load Noxfile {global_config.noxfile}, no such file exists."
    )
    return 2

Shall I create a PR?

How to reproduce
noxfile.py:

from pathlib import Path

import nox

requirements = Path("/non-existing/path/requirements.txt").read_text()

@nox.session(python=["3.9"])
def foo(session):
    pass

$ nox -s foo
nox > Failed to load Noxfile [...]/noxfile.py, no such file exists.

Expected behavior

The stack trace of the actual error should be visible.

[FollowTheProcess]

@weatherfrog Thanks for raising this! I agree the given error is not exactly helpful. PR's are always welcome!

Just thinking about this in more detail, would something like this work for you:

❯ nox
nox > File declared in <path to noxfile> not found: /non-existing/path/requirements.txt

This could be implemented with something along the lines of:

except FileNotFoundError as err:
    if err.filename.lower() == global_config.noxfile.lower():
        message = (
            f"Failed to load Noxfile {global_config.noxfile}, no such file exists."
        )
    else:
        message = (
            f"File declared in {global_config.noxfile} not found: {err.filename}"
        )
    logger.error(message)
    return 2

This has the added advantages that:

• An upfront check is not done every time the noxfile is loaded (only when the exception is hit)
• The user is given some help on the offending file/directory

What do you think?

[weatherfrog]

@FollowTheProcess Thanks for the immediate response. I agree, the upfront check is not great. I like your proposition. The only problem I'm seeing is that you never get to see the stack trace, i.e., you won't know where the error occurred. I think I would just go with

except FileNotFoundError as err:
    if err.filename.lower() == global_config.noxfile.lower():
        logger.error(
            f"Failed to load Noxfile {global_config.noxfile}, no such file exists."
        )
        return 2
    else:
        raise err

I mean, other exceptions are not caught either. So why catch general FileNotFoundErrors?

But I can live with your variant, too, of course. It's your library. You decide. 😀

[cjolowicz]

If the Noxfile cannot be read, the FileNotFoundError is raised during the check_nox_version call, so before we load the Noxfile as a Python module (_load_and_exec_nox_module). In fact, all of the except clauses relate to exceptions raised before we load the Python module: FileNotFoundError, OSError (e.g. wrong file permissions), VersionCheckFailed, InvalidVersionSpecifier.

Would it make sense then to move the call to _load_and_exec_nox_module past the entire try...except block? That should resolve this, no?

[FollowTheProcess]

@cjolowicz That's a great idea!

I've just experimented with putting _load_and_exec_nox_module in the else clause of the try...except on @weatherfrog's noxfile and it has the desired effect:

❯ nox
Traceback (most recent call last):
  File "/Users/tomfleet/Development/nox/.venv/bin/nox", line 33, in <module>
    sys.exit(load_entry_point('nox', 'console_scripts', 'nox')())
  File "/Users/tomfleet/Development/nox/nox/__main__.py", line 47, in main
    exit_code = workflow.execute(
  File "/Users/tomfleet/Development/nox/nox/workflow.py", line 51, in execute
    return_value = function_(*args, global_config=global_config)
  File "/Users/tomfleet/Development/nox/nox/tasks.py", line 116, in load_nox_module
    return _load_and_exec_nox_module(global_config)
  File "/Users/tomfleet/Development/nox/nox/tasks.py", line 66, in _load_and_exec_nox_module
    loader.exec_module(module)  # type: ignore[attr-defined]
  File "<frozen importlib._bootstrap_external>", line 883, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/Users/tomfleet/Development/nox/noxfile.py", line 5, in <module>
    requirements = Path("/non-existing/path/requirements.txt").read_text()
  File "/Users/tomfleet/.pyenv/versions/3.10.2/lib/python3.10/pathlib.py", line 1132, in read_text
    with self.open(mode='r', encoding=encoding, errors=errors) as f:
  File "/Users/tomfleet/.pyenv/versions/3.10.2/lib/python3.10/pathlib.py", line 1117, in open
    return self._accessor.open(self, mode, buffering, encoding, errors,
FileNotFoundError: [Errno 2] No such file or directory: '/non-existing/path/requirements.txt'

The only additional thing is we might want to change the exceptions raised in _load_and_exec_nox_module as they were deliberately chosen to hit the except blocks (OSError). These relate to pretty fatal internal importlib type stuff (not being able to load the module manifest for example) so maybe a RunTimeError or similar? Realistically, we should never hit these cases, and if we do it means importlib is broken so we'd have bigger problems! 😂

[cjolowicz]

The only additional thing is we might want to change the exceptions raised in _load_and_exec_nox_module as they were deliberately chosen to hit the except blocks (OSError). These relate to pretty fatal internal importlib type stuff (not being able to load the module manifest for example) so maybe a RunTimeError or similar? Realistically, we should never hit these cases, and if we do it means importlib is broken so we'd have bigger problems! 😂

I'm not familiar with the importlib.util API, but what you're describing sounds like those checks should be assertions. Or possibly better omitted entirely? Under which conditions would these checks fail? AFAICS the recipe in the Python library reference omits these checks.

[cjolowicz]

Realistically, we should never hit these cases, and if we do it means importlib is broken

To me, this means that these checks should be omitted.

If there really is an unexpected issue in importlib.util, let it crash, don't taper over it. It would be different for known bugs, of course.

[FollowTheProcess]

Yeah I think you're right, to be honest the only real reason I put them in originally was to cover off the None branch of some of the functions. For example importlib.util.spec_from_file_location returns ModuleSpec | None but importlib.util.module_from_spec requires a ModuleSpec.

There's a few more e.g. spec.loader returns a Loader | None etc. etc. Short of littering it with # type: ignore I think I had trouble satisfying mypy if I remember correctly.

[cjolowicz]

Oh I see. So the return types generally allow None, but we know that None would only be returned in case of a bug (or a broken environment). Asserting is not None is a good instrument to deal with this. The assertion narrows the type, so mypy can infer that the variable has e.g. type ModuleSpec after the assertion.