Skip to content
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

Fix absolute base python paths conflicting #3325

Merged
merged 1 commit into from
Aug 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ on:
workflow_dispatch:
push:
branches: ["main"]
tags-ignore: [ "**" ]
tags-ignore: ["**"]
pull_request:
schedule:
- cron: "0 8 * * *"
Expand Down Expand Up @@ -76,6 +76,7 @@ jobs:
- windows-latest
exclude:
- { os: windows-latest, tox_env: pkg_meta }
- { os: windows-latest, tox_env: docs }
steps:
- uses: actions/checkout@v4
with:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ jobs:

release:
needs:
- build
- build
runs-on: ubuntu-latest
environment:
name: release
Expand Down
13 changes: 10 additions & 3 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ repos:
rev: 0.29.1
hooks:
- id: check-github-workflows
args: [ "--verbose" ]
args: ["--verbose"]
- repo: https://github.com/codespell-project/codespell
rev: v2.3.0
hooks:
Expand All @@ -24,7 +24,7 @@ repos:
hooks:
- id: pyproject-fmt
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: "v0.5.6"
rev: "v0.5.7"
hooks:
- id: ruff-format
- id: ruff
Expand All @@ -33,11 +33,18 @@ repos:
rev: 1.18.0
hooks:
- id: blacken-docs
additional_dependencies: [black==24.4.2]
additional_dependencies: [black==24.8]
- repo: https://github.com/pre-commit/pygrep-hooks
rev: v1.10.0
hooks:
- id: rst-backticks
- repo: https://github.com/rbubley/mirrors-prettier
rev: "v3.3.3" # Use the sha / tag you want to point at
hooks:
- id: prettier
additional_dependencies:
- prettier@3.3.3
- "@prettier/plugin-xml@3.4.1"
- repo: local
hooks:
- id: changelogs-rst
Expand Down
1 change: 1 addition & 0 deletions docs/changelog/3325.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix absolute base python paths conflicting - by :user:`gaborbernat`.
3 changes: 2 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ lint.ignore = [
"D", # ignore documentation for now
"D203", # `one-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible
"D212", # `multi-line-summary-first-line` (D212) and `multi-line-summary-second-line` (D213) are incompatible
"DOC201", # broken with sphinx docs
"DOC201", # no restructuredtext support yet
"DOC402", # no restructuredtext support yet
"DOC501", # broken with sphinx docs
"INP001", # no implicit namespaces here
"ISC001", # conflicts with formatter
Expand Down
20 changes: 18 additions & 2 deletions src/tox/tox_env/python/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,14 @@ def _validate_base_python(
if env_base_python is not None:
spec_name = PythonSpec.from_string_spec(env_base_python)
for base_python in base_pythons:
if Path(base_python).is_absolute():
return [base_python]
spec_base = PythonSpec.from_string_spec(base_python)
if spec_base.path is not None:
path = Path(spec_base.path).absolute()
if str(spec_base.path) == sys.executable:
ver, is_64 = sys.version_info, sys.maxsize != 2**32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When reviewing fedora-python/tox-current-env#78 @frenzymadness found out that is_64 is always True:

Fedora Rawhide i686

>>> sys.maxsize
2147483647
>>> 2**32
4294967296
>>> sys.maxsize != 2**32
True

Fedora Rawhide x86_64

>>> sys.maxsize
9223372036854775807
>>> 2**32
4294967296
>>> sys.maxsize != 2**32
True

https://docs.python.org/3/library/platform.html#platform.architecture says:

To get at the “64-bitness” of the current interpreter, it is more reliable to query the sys.maxsize attribute:

is_64bits = sys.maxsize > 2**32

Fedora Rawhide i686

>>> sys.maxsize > 2**32
False

Fedora Rawhide x86_64

>>> sys.maxsize > 2**32
True

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good catch both. can you put in a PR to fix it 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am working on it. I assume you intended a string like cpython3.13-64.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct, thank you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drafted #3327 -- waiting for the CI to validate it.

spec_base = PythonSpec.from_string_spec(f"{sys.implementation}{ver.major}{ver.minor}-{is_64}")
Copy link
Contributor

@hroncok hroncok Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frenzymadness also pointed out that the string ends with a boolean, like ...313-True, while according to a PATTERN in virtualenv:

https://github.com/pypa/virtualenv/blob/95c5eed96c225faff01ac2f108368e0c4303989b/src/virtualenv/discovery/py_spec.py#L8

PATTERN = re.compile(r"^(?P<impl>[a-zA-Z]+)?(?P<version>[0-9.]+)?(?:-(?P<arch>32|64))?$")

It should probably end with -32 or -64 if we care about the 32/64bitness.

Moreover, when testing this assumption, I found out that the entire string is probably completely bogus:

>>> ver, is_64 = sys.version_info, sys.maxsize != 2**32
>>> f"{sys.implementation}{ver.major}{ver.minor}-{is_64}"
"namespace(name='cpython', cache_tag='cpython-313', version=sys.version_info(major=3, minor=13, micro=0, releaselevel='candidate', serial=1), hexversion=51183809, _multiarch='x86_64-linux-gnu')313-True"

We probably want to replace sys.implementation with sys.implementation.name if cpython313-64 is what you intended.

else:
spec_base = cls.python_spec_for_path(path)
if any(
getattr(spec_base, key) != getattr(spec_name, key)
for key in ("implementation", "major", "minor", "micro", "architecture")
Expand All @@ -183,6 +188,17 @@ def _validate_base_python(
raise Fail(msg)
return base_pythons

@classmethod
@abstractmethod
def python_spec_for_path(cls, path: Path) -> PythonSpec:
"""
Get the spec for an absolute path to a Python executable.

:param path: the path investigated
:return: the found spec
"""
raise NotImplementedError

@abstractmethod
def env_site_package_dir(self) -> Path:
"""
Expand Down
35 changes: 33 additions & 2 deletions src/tox/tox_env/python/virtual_env/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,14 @@

import os
import sys
from abc import ABC
from pathlib import Path
from typing import TYPE_CHECKING, Any, cast

from virtualenv import __version__ as virtualenv_version
from virtualenv import session_via_cli
from virtualenv import app_data, session_via_cli
from virtualenv.discovery import cached_py_info
from virtualenv.discovery.py_spec import PythonSpec

from tox.config.loader.str_convert import StrConvert
from tox.execute.local_sub_process import LocalSubProcessExecutor
Expand All @@ -17,13 +20,14 @@

if TYPE_CHECKING:
from virtualenv.create.creator import Creator
from virtualenv.discovery.py_info import PythonInfo as VirtualenvPythonInfo
from virtualenv.run.session import Session

from tox.execute.api import Execute
from tox.tox_env.api import ToxEnvCreateArgs


class VirtualEnv(Python):
class VirtualEnv(Python, ABC):
"""A python executor that uses the virtualenv project with pip."""

def __init__(self, create_args: ToxEnvCreateArgs) -> None:
Expand Down Expand Up @@ -167,3 +171,30 @@ def environment_variables(self) -> dict[str, str]:
environment_variables = super().environment_variables
environment_variables["VIRTUAL_ENV"] = str(self.conf["env_dir"])
return environment_variables

@classmethod
def python_spec_for_path(cls, path: Path) -> PythonSpec:
"""
Get the spec for an absolute path to a Python executable.

:param path: the path investigated
:return: the found spec
"""
info = cls.get_virtualenv_py_info(path)
return PythonSpec.from_string_spec(
f"{info.implementation}{info.version_info.major}{info.version_info.minor}-{info.architecture}"
)

@staticmethod
def get_virtualenv_py_info(path: Path) -> VirtualenvPythonInfo:
"""
Get the version info for an absolute path to a Python executable.

:param path: the path investigated
:return: the found information (cached)
"""
return cached_py_info.from_exe(
cached_py_info.PythonInfo,
app_data.make_app_data(None, read_only=False, env=os.environ),
str(path),
)
19 changes: 0 additions & 19 deletions tests/tox_env/python/test_python_api.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
from __future__ import annotations

import os
import sys
from typing import TYPE_CHECKING, Callable
from unittest.mock import patch
Expand Down Expand Up @@ -126,24 +125,6 @@ def test_base_python_env_no_conflict(env: str, base_python: list[str], ignore_co
assert result is base_python


@pytest.mark.parametrize(
("env", "base_python", "platform"),
[
("py312-unix", ["/opt/python312/bin/python"], "posix"),
("py312-win", [r"C:\Program Files\Python312\python.exe"], "nt"),
("py311-win", [r"\\a\python311\python.exe"], "nt"),
("py310-win", [r"\\?\UNC\a\python310\python.exe"], "nt"),
("py310", ["//a/python310/bin/python"], None),
],
ids=lambda a: "|".join(a) if isinstance(a, list) else str(a),
)
def test_base_python_absolute(env: str, base_python: list[str], platform: str | None) -> None:
if platform and platform != os.name:
pytest.skip(f"Not applicable to this platform. ({platform} != {os.name})")
result = Python._validate_base_python(env, base_python, False) # noqa: SLF001
assert result == base_python


@pytest.mark.parametrize("ignore_conflict", [True, False])
@pytest.mark.parametrize(
("env", "base_python", "expected", "conflict"),
Expand Down
Loading