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

Set 'home' to parent directory of system_executable #2441

Merged
merged 1 commit into from
Nov 12, 2022

Conversation

vfazio
Copy link
Contributor

@vfazio vfazio commented Nov 6, 2022

PEP 405 says of the "home" key:

"If a home key is found, this signifies that the Python binary belongs to a virtual environment, and the value of the home key is the directory containing the Python executable used to create this virtual environment."

And:

"In this case, prefix-finding continues as normal using the value of the home key as the effective Python binary location, which finds the prefix of the base installation."

Previously, "home" was being set to interpreter.system_exec_prefix which does not abide by the PEP specification.

In Python 3.11, the "home" directory is used to determine the value of sys._base_executable, so if the path specified is incorrect, the path + interpreter returned will be invalid. This can cause headaches later when trying to probe info via the discovery module.

Now, set this to the parent directory of interpreter.system_executable.

close #2440

It may not resolve the problem fully as virtualenv cannot control the sys._base_executable "calculation" short of specifying the home directory. This means there's still a chance that a path to "python" gets returned even though there may not be one available in the parent-most non-venv python directory (the python interpreter path that created the upper-most venv).

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

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Needs test too 👍

@vfazio
Copy link
Contributor Author

vfazio commented Nov 6, 2022

Needs test too 👍

I don't mind stashing a test in some current unittest, but when i glanced around i didn't see any existing tests really validating the contents of pyvenv.cfg.

Do you have a suggestion on where to place a test for this?

@vfazio vfazio force-pushed the vfazio-set-home-to-bin-path branch 2 times, most recently from 8f5b8ee to 6843472 Compare November 7, 2022 20:51
@vfazio vfazio requested a review from gaborbernat November 7, 2022 20:55
@vfazio vfazio force-pushed the vfazio-set-home-to-bin-path branch from 6843472 to c12355b Compare November 8, 2022 18:54
@vfazio
Copy link
Contributor Author

vfazio commented Nov 8, 2022

I'm a little confused by the failure on the macos-12 3.8 pipeline that doesn't occur in 3.9. I don't have a mac to test and debug this on.

One thing I could do is skip this for venv since it technically has the ability to overwrite this key from what is provided in the base class

    def set_pyenv_cfg(self):
        # prefer venv options over ours, but keep our extra
        venv_content = copy(self.pyenv_cfg.refresh())
        super().set_pyenv_cfg()
        self.pyenv_cfg.update(venv_content)

otherwise i'm not sure what the nuance is. The native venv should be doing the same we're doing https://github.com/python/cpython/blob/3.8/Lib/venv/__init__.py#L109

I see

using tox-3.27.0 from /usr/local/lib/python3.8/site-packages/tox/__init__.py (pid 5370)
/usr/local/opt/[email protected]/bin/python3.8 (/usr/local/opt/[email protected]/bin/python3.8) is {'executable': '/usr/local/opt/[email protected]/bin/python3.8', 'implementation': 'CPython', 'version_info': [3, 8, 15, 'final', 0], 'version': '3.8.15 (default, Oct 11 2022, 21:52:37) \n[Clang 14.0.0 (clang-1400.0.29.102)]', 'is_64': True, 'sysplatform': 'darwin', 'os_sep': '/', 'extra_version_info': None}

unless there's a symlink between /usr/local/Cellar/ and /usr/local/opt/ and it's getting resolved in one code path and not in another but the only reasonable place that could be happening is in python's getpath code when setting base_executable as part of:

https://github.com/python/cpython/blob/3.8/Modules/getpath.c#L1234

which happens before

https://github.com/python/cpython/blob/3.8/Modules/getpath.c#L1242

unless the test I wrote isn't actually testing what i thought it was

@gaborbernat
Copy link
Contributor

Seems we're failing on ubuntu too, can you take a look?

@vfazio
Copy link
Contributor Author

vfazio commented Nov 10, 2022

Seems we're failing on ubuntu too, can you take a look?

Looks related to installing nushell:

2022-11-10 02:37:39 (119 MB/s) - ‘nushell.tar.gz’ saved [25369685/25369685]

cp: cannot stat 'nu': No such file or directory
Error: Process completed with exit code 1.

@gaborbernat
Copy link
Contributor

This still seems to be failling on macos 🤔

@vfazio
Copy link
Contributor Author

vfazio commented Nov 11, 2022

This still seems to be failling on macos 🤔

I'll try to setup a VM with MacOS 12 and see if i can recreate it... it's just odd that it fails on 3.8 but not 3.9

@gaborbernat
Copy link
Contributor

brew macos is weird like that 🤦 they change the layout every version, e.g. 3.9 and 3.10 does not even pass our test suite...

@vfazio
Copy link
Contributor Author

vfazio commented Nov 11, 2022

well, i can still try to setup a VM to debug this. trying to debug it through a CI pipeline seems insane. If you have hardware and can help debug that'd be great. I probably won't be able to get around to this for the next few days however. My guess is it's a symlink issue and that maybe i need to realpath something, but i'd like to understand it before making changes since it's working on all of the other platforms.

or if there's a way for me to mark this as skip, i can do that too if that's your preference.

PEP 405 says of the "home" key:

"If a home key is found, this signifies that the Python binary belongs
to a virtual environment, and the value of the home key is the directory
containing the Python executable used to create this virtual
environment."

And:

"In this case, prefix-finding continues as normal using the value of the
home key as the effective Python binary location, which finds the prefix
of the base installation."

Previously, "home" was being set to `interpreter.system_exec_prefix`
which does not abide by the PEP specification.

In Python 3.11, the "home" directory is used to determine the value of
`sys._base_executable`, so if the path specified is incorrect, the
path + interpreter returned will be invalid. This can cause headaches
later when trying to probe info via the discovery module.

Now, set this to the parent directory of `interpreter.system_executable`.

Signed-off-by: Vincent Fazio <[email protected]>
@vfazio vfazio force-pushed the vfazio-set-home-to-bin-path branch from 98414d8 to 812b26f Compare November 12, 2022 05:06
@vfazio
Copy link
Contributor Author

vfazio commented Nov 12, 2022

As suspected:

vfazio@Vincents-Mac tmp.TKRRHjTO % readlink  /usr/local/opt/[email protected] 
../Cellar/[email protected]/3.8.15

So certain code paths will end up using "resolved" paths and others won't. So instead of doing an explicit path comparison, i'm just going to check that python exists in the path pointed to by the "home" key, regardless of if venv was the creator or if virtualenv was.

@vfazio
Copy link
Contributor Author

vfazio commented Nov 12, 2022

There is no reason why that ubuntu 3.11 pipeline should be failing... it seems to imply that apt-get install wget curl -y is returning some non-zero error code or that hitting the nushell release API endpoint is failing, both of which seems weird

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.

3.11 support
2 participants