-
-
Notifications
You must be signed in to change notification settings - Fork 526
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
Changes from all commits
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 |
---|---|---|
|
@@ -29,7 +29,7 @@ jobs: | |
|
||
release: | ||
needs: | ||
- build | ||
- build | ||
runs-on: ubuntu-latest | ||
environment: | ||
name: release | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fix absolute base python paths conflicting - by :user:`gaborbernat`. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
spec_base = PythonSpec.from_string_spec(f"{sys.implementation}{ver.major}{ver.minor}-{is_64}") | ||
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. @frenzymadness also pointed out that the string ends with a boolean, like PATTERN = re.compile(r"^(?P<impl>[a-zA-Z]+)?(?P<version>[0-9.]+)?(?:-(?P<arch>32|64))?$") It should probably end with 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 |
||
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") | ||
|
@@ -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: | ||
""" | ||
|
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.
When reviewing fedora-python/tox-current-env#78 @frenzymadness found out that
is_64
is always True:Fedora Rawhide i686
Fedora Rawhide x86_64
https://docs.python.org/3/library/platform.html#platform.architecture says:
Fedora Rawhide i686
Fedora Rawhide x86_64
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.
Ah good catch both. can you put in a PR to fix it 😅
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.
I am working on it. I assume you intended a string like
cpython3.13-64
.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.
That is correct, thank you.
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.
Drafted #3327 -- waiting for the CI to validate it.