Skip to content
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

Detect whether we are running in a Conda environment and adjust get_include() #1877

Merged
merged 2 commits into from Aug 19, 2019

Conversation

sdebionne
Copy link
Contributor

Try to address this issue with the conda-forge feedstock.

For the records, Conda packages install headers in prefix/include on Linux and prefix\Library\include on Windows.

@@ -9,10 +9,18 @@ def get_include(user=False):
# Are we running in a virtual environment?
virtualenv = hasattr(sys, 'real_prefix') or \
sys.prefix != getattr(sys, "base_prefix", sys.prefix)

Copy link
Collaborator

Choose a reason for hiding this comment

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

the PEP8 check fails since this empty line contains whitespaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@wjakob
Copy link
Member

wjakob commented Aug 18, 2019

Hi @sdebionne, @ax3l,

this looks reasonable -- do you want me to merge it, or should I wait until it is proven to work in an actual CI build (as suggested by @ax3l here: conda-forge/pybind11-feedstock#32 (comment))

Thanks,
Wenzel

@ax3l
Copy link
Collaborator

ax3l commented Aug 19, 2019

I added the current PR's changeset to the conda-forge feedstock now.
Yes, let's wait for it to pass before merging this here as well.

@@ -10,9 +10,17 @@ def get_include(user=False):
virtualenv = hasattr(sys, 'real_prefix') or \
sys.prefix != getattr(sys, "base_prefix", sys.prefix)

# Are we running in a conda environment?
conda = os.path.exists(os.path.join(sys.prefix, 'conda-meta'))

if virtualenv:
Copy link
Collaborator

@ax3l ax3l Aug 19, 2019

Choose a reason for hiding this comment

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

@sdebionne maybe that's a stupid question but let me ask it anyway:
can one be inside conda and therein as well within a virtualenv? Not that I would do that... conda has its one environments. But if so, how to handle this gracefully (or does this already work)? Or shall we just not support such a constellation?
(The opposite case is not possible, I guess.)

@ax3l
Copy link
Collaborator

ax3l commented Aug 19, 2019

As the tests turned out, the get_include() paths were already broken under Linux as well when taken from a conda install. Good job @sdebionne fixing them as well! Thanks again!

@wjakob ready for merge :)

@wjakob wjakob merged commit 87fa6a4 into pybind:master Aug 19, 2019
@wjakob
Copy link
Member

wjakob commented Aug 19, 2019

Okay, merged then.

@wjakob
Copy link
Member

wjakob commented Aug 19, 2019

btw: I assume that you'll want this PR to be included in the next patch release 2.3.1, right?

@ax3l
Copy link
Collaborator

ax3l commented Aug 19, 2019

Yes, please :)

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.

None yet

3 participants