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

Try alternate filenames for system_executable #2442

Merged
merged 1 commit into from Nov 11, 2022

Conversation

vfazio
Copy link
Contributor

@vfazio vfazio commented Nov 7, 2022

The value of sys._base_executable may not be a real file due to changes made in CPython 3.11. The value is derived from the current executable name and the "home" key from pyvenv.cfg.

On POSIX systems, virtual environments deploy python for use within the venv however CPython's make install and a number of distributions do not provide a system "python" in part because of PEP 394.

Virtualenv exposes this via PythonInfo.system_executable and can encounter issues when attempting to execute a non-existent file.

Attempt to fallback to python<MAJOR> and python<MAJOR>.<MINOR> if python does not exist.

Thanks for contributing, make sure you address all the checklists (for details on how see development documentation)!

  • ran the linter to address style issues (tox -e fix_lint)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

@vfazio vfazio mentioned this pull request Nov 7, 2022
@vfazio
Copy link
Contributor Author

vfazio commented Nov 7, 2022

I'll add a news entry later but wanted to get this up to get feedback

@gaborbernat
Copy link
Contributor

test too please

@vfazio
Copy link
Contributor Author

vfazio commented Nov 7, 2022

test too please

I haven't quite figured out how i'm going to be able to test this.

@vfazio vfazio force-pushed the vfazio-fallback-python3 branch 5 times, most recently from 32a7343 to 0a01a54 Compare November 10, 2022 02:11
@vfazio vfazio marked this pull request as ready for review November 10, 2022 02:12
# search relative to the directory of sys._base_executable
base_dir = os.path.dirname(base_executable)
for base_executable in [
os.path.join(base_dir, "{}".format(exe))
Copy link
Contributor

Choose a reason for hiding this comment

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

exe is already a string we don't need this format, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gaborbernat you're absolutely correct and this is a good example of why i shouldn't be making commits late at night.

Do you prefer subsequent commits to address comments or can i amend and force push this branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in the latest push

diff --git a/src/virtualenv/discovery/py_info.py b/src/virtualenv/discovery/py_info.py
index a2eb633..2a39e50 100644
--- a/src/virtualenv/discovery/py_info.py
+++ b/src/virtualenv/discovery/py_info.py
@@ -151,12 +151,11 @@ class PythonInfo(object):
                             # search relative to the directory of sys._base_executable
                             base_dir = os.path.dirname(base_executable)
                             for base_executable in [
-                                os.path.join(base_dir, "{}".format(exe))
+                                os.path.join(base_dir, exe)
                                 for exe in ("python{}".format(major), "python{}.{}".format(major, minor))
                             ]:
                                 if os.path.exists(base_executable):
                                     return base_executable
-                        return None  # File doesn't exist and no alternate was available
             return None  # in this case we just can't tell easily without poking around FS and calling them, bail
         # if we're not in a virtual environment, this is already a system python, so return the original executable
         # note we must choose the original and not the pure executable as shim scripts might throw us off

]:
if os.path.exists(base_executable):
return base_executable
return None # File doesn't exist and no alternate was available
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this line as would then go to next line that already does this, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you're correct, i just wasn't sure if that was a convention you were ok with. i'll fix shortly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in the latest push

diff --git a/src/virtualenv/discovery/py_info.py b/src/virtualenv/discovery/py_info.py
index a2eb633..2a39e50 100644
--- a/src/virtualenv/discovery/py_info.py
+++ b/src/virtualenv/discovery/py_info.py
@@ -151,12 +151,11 @@ class PythonInfo(object):
                             # search relative to the directory of sys._base_executable
                             base_dir = os.path.dirname(base_executable)
                             for base_executable in [
-                                os.path.join(base_dir, "{}".format(exe))
+                                os.path.join(base_dir, exe)
                                 for exe in ("python{}".format(major), "python{}.{}".format(major, minor))
                             ]:
                                 if os.path.exists(base_executable):
                                     return base_executable
-                        return None  # File doesn't exist and no alternate was available
             return None  # in this case we just can't tell easily without poking around FS and calling them, bail
         # if we're not in a virtual environment, this is already a system python, so return the original executable
         # note we must choose the original and not the pure executable as shim scripts might throw us off

@vfazio
Copy link
Contributor Author

vfazio commented Nov 10, 2022

The ubuntu pipelines are also failing here due to nushell

@gaborbernat
Copy link
Contributor

The ubuntu pipelines are also failing here due to nushell

Reached out to them separately, hopefully they fix it soon.

@vfazio
Copy link
Contributor Author

vfazio commented Nov 10, 2022

The ubuntu pipelines are also failing here due to nushell

Reached out to them separately, hopefully they fix it soon.

@gaborbernat
It looks like as part of 0.71 they're now including a subdirectory in their releases archives which is actually pretty typical for most releases i've seen from other projects. 0.70 did not have a subdirectory

vfazio@vfazio2 ~/Downloads :( $ tar -tvf nu-0.70.0-x86_64-unknown-linux-gnu.tar.gz
-rw-r--r-- runner/docker  1094 2022-10-18 14:11 LICENSE
-rw-r--r-- runner/docker   124 2022-10-18 14:11 README.txt
-rwxr-xr-x runner/docker 70621816 2022-10-18 14:11 nu
-rwxr-xr-x runner/docker  2752208 2022-10-18 14:11 nu_plugin_custom_values
-rwxr-xr-x runner/docker  2699672 2022-10-18 14:11 nu_plugin_example
-rwxr-xr-x runner/docker  6006152 2022-10-18 14:11 nu_plugin_gstat
-rwxr-xr-x runner/docker  2130760 2022-10-18 14:11 nu_plugin_inc
-rwxr-xr-x runner/docker  4950368 2022-10-18 14:11 nu_plugin_query

vfazio@vfazio2 ~/Downloads $ tar -tvf nu-0.71.0-x86_64-unknown-linux-gnu.tar.gz 
drwxr-xr-x runner/docker     0 2022-11-08 12:11 nu-0.71.0-x86_64-unknown-linux-gnu/
-rw-r--r-- runner/docker  1094 2022-11-08 12:11 nu-0.71.0-x86_64-unknown-linux-gnu/LICENSE
-rwxr-xr-x runner/docker 2050928 2022-11-08 12:11 nu-0.71.0-x86_64-unknown-linux-gnu/nu_plugin_inc
-rw-r--r-- runner/docker     124 2022-11-08 12:11 nu-0.71.0-x86_64-unknown-linux-gnu/README.txt
-rwxr-xr-x runner/docker 65975456 2022-11-08 12:11 nu-0.71.0-x86_64-unknown-linux-gnu/nu
-rwxr-xr-x runner/docker  2655216 2022-11-08 12:11 nu-0.71.0-x86_64-unknown-linux-gnu/nu_plugin_example
-rwxr-xr-x runner/docker  4827000 2022-11-08 12:11 nu-0.71.0-x86_64-unknown-linux-gnu/nu_plugin_query
-rwxr-xr-x runner/docker  2708240 2022-11-08 12:11 nu-0.71.0-x86_64-unknown-linux-gnu/nu_plugin_custom_values
-rwxr-xr-x runner/docker  5961760 2022-11-08 12:11 nu-0.71.0-x86_64-unknown-linux-gnu/nu_plugin_gstat

So i think the options are:

@gaborbernat
Copy link
Contributor

gaborbernat commented Nov 10, 2022

Trying this out here https://github.com/pypa/virtualenv/actions/runs/3438815774; seems it does not work 🤔

@vfazio
Copy link
Contributor Author

vfazio commented Nov 10, 2022

Trying this out here https://github.com/pypa/virtualenv/actions/runs/3438815774; seems it does not work thinking

@gaborbernat You have to place the commandline option somewhere else. -f expects a filename immediately after so it's treating --strip-compoments=1 as a filename instead of an argument

https://www.gnu.org/software/tar/manual/html_section/transform.html

@vfazio
Copy link
Contributor Author

vfazio commented Nov 10, 2022

Surprisingly, my cpython fix got merged and was backported into 3.11 so this likely won't be as big of an issue in starting in 3.11.1 and 3.12.

It's still worthwhile to perform this check because cpython will still return a base executable that may not exist. The primary fix for that would be #2441 to ensure it searches in the right path. The code introduced here still only applies to 3.11 going forward and based on our discussion should still return None if the file does not exist.

1 similar comment
@vfazio
Copy link
Contributor Author

vfazio commented Nov 10, 2022

Surprisingly, my cpython fix got merged and was backported into 3.11 so this likely won't be as big of an issue in starting in 3.11.1 and 3.12.

It's still worthwhile to perform this check because cpython will still return a base executable that may not exist. The primary fix for that would be #2441 to ensure it searches in the right path. The code introduced here still only applies to 3.11 going forward and based on our discussion should still return None if the file does not exist.

The value of `sys._base_executable` may not be a real file due to
changes made in CPython 3.11. The value is derived from the current
executable name and the "home" key from pyvenv.cfg.

On POSIX systems, virtual environments deploy "python" for use within
the venv however CPython's `make install` and a number of distributions
do not provide a system "python" in part because of PEP 394.

Virtualenv exposes this via `PythonInfo.system_executable` and can
encounter issues when attempting to execute a non-existent file.

Attempt to fallback to "python<MAJOR>" and "python<MAJOR>.<MINOR>" if
"python" does not exist.

Signed-off-by: Vincent Fazio <vfazio@gmail.com>
@gaborbernat gaborbernat merged commit 50b4b6b into pypa:main Nov 11, 2022
github-actions bot added a commit to MaRDI4NFDI/open-interfaces that referenced this pull request Nov 14, 2022
Bumps [virtualenv](https://github.com/pypa/virtualenv) from 20.16.6 to
20.16.7.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a
href="https://github.com/pypa/virtualenv/releases">virtualenv's
releases</a>.</em></p>
<blockquote>
<h2>20.16.7</h2>
<h2>What's Changed</h2>
<ul>
<li>release 20.16.6 by <a
href="https://github.com/gaborbernat"><code>@​gaborbernat</code></a> in
<a
href="https://github-redirect.dependabot.com/pypa/virtualenv/pull/2437">pypa/virtualenv#2437</a></li>
<li>Replace six in tests/unit/test_run.py by <a
href="https://github.com/cjmayo"><code>@​cjmayo</code></a> in <a
href="https://github-redirect.dependabot.com/pypa/virtualenv/pull/2439">pypa/virtualenv#2439</a></li>
<li>Try to fix Nushell install by <a
href="https://github.com/kubouch"><code>@​kubouch</code></a> in <a
href="https://github-redirect.dependabot.com/pypa/virtualenv/pull/2444">pypa/virtualenv#2444</a></li>
<li>Try alternate filenames for system_executable by <a
href="https://github.com/vfazio"><code>@​vfazio</code></a> in <a
href="https://github-redirect.dependabot.com/pypa/virtualenv/pull/2442">pypa/virtualenv#2442</a></li>
<li>Bump embedded by <a
href="https://github.com/gaborbernat"><code>@​gaborbernat</code></a> in
<a
href="https://github-redirect.dependabot.com/pypa/virtualenv/pull/2443">pypa/virtualenv#2443</a></li>
<li>Set 'home' to parent directory of system_executable by <a
href="https://github.com/vfazio"><code>@​vfazio</code></a> in <a
href="https://github-redirect.dependabot.com/pypa/virtualenv/pull/2441">pypa/virtualenv#2441</a></li>
</ul>
<h2>New Contributors</h2>
<ul>
<li><a href="https://github.com/cjmayo"><code>@​cjmayo</code></a> made
their first contribution in <a
href="https://github-redirect.dependabot.com/pypa/virtualenv/pull/2439">pypa/virtualenv#2439</a></li>
<li><a href="https://github.com/vfazio"><code>@​vfazio</code></a> made
their first contribution in <a
href="https://github-redirect.dependabot.com/pypa/virtualenv/pull/2442">pypa/virtualenv#2442</a></li>
</ul>
<p><strong>Full Changelog</strong>: <a
href="https://github.com/pypa/virtualenv/compare/20.16.6...20.16.7">https://github.com/pypa/virtualenv/compare/20.16.6...20.16.7</a></p>
</blockquote>
</details>
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a
href="https://github.com/pypa/virtualenv/blob/main/docs/changelog.rst">virtualenv's
changelog</a>.</em></p>
<blockquote>
<h2>v20.16.7 (2022-11-12)</h2>
<p>Bugfixes - 20.16.7</p>
<pre><code>- Use parent directory of python executable for pyvenv.cfg
&quot;home&quot; value per PEP 405 - by :user:`vfazio`.
(`[#2440](pypa/virtualenv#2440)
&lt;https://github.com/pypa/virtualenv/issues/2440&gt;`_)
- In POSIX virtual environments, try alternate binary names if
``sys._base_executable`` does not exist - by :user:`vfazio`.
(`[#2442](pypa/virtualenv#2442)
&lt;https://github.com/pypa/virtualenv/issues/2442&gt;`_)
- Upgrade embedded wheel to ``0.38.4`` and pip to ``22.3.1`` from
``22.3`` and setuptools to ``65.5.1`` from
``65.5.0`` - by :user:`gaborbernat`.
(`[#2443](pypa/virtualenv#2443)
&lt;https://github.com/pypa/virtualenv/issues/2443&gt;`_)
</code></pre>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/pypa/virtualenv/commit/d536b25fd38699a15cb9d782be2786d2aec4de83"><code>d536b25</code></a>
release 20.16.7</li>
<li><a
href="https://github.com/pypa/virtualenv/commit/cf36e4f485fe06c9b52dd657c02b84867e4b8bce"><code>cf36e4f</code></a>
Set 'home' to parent directory of system_executable (<a
href="https://github-redirect.dependabot.com/pypa/virtualenv/issues/2441">#2441</a>)</li>
<li><a
href="https://github.com/pypa/virtualenv/commit/b7178cdbfe1e6b59734137c142ae260673698a0e"><code>b7178cd</code></a>
Bump embedded (<a
href="https://github-redirect.dependabot.com/pypa/virtualenv/issues/2443">#2443</a>)</li>
<li><a
href="https://github.com/pypa/virtualenv/commit/50b4b6b72a0709501b36f714e317b07c3d765833"><code>50b4b6b</code></a>
Try alternate filenames for system_executable (<a
href="https://github-redirect.dependabot.com/pypa/virtualenv/issues/2442">#2442</a>)</li>
<li><a
href="https://github.com/pypa/virtualenv/commit/dbc57c2bd6c9d23d482bc7400ddb6c7c7022de09"><code>dbc57c2</code></a>
Try to fix Nushell install (<a
href="https://github-redirect.dependabot.com/pypa/virtualenv/issues/2444">#2444</a>)</li>
<li><a
href="https://github.com/pypa/virtualenv/commit/e4a42c6eeb48004461bb8711fa06e830bc40eb37"><code>e4a42c6</code></a>
Fix nushell install</li>
<li><a
href="https://github.com/pypa/virtualenv/commit/c13db75469d6d39778049930431f3496c1e2c4ac"><code>c13db75</code></a>
Fix documentation build</li>
<li><a
href="https://github.com/pypa/virtualenv/commit/2224abb367286cd2d6b888b0e22ee4d108ec6b74"><code>2224abb</code></a>
Replace six in tests/unit/test_run.py (<a
href="https://github-redirect.dependabot.com/pypa/virtualenv/issues/2439">#2439</a>)</li>
<li><a
href="https://github.com/pypa/virtualenv/commit/1e90b97f70f03b1fc6bde3b187bdc9d9f4590f7e"><code>1e90b97</code></a>
Simplify docs conf</li>
<li><a
href="https://github.com/pypa/virtualenv/commit/ba6ac17c9b058aadcddd95e2a3ec689a2c3c9b69"><code>ba6ac17</code></a>
Bump tools and deps</li>
<li>Additional commits viewable in <a
href="https://github.com/pypa/virtualenv/compare/20.16.6...20.16.7">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=virtualenv&package-manager=pip&previous-version=20.16.6&new-version=20.16.7)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants