Fix isolated environment scripts path on Debian

[dnicolodi]

The scripts path was looked up passing explicitly the scheme to be used using "nt" on Windows and "posix_prefix" everywhere else. However, when the isolated build environment is created, packages are installed using the default scheme for the platform. On most platforms this works because normally "nt" and "posix_prefix" are the default schemes.

However, Debian customizes sysconfig to use a "posix_local" scheme by default and under this scheme the scripts path does not match the one of the "posix_prefix" scheme. This results in scripts installed as part of the build dependencies not to be found during the build, as reported here mesonbuild/meson-python#109 and here https://bugs.debian.org/1019293.

The problem can be solved omitting to specify a scheme when looking up the scripts path. To future proof the path lookup, use the "venv" scheme if available as done in #11598. For uniformity use similar functions as used to lookup the library paths.

[uranusjr]

You also need a corresponding function for distutils; it’d be enough to simply move the old logic there since nobody on old systems are complaining about that being wrong.

[dnicolodi]

You also need a corresponding function for distutils

Unless there is some magic trick I don't see, the existing code uses sysconfig unconditionally. Thus I thought adding a distutils fallback was not necessary.

[uranusjr]

You’re right, now I’m wondering why no-one reported any issues about it… but I’m not going to argue with success.

Since this logic is the same across both backends, can you simply move the logic to locations/__init__.py and get rid of the unnecessary call to _sysconfig?

[dnicolodi]

I think there are not many packages that depend on scripts installed as build dependencies out there. This came up in packages using meson-python as pep517 backend. meson-python installs meson and ninja if required as build dependencies. On which platforms is pip still using installation paths from distutils?

I decided to keep the implementation next to the implementation of get_isolated_environment_lib_paths() to make it more evident that the two should be most likely kept in sync. I don't think the overhead of one more function call matters at all here. However, I don't have a strong opinion about this, and I can move the implementation, if you like it more that way.

[uranusjr]

On which platforms is pip still using installation paths from distutils?

See docstrings on functions and constants near the top of the __init__.py.

I decided to keep the implementation next to the implementation of get_isolated_environment_lib_paths() to make it more evident that the two should be most likely kept in sync

If the goal is to keep things in sync, we should probably merge the two functions. Something like get_isolated_environment_scheme() that returns a scheme object (like get_scheme() in the same module) so the logic and results always stay together.

[dnicolodi]

See docstrings on functions and constants near the top of the __init__.py

Which __init__.py?

I reworked the implementation slightly. Let me know if you like this better.

[dnicolodi]

ping?

[uranusjr]

You didn’t answer my question earlier. Is it viable to merge get_isolated_environment_bin_path and get_isolated_environment_lib_paths into one function?

[dnicolodi]

Have you seen the changes in the last push? The implementation has been merged into a single function. I don't think that restructuring the code any further makes it easier to read or understand. If you think differently, please let me know and I'll restructure it. However, this sort of cleanup IMHO does not belong into a PR that is fixing a very real and very serious bug.

[uranusjr]

But why do you keep the two functions, instead of simply exposing get_isolated_environment_paths in locations/__init__.py and calling it from other modules? The part I really don’t understand is the insistence on the extra function call layer, either in the earlier or the current implementation.

[dnicolodi]

Please note that the extra function call layer was already there for get_isolated_environment_lib_paths(). I simply keep the same code structure for get_isolated_environment_bin_path(). I don't understand why there needs to be a locations._sysconfig module either, but I just followed what was done before. I can change that, if you like, but maybe a more cohesive refactoring is necessary, and it would be outside the scope of this PR.

[dnicolodi]

Note that get_isolated_environment_lib_paths() has two implementations, one in locations._sysconfig and one in locations._distutils that's the reason for the indirection. get_isolated_environment_bin_path() does not have a distutils implementation, thus we get what seems to be a pointless redirection to keep the symmetry between the two functions.

[dnicolodi]

I don't see a way to easily simplify this further without bigger reorganization of the code or without loosing the obvious symmetry between get_isolated_environment_lib_paths() and get_isolated_environment_bin_path().

[pradyunsg]

This is https://discuss.python.org/t/linux-distro-patches-to-sysconfig-are-changing-pip-install-prefix-outside-virtual-environments/18240 and (personally) I'd much rather we fix this either via the redistributors unbreaking their patches or #10720.

[dnicolodi]

While I also think #10720 is probably the right solution in the long run (that PR seems stalled and in the meanwhile we need a solution that works) this PR fixes an instance of pip using the sysconfig API incorrectly. The fact that using the API correctly fixes pip on Debian can be seen as incidental (and I don't see what is wrong with the way in which Debian customizes the installation path scheme, in this case).

[pradyunsg]

Bah, I meant to mention #11619.

That said, I do agree with your point that improving the custom isolation to be less broken is likely still a good idea.

[dnicolodi]

I see that #11619 proposes to add the venv-based isolation as an option and not as the only implementation. Fixing the current implementation is thus still required. Is there any remaining issue that needs to be addresses in this PR before merging?

It would be great to have a 22.3.2 release with this fix and #11598 to fix pip on Homebrew and on Debian.

[pfmoore]

Rge release 22.3 cycle is now complete, so any changes would now have to wait for 23.0.

[dnicolodi]

Do we really need keep pip broken on Debian till February?

[pfmoore]

This is no different than any other bug fix that gets implemented between releases.

[dnicolodi]

Indeed. But the issue is unrelated to this PR. If you revert to the commit before the merge of this PR you get the same error. I'm looking at this issue.

[stefanor]

Yeah, looks like the regression comes from 19e8022 (#11598)

[dnicolodi]

I suspected it. I'm trying to understand why that patch breaks pip on Debian.

[stefanor]

Because packages were installed into overlay/local/lib/python3.11/dist-packages (with the Debian posix_local scheme), but sitecustomize.py has overlay/lib/python3.11/site-packages, where it used to have overlay/local/...

[dnicolodi]

That much was obvious. Understanding why the paths are resolved like that is the real question.

[stefanor]

Previously get_prefixed_libs() didn't specify a scheme, so it used to use the default scheme.

Pip's isolated environment isn't a virtualenv (yet?), so it doesn't necessarily use the venv layout.

[stefanor]

Confirmed that patching out the venv branch in #11598 will give a pip that resolves the Debian bug here.

[dnicolodi]

Sure, but it then pip breaks on Homebrew. I wonder if there is an incarnation of this that works everywhere or the customisations Debian and Homebrew do to sysconfig are incompatible.

[stefanor]

I don't know exactly what behaviour they're referring to in:
https://github.com/Homebrew/homebrew-core/blob/5c70586729909455ef0ff01f835e0d8821a67e8d/Formula/python%403.11.rb#L70

But I imagine the right thing to do is to have pip select the posix_prefix scheme when a prefix is specified (And I'm sure there's a lot more detail here, that I'm handwaving away). Both Debian and Homebrew are relying on schemes to imply prefixes, which is problematic.

I'm not speaking authoritatively for Debian here: I kind of hope that the replacement of setup.py files by PEP517 installers means we can get away from some of the sysconfig scheme customizations that we do. If all package installation is centralized in pip (and a few similar tools), then those can get patches to support PEP-668, and we won't need things like the posix_local sysconfig scheme.

[stefanor]

I think on homebrew the venv scheme just happens to match what pip install --prefix /tmp/foo is doing. But on Debian it isn't.

[pradyunsg]

@stefanor Has Debian added PEP-668's EXTERNALLY-MANAGED file on testing, to Python?

[stefanor]

That's a good point. Now's the time to do it, before we freeze!

[dnicolodi]

@stefanor Can you please give #11740 a try? It works in my tests.

[stefanor]

@stefanor Can you please give #11740 a try? It works in my tests.

Yeah works for me.

@stefanor Has Debian added PEP-668's EXTERNALLY-MANAGED file on testing, to Python?

Proposed that here. https://salsa.debian.org/cpython-team/python3/-/merge_requests/28
Of course that would make the specific bug that brought me here moot, because pip won't let you install packages (into --user or the system), on an EXTERNALLY-MANAGED python. I'm guessing we'll see people recommending to delete that file, soon :)

[dnicolodi]

@stefanor Thank you for testing.

[dnicolodi]

Of course that would make the specific bug that brought me here moot, because pip won't let you install packages (into --user or the system), on an EXTERNALLY-MANAGED python. I'm guessing we'll see people recommending to delete that file, soon :)

You could not even install a pip that fixes this issue 😃