Skip to content

Detect pybind11 header path without depending on pip internals (fixes #1174) #1190

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 1 commit into from
Apr 16, 2018

Conversation

wjakob
Copy link
Member

@wjakob wjakob commented Nov 16, 2017

The upcoming version of pip will disable all access to internals including the pip.locations.distutils_scheme('pybind11')['headers'] function that we currently rely on. This commit reimplements a basic subset of this function within pybind11. I'm also open to other approaches if anybody has a better idea :)

We should probably backport a similar patch to previous releases.

@marscher
Copy link

marscher commented Dec 3, 2017

you could also use pkg_resources (setuptools), but I'd guess that distutils is even more portable in terms of availability.

@anntzer
Copy link
Contributor

anntzer commented Dec 14, 2017

As get_include is being rewritten, perhaps this is also an opportunity to get rid of the user=bool switch? AFAICT, the currently recommended setuptools scheme is to pass both get_include(user=True) and get_include(user=False) to include_dirs. This means that if two different versions of pybind11 are installed system-wide and at user-level, the first listed will win; so user=True should probably be listed first, to follow the normal Python import path. But this then means that the PYTHONNOUSERSITE environment variable would not be respected in that case.

Instead, I would suggest making get_include() check whether pybind11.__file__ is a child of site.USER_SITE; if it is, then behave as get_include(user=True) did, if not, behave as get_include(user=False) did. This may optionally be combined with deprecation of the user= kwarg (but that's not necessary either).

@wjakob
Copy link
Member Author

wjakob commented Apr 8, 2018

I'm against changing the user=true/false as part of this PR. This would break the calling conventions of the get_include function, so it's something we could consider in the next major revision.

@wjakob
Copy link
Member Author

wjakob commented Apr 8, 2018

Any other objections to getting this merged soon?

@jagerman
Copy link
Member

jagerman commented Apr 8, 2018

I'm not a huge fan of implementing this logic inside of pybind; it's a maintenance burden that has very little to do with pybind's purposes.

One possible alternative is that we simply change import pip.locations to import pip.internals_.locations, while petitioning upstream to expose this more publicly. Despite the underscore, pip upstream says we've been using an internal API all along already, so continuing to use an internal API doesn't seem like that big a deal (rather than needing to test and confirm that we're covering various edge cases in the correct way).

@jagerman jagerman modified the milestones: v2.2.3, v2.3 Apr 8, 2018
@jagerman
Copy link
Member

jagerman commented Apr 8, 2018

That said, don't take that as opposition to the PR: I'm not opposed to merging it if you firmly believe this is the better approach. (On the other hand, if @SylvainCorlay manages to convince pip to change their mind about hiding this—a long shot, I think—then I'd definitely prefer to stick with the non-internal, officially sanctioned approach).

@jagerman
Copy link
Member

jagerman commented Apr 8, 2018

The upstream vibe definitely seems to be "go away, this is our private implementation that we don't want to share because we only want pip to be used as a command line tool." So I suppose I'm reluctantly moved towards saying go ahead with this PR. (On the plus side, it gets rid of the pip dependency).

@anntzer
Copy link
Contributor

anntzer commented Apr 8, 2018

I'm against changing the user=true/false as part of this PR. This would break the calling conventions of the get_include function, so it's something we could consider in the next major revision.

Note that user=true is returning an completely incorrect value when PYTHONNOUSERSITE is set. If you don't want to break the interface, one option would be for user=False (or rather, for the default) to return the "correct" value (ie from the user directory if pybind was itself imported from the user directory), and for user=True to always return an invalid path (e.g. os.devnull).

@SylvainCorlay
Copy link
Member

That said, don't take that as opposition to the PR: I'm not opposed to merging it if you firmly believe this is the better approach. (On the other hand, if @SylvainCorlay manages to convince pip to change their mind about hiding this—a long shot, I think—then I'd definitely prefer to stick with the non-internal, officially sanctioned approach).

The upstream vibe definitely seems to be "go away, this is our private implementation that we don't want to share because we only want pip to be used as a command line tool." So I suppose I'm reluctantly moved towards saying go ahead with this PR. (On the plus side, it gets rid of the pip dependency).

I think that it makes sense to merge this. The upstream breakage remains very annoying as a lot of client code to pybind11 uses the distutils_scheme pip API.

In any case, the upstream team appears to be quite bitter and it seems unlikely that they will go with a deprecation approach.

@SylvainCorlay
Copy link
Member

@wjakob I have tested your PR with a regular python install and the exotic homebrew distutils scheme and it works well.

@wjakob
Copy link
Member Author

wjakob commented Apr 16, 2018

I'll go ahead and merge this now to fix breakage with PIP 10.x. Sanitizing the behavior of the user= parameter in a future PR would be nice, but let's do this separately (potentially in a new major version). This PR is more conservative and retains the old behavior.

@wjakob wjakob merged commit 060936f into pybind:master Apr 16, 2018
@wjakob wjakob deleted the pip-fixes branch April 16, 2018 08:27
@anntzer
Copy link
Contributor

anntzer commented Apr 16, 2018

Actually even simpler is to make both user=True and =False return the same correct path...

Suggested "correct" implementation:

_SENTINEL = object()


def get_include(user=_SENTINEL):
    import os
    import re
    import subprocess
    import sys
    import warnings

    if user is not _SENTINEL:
        warnings.warn("Passing 'user' to get_include is deprecated.",
                      DeprecationWarning)
    lines = subprocess.check_output(
        [sys.executable, "-mpip", "show", "-f", "pybind11"],
        universal_newlines=True).splitlines()
    loc_match, = filter(
        None, (re.match(r"\ALocation: (.*)\Z", line) for line in lines))
    header_match, = filter(
        None, (re.match(r"\A  (.*\bpybind11.h)\Z", line) for line in lines))
    loc = loc_match.group(1)
    header = header_match.group(1)
    return os.path.dirname(os.path.dirname(os.path.join(loc, header)))

If you don't like running pip in a subprocess we can also just copy-paste the relevant parts of the implementation of pip show, I don't particularly care either way.

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.

5 participants