-
-
Notifications
You must be signed in to change notification settings - Fork 151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add nox.needs_version to specify Nox version requirements #388
Changes from 14 commits
885a1aa
78ffeff
cdf3599
57922a6
5b72c8c
b768b1d
2e492b3
90befd5
f74839d
6a401c4
2e03ab0
162afa0
db0cbb8
2681677
966a39f
3b879a8
0f26270
0b02729
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,117 @@ | ||
# Copyright 2021 Alethea Katherine Flowers | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
import ast | ||
import contextlib | ||
import sys | ||
from typing import Optional | ||
|
||
from packaging.specifiers import InvalidSpecifier, SpecifierSet | ||
from packaging.version import InvalidVersion, Version | ||
|
||
try: | ||
import importlib.metadata as metadata | ||
except ImportError: # pragma: no cover | ||
import importlib_metadata as metadata | ||
|
||
|
||
class VersionCheckFailed(Exception): | ||
"""The Nox version does not satisfy what ``nox.needs_version`` specifies.""" | ||
|
||
|
||
class InvalidVersionSpecifier(Exception): | ||
"""The ``nox.needs_version`` specifier cannot be parsed.""" | ||
|
||
|
||
def get_nox_version() -> str: | ||
"""Return the version of the installed Nox package.""" | ||
return metadata.version("nox") | ||
|
||
|
||
def _parse_string_constant(node: ast.AST) -> Optional[str]: # pragma: no cover | ||
"""Return the value of a string constant.""" | ||
if sys.version_info < (3, 8): | ||
if isinstance(node, ast.Str) and isinstance(node.s, str): | ||
return node.s | ||
elif isinstance(node, ast.Constant) and isinstance(node.value, str): | ||
return node.value | ||
return None | ||
|
||
|
||
def _parse_needs_version(source: str, filename: str = "<unknown>") -> Optional[str]: | ||
"""Parse ``nox.needs_version`` from the user's noxfile.""" | ||
value: Optional[str] = None | ||
module: ast.Module = ast.parse(source, filename=filename) | ||
for statement in module.body: | ||
if isinstance(statement, ast.Assign): | ||
for target in statement.targets: | ||
if ( | ||
isinstance(target, ast.Attribute) | ||
and isinstance(target.value, ast.Name) | ||
and target.value.id == "nox" | ||
and target.attr == "needs_version" | ||
): | ||
value = _parse_string_constant(statement.value) | ||
return value | ||
|
||
|
||
def _read_needs_version(filename: str) -> Optional[str]: | ||
"""Read ``nox.needs_version`` from the user's noxfile.""" | ||
with open(filename) as io: | ||
source = io.read() | ||
|
||
return _parse_needs_version(source, filename=filename) | ||
|
||
|
||
def _check_nox_version_satisfies(needs_version: str) -> None: | ||
"""Check if the Nox version satisfies the given specifiers.""" | ||
version = Version(get_nox_version()) | ||
|
||
try: | ||
specifiers = SpecifierSet(needs_version) | ||
except InvalidSpecifier as error: | ||
message = f"Cannot parse `nox.needs_version`: {error}" | ||
with contextlib.suppress(InvalidVersion): | ||
Version(needs_version) | ||
message += f", did you mean '>= {needs_version}'?" | ||
raise InvalidVersionSpecifier(message) | ||
|
||
if not specifiers.contains(version, prereleases=True): | ||
raise VersionCheckFailed( | ||
f"The Noxfile requires nox {specifiers}, you have {version}" | ||
) | ||
|
||
|
||
def check_nox_version(filename: Optional[str] = None) -> None: | ||
"""Check if ``nox.needs_version`` in the user's noxfile is satisfied. | ||
|
||
Args: | ||
filename: The location of the user's noxfile. When specified, read | ||
``nox.needs_version`` from the noxfile by parsing the AST. | ||
Otherwise, assume that the noxfile was already imported, and | ||
use ``nox.needs_version`` directly. | ||
|
||
Raises: | ||
VersionCheckFailed: The Nox version does not satisfy what | ||
``nox.needs_version`` specifies. | ||
InvalidVersionSpecifier: The ``nox.needs_version`` specifier cannot be | ||
parsed. | ||
""" | ||
from nox import needs_version | ||
|
||
if filename is not None: | ||
needs_version = _read_needs_version(filename) # noqa: F811 | ||
|
||
if needs_version is not None: | ||
_check_nox_version_satisfies(needs_version) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ | |
import nox | ||
from colorlog.escape_codes import parse_colors | ||
from nox import _options, registry | ||
from nox._version import InvalidVersionSpecifier, VersionCheckFailed, check_nox_version | ||
from nox.logger import logger | ||
from nox.manifest import WARN_PYTHONS_IGNORED, Manifest | ||
from nox.sessions import Result | ||
|
@@ -51,15 +52,26 @@ def load_nox_module(global_config: Namespace) -> Union[types.ModuleType, int]: | |
os.path.expandvars(global_config.noxfile) | ||
) | ||
|
||
# Check ``nox.needs_version`` by parsing the AST. | ||
check_nox_version(global_config.noxfile) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious what the performance impact is of parsing the AST for this twice every time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right, thanks for mentioning this, I had wanted to check that. I used cProfile 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 |
||
|
||
# Move to the path where the Noxfile is. | ||
# This will ensure that the Noxfile's path is on sys.path, and that | ||
# import-time path resolutions work the way the Noxfile author would | ||
# guess. | ||
os.chdir(os.path.realpath(os.path.dirname(global_config.noxfile))) | ||
return importlib.machinery.SourceFileLoader( | ||
module = importlib.machinery.SourceFileLoader( | ||
"user_nox_module", global_config.noxfile | ||
).load_module() # type: ignore | ||
|
||
# Check ``nox.needs_version`` as set by the Noxfile. | ||
check_nox_version() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, why is this done twice? When would this return something different from the AST parsing version? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anytime the user does something more complex than assigning a string literal to Here are some examples:
The last example shows that the AST-based version check could actually lead to a I'm not sure how problematic this is, what do you think? For now, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I should maybe elaborate more on why we would bother to parse the AST instead of |
||
|
||
return module | ||
|
||
except (VersionCheckFailed, InvalidVersionSpecifier) as error: | ||
logger.error(str(error)) | ||
return 2 | ||
except (IOError, OSError): | ||
logger.exception("Failed to load Noxfile {}".format(global_config.noxfile)) | ||
return 2 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,131 @@ | ||
# Copyright 2021 Alethea Katherine Flowers | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
from pathlib import Path | ||
from textwrap import dedent | ||
from typing import Optional | ||
|
||
import pytest | ||
from nox import needs_version | ||
from nox._version import ( | ||
InvalidVersionSpecifier, | ||
VersionCheckFailed, | ||
_parse_needs_version, | ||
check_nox_version, | ||
get_nox_version, | ||
) | ||
|
||
|
||
def test_needs_version_default() -> None: | ||
"""It is None by default.""" | ||
assert needs_version is None | ||
|
||
|
||
def test_get_nox_version() -> None: | ||
"""It returns something that looks like a Nox version.""" | ||
result = get_nox_version() | ||
year, month, day = [int(part) for part in result.split(".")[:3]] | ||
assert year >= 2020 | ||
|
||
|
||
@pytest.mark.parametrize( | ||
"text,expected", | ||
[ | ||
("", None), | ||
( | ||
dedent( | ||
""" | ||
import nox | ||
nox.needs_version = '>=2020.12.31' | ||
""" | ||
), | ||
">=2020.12.31", | ||
), | ||
( | ||
dedent( | ||
""" | ||
import nox | ||
nox.needs_version = 'bogus' | ||
nox.needs_version = '>=2020.12.31' | ||
""" | ||
), | ||
">=2020.12.31", | ||
), | ||
( | ||
dedent( | ||
""" | ||
import nox.sessions | ||
nox.needs_version = '>=2020.12.31' | ||
""" | ||
), | ||
">=2020.12.31", | ||
), | ||
( | ||
dedent( | ||
""" | ||
import nox as _nox | ||
_nox.needs_version = '>=2020.12.31' | ||
""" | ||
), | ||
None, | ||
), | ||
], | ||
) | ||
def test_parse_needs_version(text: str, expected: Optional[str]) -> None: | ||
"""It is parsed successfully.""" | ||
assert expected == _parse_needs_version(text) | ||
|
||
|
||
@pytest.mark.parametrize("specifiers", ["", ">=2020.12.31", ">=2020.12.31,<9999.99.99"]) | ||
def test_check_nox_version_succeeds(tmp_path: Path, specifiers: str) -> None: | ||
"""It does not raise if the version specifiers are satisfied.""" | ||
text = dedent( | ||
f""" | ||
import nox | ||
nox.needs_version = "{specifiers}" | ||
""" | ||
) | ||
path = tmp_path / "noxfile.py" | ||
path.write_text(text) | ||
check_nox_version(str(path)) | ||
|
||
|
||
@pytest.mark.parametrize("specifiers", [">=9999.99.99"]) | ||
def test_check_nox_version_fails(tmp_path: Path, specifiers: str) -> None: | ||
"""It raises an exception if the version specifiers are not satisfied.""" | ||
text = dedent( | ||
f""" | ||
import nox | ||
nox.needs_version = "{specifiers}" | ||
""" | ||
) | ||
path = tmp_path / "noxfile.py" | ||
path.write_text(text) | ||
with pytest.raises(VersionCheckFailed): | ||
check_nox_version(str(path)) | ||
|
||
|
||
@pytest.mark.parametrize("specifiers", ["invalid", "2020.12.31"]) | ||
def test_check_nox_version_invalid(tmp_path: Path, specifiers: str) -> None: | ||
"""It raises an exception if the version specifiers cannot be parsed.""" | ||
text = dedent( | ||
f""" | ||
import nox | ||
nox.needs_version = "{specifiers}" | ||
""" | ||
) | ||
path = tmp_path / "noxfile.py" | ||
path.write_text(text) | ||
with pytest.raises(InvalidVersionSpecifier): | ||
check_nox_version(str(path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
nox
isNox
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 966a39f