Skip to content

bpo-34977: Use venv redirector instead of original python.exe on Windows #11029

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

Merged
merged 4 commits into from
Dec 10, 2018
Merged
Show file tree
Hide file tree
Changes from 3 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
1 change: 1 addition & 0 deletions Lib/test/test_venv.py
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ def test_isolation(self):
self.assertIn('include-system-site-packages = %s\n' % s, data)

@unittest.skipUnless(can_symlink(), 'Needs symlinks')
@unittest.skipIf(os.name == 'nt', 'Symlinks are never used on Windows')
def test_symlinking(self):
"""
Test symlinking works as expected
Expand Down
49 changes: 19 additions & 30 deletions Lib/venv/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,11 @@ def create(self, env_dir):
self.system_site_packages = False
self.create_configuration(context)
self.setup_python(context)
if not self.upgrade:
self.setup_scripts(context)
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

"setup_python" on Windows no longer creates a "python.exe" file - those come from the scripts directory. So we need to set up scripts before installing pip, or else we can't install pip.

These were previously totally orthogonal steps on all platforms, and now there is a dependency that requires a certain ordering.

if self.with_pip:
self._setup_pip(context)
if not self.upgrade:
self.setup_scripts(context)
self.post_setup(context)
if true_system_site_packages:
# We had set it to False before, now
Expand Down Expand Up @@ -158,14 +159,6 @@ def create_configuration(self, context):
f.write('include-system-site-packages = %s\n' % incl)
f.write('version = %d.%d.%d\n' % sys.version_info[:3])

if os.name == 'nt':
def include_binary(self, f):
if f.endswith(('.pyd', '.dll')):
result = True
else:
result = f.startswith('python') and f.endswith('.exe')
return result

def symlink_or_copy(self, src, dst, relative_symlinks_ok=False):
"""
Try symlinking a file, and if that fails, fall back to copying.
Expand Down Expand Up @@ -195,9 +188,9 @@ def setup_python(self, context):
binpath = context.bin_path
path = context.env_exe
copier = self.symlink_or_copy
copier(context.executable, path)
dirname = context.python_dir
if os.name != 'nt':
copier(context.executable, path)
if not os.path.islink(path):
os.chmod(path, 0o755)
for suffix in ('python', 'python3'):
Expand All @@ -209,26 +202,22 @@ def setup_python(self, context):
if not os.path.islink(path):
os.chmod(path, 0o755)
else:
# See bpo-34011. When using a proper install, we should only need to
# copy the top-level of DLLs.
include = self.include_binary
files = [f for f in os.listdir(dirname) if include(f)]
for f in files:
src = os.path.join(dirname, f)
dst = os.path.join(binpath, f)
if dst != context.env_exe: # already done, above
copier(src, dst)

# When creating from a build directory, we continue to copy all files.
# For normal cases, the venvlauncher will be copied from
# our scripts folder. For builds, we need to copy it
# manually.
if sysconfig.is_python_build(True):
subdir = 'DLLs'
dirname = os.path.join(dirname, subdir)
if os.path.isdir(dirname):
files = [f for f in os.listdir(dirname) if include(f)]
for f in files:
src = os.path.join(dirname, f)
dst = os.path.join(binpath, f)
copier(src, dst)
suffix = '.exe'
if context.python_exe.lower().endswith('_d.exe'):
suffix = '_d.exe'

src = os.path.join(dirname, "venvlauncher" + suffix)
dst = os.path.join(binpath, context.python_exe)
copier(src, dst)

src = os.path.join(dirname, "venvwlauncher" + suffix)
dst = os.path.join(binpath, "pythonw" + suffix)
copier(src, dst)

# copy init.tcl over
for root, dirs, files in os.walk(context.python_dir):
if 'init.tcl' in files:
Expand Down Expand Up @@ -326,7 +315,7 @@ def install_scripts(self, context, path):
dstfile = os.path.join(dstdir, f)
with open(srcfile, 'rb') as f:
data = f.read()
if not srcfile.endswith('.exe'):
if not srcfile.endswith(('.exe', '.pdb')):
try:
data = data.decode('utf-8')
data = self.replace_variables(data, context)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
venv on Windows will now use a python.exe redirector rather than copying the
actual binaries from the base environment.
Copy link
Member

Choose a reason for hiding this comment

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

You may add "As a result, the base Python install can be safely upgraded without breaking all venvs, and venv creation is significantly faster." (or explain it differently)

Moreover, this change might have side effect, so I suggest to document it in https://docs.python.org/dev/whatsnew/3.7.html#notable-changes-in-python-3-7-1 (add a new Python 3.7.2 section).

I'm not sure if it's worth it to add a ".. versionchanged:: 3.7.2" to document the change in https://docs.python.org/dev/library/venv.html as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically it's not backwards compatible, but upgrading Python without recreating all your venvs wasn't backwards-compatible either. So it remains broken for that scenario, which would be broken without this change. But creating a venv with 3.7.2 and upgrading to 3.7.3 will be fine :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm adding the docs - I found a spot in the venv docs where it's worth noting the change (specifically that "setup_python" doesn't copy the binaries/DLLs anymore).

8 changes: 7 additions & 1 deletion PC/getpathp.c
Original file line number Diff line number Diff line change
Expand Up @@ -536,10 +536,16 @@ static _PyInitError
get_program_full_path(const _PyCoreConfig *core_config,
PyCalculatePath *calculate, _PyPathConfig *config)
{
const wchar_t *pyvenv_launcher;
wchar_t program_full_path[MAXPATHLEN+1];
memset(program_full_path, 0, sizeof(program_full_path));

if (!GetModuleFileNameW(NULL, program_full_path, MAXPATHLEN)) {
/* The launcher may need to force the executable path to a
* different environment, so override it here. */
pyvenv_launcher = _wgetenv(L"__PYVENV_LAUNCHER__");
if (pyvenv_launcher && pyvenv_launcher[0]) {
wcscpy_s(program_full_path, MAXPATHLEN+1, pyvenv_launcher);
} else if (!GetModuleFileNameW(NULL, program_full_path, MAXPATHLEN)) {
/* GetModuleFileName should never fail when passed NULL */
return _Py_INIT_ERR("Cannot determine program path");
}
Expand Down
Loading