Refactor virtualenv bin/ / Scripts/ path resolution using sysconfig mechanism

[ByteB4rb1e]

This pull request refactors the logic for determining the venv's scripts/. or bin/ directory location by leveraging sysconfig’s built-in mechanism for retrieving the complete global install directory path of scripts and retrieving its basename. This removes any platform-dependent logic and does not require an additional dependency.

Changes:

feat: Introduced a new approach for venv scripts path lookup, eliminating reliance on platform identifiers and improving compatibility with POSIX-like environments such as Cygwin and MinGW.

refactor: Updated both project-wide and routine-specific implementations to use the virtualenv mechanism for dynamic script location lookup, streamlining execution.

These changes mitigate execution inconsistencies in non-native Windows environments and improve portability across platforms.

Fixes: #5978

[Copilot]

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

pipenv/utils/virtualenv.py:16

• Consider adding tests for the new virtualenv_scripts_dir function to ensure that script path resolution works correctly across different environments.

PYINFO_CURRENT = PythonInfo.current_system()

[matteius]

@ByteB4rb1e please pip install pre-commit and then run pre-commit install in the pipenv dir, and check the lint changes for this PR so the tests can run.

[ByteB4rb1e]

Oh, that's unfortunate. My bad, I didn't check the requirements and just assumed virtualenv to already be a dependency. I don't want to introduce a new dependency for these small changes. So I'm reconsidering my approach, instead focusing on the built-in venv, which was introduced in v3.3, so would pass the Vendoring test.

I've tested venv in CPython 3.9 locally and am running into a similar issue as pipenv currently does, only that the wrong directories are being created. The issue was seemingly fixed in CPython v3.11, interestingly pipenv Issue #5109 (closed) references the linked issue report (CPython #89576). I don't yet know what the implications of that are.

A quick workaround that comes to mind would be:

>>> import sysconfig
>>> from pathlib import Path
>>> Path(sysconfig.get_path('scripts')).name
'bin'

So I will see how the internals of that work and compare it to how venv::__main__ does the bootstrapping.

EDIT: The "workaround" might actually be the most pragmatic and sensible approach as the underlying mechanism is not a public API and I can see a few pitfalls mocking it.

@matteius, @oz123, is this approach fine with you?

[matteius]

@ByteB4rb1e Would be great if we can get a new test to cover this. Also everything else looked good--please add a news fragment too when you get a chance.

[ByteB4rb1e]

@matteius, will do! It'll probably have to wait for a couple of days though as I have to take a brief detour. I'm currently setting up another proposal for refactoring the test suite to work locally without the run-tests.sh wrapper script as I currently need a VM and also had to adapt the script to work. Doing that I ran into the same path issues with the pre-commit package, so I will file an issue report and create a PR there first.

[ByteB4rb1e]

0017-sysconfig-treat-MINGW-builds-as-POSIX-builds.patch is the MSYS2 specific patch of CPython's sysconfig I aim to have test coverage for, as this is the strongest deviation from conventional CPython distributions. The patch is indeed a little quirky, but doesn't provoke any unintended behavior for what we're achieving with this refactoring, as we're utilizing a higher-level facility. Actually quite the opposite: The refactoring ensures, that the logic stays intact, even for other unconventional CPython builds that may not exist yet. Just stumbled upon this to figure out what's best to mock...

Sadly though, there was a patch introduced which requires special handling in the test case. I've filed a bug report and am hopeful that this can be resolved soon!

I will now add parameterization for other platform combinations, then lint, then add the news fragment, as requested by @matteius.

[matteius]

looks like the tests are failing for a variety of reasons depending on platform and version.

[ByteB4rb1e]

Ok, wow... I didn't expect it to be this construed. What I'm taking from this is, while the sysconfig::get_path interface is constant, each distributor has different ways of patching things further down along the call tree. I've tested it locally with the embedded python distribution for Windows, MSYS2 MinGW distribution and Ubuntu distribution and they all passed.

It would be possible to define separate test cases for each platform and just ignore non-applicable ones, but that doesn't sit quite right with me as this wouldn't address testing any future deviations by distributors.

EDIT: Ok, half the test failures in regards to _winapi are plausible to me and can be resolved quickly, which actually do require the separation of test cases between Windows and POSIX. The macOS specific failures though...

I think I will push through with this, trying to analyze the deviations and make the test case work as I intended. It will take some time though as I first have to identify the distributions in use by the CI test and come up with a faster testing setup for debugging.

Bear with me, please. I'm hoping to get it done before the next release.

[matteius]

which actually do require the separation of test cases

We have this pattern with pytest markers, so you can specific which OS a test should run on. That being said, as you pointed out, I've run into it too--the vendor implementations of sysconfig are always different, so relying on private _ methods seems a bit risky.

[ByteB4rb1e]

You're right. I got carried away. I'm just trying to grasp the history of how this issue arose in the first place. But this is the wrong circumstance to do that. Mocking private API functions is a testament to that. Pardon...
There are now two separate test cases, authored like all the others, with the exception that I've added a secondary condition for skipping the test, so that they properly execute within MinGW CPython as well.

I'll create the news fragment now and then make sure the changes actually fixed the original problem.

[ByteB4rb1e]

I've tested all interactive commands manually, they're working fine. In addition, fixed another fault during the uninstall routine, and fixed the faulty path in the Windows test case. All is seemingly working now. Don't know though why ruff didn't trigger locally...

$> cat /proc/version
MSYS_NT-10.0-26100 version 3.5.7-2644508f.x86_64 (runneradmin@fv-az1853-986) (gcc version 13.3.0 (GCC) ) 2025-01-30 09:08 UTC
$> which python3 && python3 -V && pacman -Q mingw-w64-x86_64-python
/mingw64/bin/python3
Python 3.12.9
mingw-w64-x86_64-python 3.12.9-1
$> test -d .venv || echo $?
1
$> python3 -m venv .venv
$> .venv/bin/pip install git+https://github.com/ByteB4rb1e/pipenv.git@bugfix/5978 2>&1 | tee pip-install-pipenv.txt
...
Successfully installed certifi-2025.1.31 distlib-0.3.9 filelock-3.18.0 packaging-24.2 pipenv-2024.4.1 platformdirs-4.3.7 setuptools-78.1.0 virtualenv-20.30.0

[notice] A new release of pip is available: 24.3.1 -> 25.0.1
[notice] To update, run: C:/Users/me/Repositories/local/pipenv-debug/.venv/bin/python3.exe -m pip install --upgrade pip
$> .venv/bin/pipenv install cowsay && .venv/bin/cowsay -t works
To activate this project's virtualenv, run pipenv shell.
Alternatively, run a command inside the virtualenv with pipenv run.
Installing cowsay...
Installation Succeeded
To activate this project's virtualenv, run pipenv shell.
Alternatively, run a command inside the virtualenv with pipenv run.
Installing dependencies from Pipfile.lock (b54bd8)...
All dependencies are now up-to-date!
Upgrading cowsay in  dependencies.
Building requirements...
Resolving dependencies...
Success!
Building requirements...
Resolving dependencies...
Success!
To activate this project's virtualenv, run pipenv shell.
Alternatively, run a command inside the virtualenv with pipenv run.
Installing dependencies from Pipfile.lock (c93707)...
All dependencies are now up-to-date!
Installing dependencies from Pipfile.lock (c93707)...
  _____
| works |
  =====
     \
      \
        ^__^
        (oo)\_______
        (__)\       )\/\
            ||----w |
            ||     ||
$> .venv/bin/pipenv uninstall cowsay && .venv/bin/cowsay || echo 'removed'
Removed cowsay from Pipfile.
Uninstalling cowsay...
Found existing installation: cowsay 6.1
Uninstalling cowsay-6.1:
  Successfully uninstalled cowsay-6.1

-bash: .venv/bin/cowsay: No such file or directory
removed

pip-install-pipenv.txt

[Copilot]

Pull Request Overview

This PR refactors virtualenv path resolution using sysconfig to dynamically determine the scripts directory (either bin/ or Scripts/) and improves cross-platform compatibility. Key changes include introducing the virtualenv_scripts_dir function, updating tests to cover both Windows and POSIX environments, and refactoring usages in routines and project properties.

Reviewed Changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/unit/test_utils.py	Added new test cases for asserting expected platform-based path results.
pipenv/utils/virtualenv.py	Introduced virtualenv_scripts_dir to determine the scripts directory.
pipenv/routines/shell.py	Updated shell invocation to use virtualenv_scripts_dir for constructing paths.
pipenv/project.py	Refactored virtualenv existence check and Finder path resolution.
pipenv/environment.py	Updated python property to derive path using virtualenv_scripts_dir.

Files not reviewed (1)

• news/6737.bugfix.rst: Language not supported

Comments suppressed due to low confidence (1)

pipenv/utils/virtualenv.py:17

• [nitpick] Consider renaming the parameter 'b' to 'base_path' for improved readability and clarity.

def virtualenv_scripts_dir(b):

[matteius]

Thanks for your hard work on this change!