Add nox.needs_version to specify Nox version requirements

[cjolowicz]

Exit with a friendly error if the Nox version does not satisfy the requirements
specified by the user's Noxfile in nox.needs_version:

• Add dependency on packaging to Nox install_requires
• Add nox.needs_version attribute with PEP 440 version specifiers for Nox
• Add nox._version module with these functions and classes:
  • function get_nox_version
  • function check_nox_version
  • exception VersionCheckFailed
  • exception InvalidVersionSpecifier
• Adapt tasks.load_nox_module to invoke check_nox_version
• Refactor __main__ to use get_nox_version for --version option
• Tests and documentation

Closes #99
Supercedes #210

Some words about the approach taken here:

1. Rather than a minimum required version, needs_version accepts any PEP 440
   version specifier. Mostly, this just seemed a better conceptual match. If
   users specify a version V, Nox will suggest to use >=V instead.

   Version specifiers are also a much more expressive device. For example, a
   Noxfile could specify an upper bound if it has not been adapted to a breaking
   change in a newer Nox version. It could exclude a version with a bug that it
   was affected by. Nox, on its part, could offer a compatibility interface for
   older Noxfiles, should a large breaking change to the Nox API ever become
   necessary.

2. Version requirements are checked in two passes. In the first pass, Nox
   attempts to determine nox.needs_version from the AST, using a best effort
   approach; if found, Nox checks the version a first time. The second pass
   occurs after the Noxfile was imported, and checks the version against
   nox.needs_version directly. The rationale is that we should avoid executing
   user code that wasn't written for our version. While parsing the AST should
   cover the majority of uses, the AST parser is kept deliberately simple;
   anything more complex than assigning a string literal is left for the second
   pass. A regular expression could have covered most of these use cases as
   well; however, parsing the AST seemed a better fit and not overly complex.

3. The implementation uses packaging instead of the deprecated distutils
   module. LooseVersion has been removed from the packaging API, and
   should not be needed anyway because, as a PyPI package, Nox versions conform
   to PEP 440.

[theacodes]

This looks really good, I just have a few questions.

[theacodes]

Nit: nox is Nox

[cjolowicz]

Fixed in 966a39f

[theacodes]

I'm curious what the performance impact is of parsing the AST for this twice every time.

[cjolowicz]

I'm curious what the performance impact is of parsing the AST for this twice every time.

Right, thanks for mentioning this, I had wanted to check that. I used cProfile
to measure performance for the Noxfiles of Nox (116 lines) and pip (334 lines).

These are the results for Nox:

❯ python -m cProfile -s cumtime -m nox -l | grep -E 'ncalls|builtins.exec|tasks.py|ast.py'
   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
     83/1    0.000    0.000    0.068    0.068 {built-in method builtins.exec}
        1    0.000    0.000    0.053    0.053 tasks.py:15(<module>)
        1    0.000    0.000    0.001    0.001 tasks.py:32(load_nox_module)
        1    0.000    0.000    0.001    0.001 ast.py:33(parse)
        1    0.000    0.000    0.000    0.000 tasks.py:94(discover_manifest)
        1    0.000    0.000    0.000    0.000 tasks.py:156(honor_list_request)
        1    0.000    0.000    0.000    0.000 tasks.py:80(merge_noxfile_options)
        1    0.000    0.000    0.000    0.000 tasks.py:115(filter_manifest)

These are the results for pip:

❯ python -m cProfile -s cumtime -m nox -l | grep -E 'ncalls|builtins.exec|tasks.py|ast.py'
   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
     88/1    0.000    0.000    0.071    0.071 {built-in method builtins.exec}
        1    0.000    0.000    0.052    0.052 tasks.py:15(<module>)
        1    0.000    0.000    0.006    0.006 tasks.py:32(load_nox_module)
        1    0.000    0.000    0.002    0.002 ast.py:33(parse)
        1    0.000    0.000    0.000    0.000 tasks.py:94(discover_manifest)
        1    0.000    0.000    0.000    0.000 tasks.py:156(honor_list_request)
        1    0.000    0.000    0.000    0.000 tasks.py:80(merge_noxfile_options)
        1    0.000    0.000    0.000    0.000 tasks.py:115(filter_manifest)

So parsing the AST adds around 1-2 milliseconds to startup time. Given that it
only happens once (in addition to when the Noxfile is imported), this would be
imperceptible.

[theacodes]

Hmm, why is this done twice? When would this return something different from the AST parsing version?

[cjolowicz]

Anytime the user does something more complex than assigning a string literal to nox.needs_version.

Here are some examples:

NOX_VERSION = ">=2022.1.1"
nox.needs_version = NOX_VERSION

import nox as _nox
_nox.needs_version = ">=2022.1.1"

nox.needs_version = ">=2022.1.1"

if sys.platform == "win32":
    nox.needs_version = None

The last example shows that the AST-based version check could actually lead to a
false positive: It would return ">=2022.1.1", ignoring the assignment in the
if clause. On Windows, Nox older than 2022.1.1 would therefore bail out early,
before even importing the Noxfile. It would never see the if clause that
permits any version on this platform.

I'm not sure how problematic this is, what do you think? For now, the
documentation suggests to assign a simple string literal, and I expect this is
what most users would do anyway.

[cjolowicz]

I should maybe elaborate more on why we would bother to parse the AST instead of
just importing the Noxfile. The idea is to avoid executing user code that wasn't
written for our version (as far as we can detect this). #210 took a different
approach here: Import the Noxfile, and only parse the source code if the import
failed. I think both approaches have their merits and drawbacks, and I'm happy
to take this either way.

[theacodes]

Okay, I actually had some time to sit down and read and process this.

I'm not sure how I feel about the two-phase approach. It's clever, but I want to think through some scenarios. The one I'm most worried about is the situation where the user has an incompatible version of Nox, but the Noxfile specifies it in a way that the AST approach can't read. Nox will try to import the module, but, if there's some incompatibility that rears its head during import (like us adding a new @nox.{something} decorator`, then it's going to fail with an unhelpful message.

I'm curious how you feel about only doing the AST check and explicitly documenting that it only works if it is, verbatim, nox.needs_version = "{spec}"?

[cjolowicz]

I'm curious how you feel about only doing the AST check and explicitly documenting that it only works if it is, verbatim, nox.needs_version = "{spec}"?

Yes, that makes sense to me. By dropping the dynamic version of needs_version,
we're able to give the user a clear message about its use and behavior. I
updated the PR to implement this, see 3b879a8. Does this address your concerns?

This leaves the door open to re-introduce the second pass, should a compelling
use case for dynamic needs_version ever come up. In that case, the
documentation would need to make it abundantly clear that the static and dynamic
versions should not be mixed, something like this:

There are two ways to specify ``nox.needs_version``:

1. Assign a string literal to it at the module level. This is the preferred
   method and allows Nox to check the version without importing the Noxfile.
2. Any other valid Python code that sets the ``nox.needs_version`` attribute. This
   method is provided if you need to specify version requirements dynamically.
   **Important**: The two methods cannot be mixed. If you set ``nox.needs_version``
   dynamically, you must not assign a string literal to it at the module level.

[theacodes]

This looks great. I made a few little changes and I'll merge when all is green.

[theacodes]

Thank you @cjolowicz. This PR qualifies for a payout of $50 from our OpenCollective. Let me know if you have any questions about that.

[cjolowicz]

I am a bit worried I might be using up a budget that could serve as an incentive for other contributors as well. Especially given that this appears to rely on donations of individuals (not corporate funding). So I think I’ll just pass on the money this time around. However, thank you so much for doing this on top of building a positive and welcoming community!

[theacodes]

Of course, it's totally optional. (I'm also planning on donating via Winterbloom, so don't sweat the budget too much)

…

On Wed, Feb 24, 2021, 6:46 AM Claudio Jolowicz ***@***.***> wrote: I am a bit worried I might be using up a budget that could serve as an incentive for other contributors as well. Especially given that this appears to rely on donations of individuals (not corporate funding). So I think I’ll just pass on the money this time around. However, thank you so much for doing this on top of building a positive and welcoming community! — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#388 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5I42ZPQUQTSH42ZDPIN3TATRIXANCNFSM4XYCB5FA> .