Fix uninstall of editable on py27 in debian virtualenv

[sbidoul]

Fixes #5193

• The first commit is a workaround for what appears to be a virtualenv/debian bug with python 2.7
• With that workaround in place, the second commit simplifies detection of editable installs by reusing the egg-link detection method

[pradyunsg]

What's the exact issue here and why is this PR the appropriate fix? Wouldn't fixing the bug in debian be a better approach here?

I'm super skeptical of fixing/working around bugs introduced downstream, since that means pip maintainers would have to do a lot more work than they already do. :)

---

Anyway, assuming we go this route, I'd suggest creating a variable can_not_depend_on_purelib (or equivalent) and using that:

# This is needed as Debian-patched virtualenv and PyPy
# have a bug in sysconfig module.
# https://github.com/pypa/pip/issues/5193
# https://bitbucket.org/pypy/pypy/issues/2506/sysconfig-returns-incorrect-paths
can_not_depend_on_purelib = (
    ...
)
if can_not_depend_on_purelib:
    site_packages = distutils_sysconfig.get_python_lib()  # type: Optional[str]
else:
    site_packages = sysconfig.get_path("purelib")  # type: Optional[str]

Further, if there's a way to detect being run on Debian, we should only use the workaround in that case.

Lastly, this should probably have tests?

[pfmoore]

+1 on getting Debian to fix their problem. We do enough already to deal with issues caused by Debian patching Python and pip, I don't think we should spend limited pip developer resource on supporting/maintaining workarounds like this.

If people don't start pushing this sort of problem back on Debian, what's the incentive for Debian to get things right?

Edit: Particularly as it's 2.7-only I'm against including this workaround.

[sbidoul]

@pradyunsg thanks for reviewing this.

What's the exact issue

I think it is described extensively in #5193.

Wouldn't fixing the bug in debian be a better approach here?

Probably, but I think noone could pinpoint the root cause of the issue yet.
So since it could be considered a regression in pip 10, I figured it would be acceptable to stay on distutils_sysconfig.get_python_lib() on some platforms.

In that spirit your suggestion of having can_not_depend_on_purelib looks better indeed.

Regarding tests, I'm temporarily pushing a version with only the second commit (without the python 2.7 workaround) to see if that is sufficient to reveal the problem.

[pfmoore]

Probably, but I think noone could pinpoint the root cause of the issue yet.

Naively, isn't the issue that sysconfig.get_path("purelib") isn't returning the right value in a virtualenv? That's what "cannot depend on purelib" says to me...

Moving away from using an undocumented distutils function to a documented, standard stdlib function seems entirely right to me.

[sbidoul]

isn't the issue that sysconfig.get_path("purelib") isn't returning the right value in a virtualenv

It is, on python 2.7, in some virtualenvs (I confirmed the problem is present on ubuntu 19, but not on centos 7, for instance).

Moving away from using an undocumented distutils function to a documented, standard stdlib function seems entirely right to me

I agree. Yet the issue is a regression in pip >= 10.

I personally have little hope that an upstream fix, if possible at all, would percolate to all places where it is needed.

In truth, the original issue is not a very big one. The reason I made this PR is that it can have nastier consequences. For instance if you apply the simplification in 9980841 without the sysconfig workaround, it breaks pip freeze of editable installs in the same py27 debian virtualenvs.

Unfortunately it does not appear in CI. On my Ubuntu 19 machine I can see it though, by doing

$ tox -e py27 -- -k freeze_editable
FAIL
$ tox -e py37 -- -k freeze_editable
OK

So, while I totally understand why you want to keep exotic workarounds out of the pip codebase, IMHO, if you decide to close this PR and related issue as won't fix, there is a big risk it will manifest itself again in a worse way later.

I reworked the workaround as per @pradyunsg suggestion.

Edit: and the fix is not that exotic, since it's there for pypy too

[pradyunsg]

link about expectations in OSS

I empathize that this is an issue for end-users. However, the fact that this only occurs on Debian systems likely points to some downstream packager introduced quirk, which should be fixed on their end. In essence, upstream shouldn't do bug-fixes for when downstream introduces bugs.

Yes, it sucks that we have to let users have broken systems. We already have a lot of users face issues because downstream packagers introduces/patches functionality in the Python/pip/virtualenv that they distribute. If we start papering over these issues, then we'll only set the expectation that pip maintainers would fix issues caused by downstream's patches, causing more work for us. It's the responsibility of downstream packagers to fix those issues.

While this fix is not exotic, PyPy was updated to remove the need for this workaround. I'd prefer that Debian does the same.

On a related note, now that it's been brought to my attention, I'm actually inclined to actually remove (or at least constraint when we use) that workaround on PyPy, based on usage numbers of them on PyPI.

---

All that said, thanks for filing this PR, and getting things moving on this issue @sbidoul! Even though we may not accept this PR, your contribution is much appreciated. :)

[pradyunsg]

I reworked the workaround as per @pradyunsg suggestion.

Thank you! ^>^

[sbidoul]

link about expectations in OSS

No worries, I know how OSS works. If I sounded like I did not, sincere apologies and please don't hesitate to point out where I miscommunicated.

I merely wanted to point out why I think the issue might be considered important and risked to become more significant in the future.

some downstream packager introduced quirk

It looks like it's the case indeed. Or perhaps the quirk is, or the fix should go, in virtualenv, I don't know. I would not even have thought about proposing the fix to pip if it was another package that suddenly broke pip. In this case the quirk was present before and it's a pip change that revealed it. So some users may complain that a pip upgrade broke something, even if it's not pip's "fault".

Anyway feel free to close this and the related issue, you are the maintainer, no worries from my side.

Please, pretty please, then don't apply anything like 9980841 until the root cause has been fixed and it's deployment sufficiently widespread.

[pradyunsg]

I'm gonna go ahead and close this, and instead push downstream to fix this on their side (if they haven't already).

[kitterma]

I can fix this in Debian, but it does occur to me that this won't help on Debian stable because the version of virtualenv we have there automatically installs the latest pip from pypi, so a Debian patch would never get used inside a virtualenv.

On unstable, at least the local pip gets used be default, but once a user upgrades pip inside the virtualenv this problem will recur. Since it's python2.7 only and pip versions later than the one we have in our next release won't support python2.7 anymore, it's probably a don't care.

[uranusjr]

For the Debian fix, I feel it’d be easier to patch sysconfig in site.py generated by virtualenv instead, so any version of pip can pick up the correct value automatically. It would also be potentially helpful for tools other than pip that want to use the sysconfig module.

[kitterma]

"Easier" is the patch in hand. I've staged that for the next Debian pip upload. I see your point about that being a more general fix. No promises on if I will have time to investigate that too, but I think it makes sense.

[sbidoul]

@kitterma @uranusjr is correct to say that the root cause is in debian's python patches and cannot be fixed? In which case can we reconsider merging this in pip? It won't cost us much in terms of maintenance and we can remove it in 6 months with the rest of python2-specific stuff, and at least we'll leave a python2 pip in a slightly better state.

[kitterma]

Since this is Debian only, I think for pip's purposes it's fine to leave it to us to patch (and I already have the patch staged in our git for the next upload, so it would actually be more work for me now not to apply it).

We need to look into the virtualenv part of the equation and see what we can do there. If we come up with a virtualenv based solution to the problem, then we might be able to support a fix in the current Debian stable release too (no promises).

[kitterma]

One of the other Debian developers pointed out to me that if this change is pulled into pip then our stable users will get it right away since the stable virtualenv always upgrades to the most recent pip. Maybe that is a better idea.

[uranusjr]

I think it depends on how you define fixability. It is true the root problem is in the Python patch made by Debian.

But as I said in the linked issue, personally I think it is a good idea to change pip’s uninstallation logic to be a bit more permissibe (not necessarily with this patch since it effects more than uninstallation). It’s great the change would also make this Debian issue go away.