Skip to content

Install headers using both headers and package_data #1995

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 7 commits into from
Nov 28, 2019

Conversation

isuruf
Copy link
Contributor

@isuruf isuruf commented Nov 19, 2019

@wjakob
Copy link
Member

wjakob commented Nov 19, 2019

This commit will break virtually every project that recursively includes pybind11 as a git submodule because you are changing the location of the header files. That's really a no-go for me.

@isuruf isuruf force-pushed the headers branch 2 times, most recently from 4193e3f to 929fc3b Compare November 19, 2019 22:23
@isuruf
Copy link
Contributor Author

isuruf commented Nov 19, 2019

@wjakob, it turns out that changing the location is not needed. This is now backwards compatible.

@isuruf isuruf changed the title Install headers using both headers and package_data [WIP] Install headers using both headers and package_data Nov 20, 2019
@isuruf isuruf changed the title [WIP] Install headers using both headers and package_data Install headers using both headers and package_data Nov 20, 2019
@isuruf
Copy link
Contributor Author

isuruf commented Nov 21, 2019

With this PR, pybind11 doesn't have to be installed before installing the downstream package and downstream packages need pybind11 only in setup_requires. See pybind/python_example#47

@wjakob
Copy link
Member

wjakob commented Nov 24, 2019

cc @sdebionne, @ax3l
This PR removes some of the Conda-specific changes made as part of #1877. Would you mind taking a look?

@ax3l
Copy link
Collaborator

ax3l commented Nov 25, 2019

Thanks for the ping. I'll try this with spack as well after lunch.

@ax3l
Copy link
Collaborator

ax3l commented Nov 26, 2019

Hm, I do not think the new implementation helps much if we are in CMake-controlled/PYBIND11_USE_CMAKE installation mode... but otherwise okay for me if it helps with purely setup.py-centric installs.

@isuruf
Copy link
Contributor Author

isuruf commented Nov 26, 2019

Even in PYBIND11_USE_CMAKE mode, the headers are installed in site-packages/pybind11/include inside the python installation, which duplicates the headers, but that's a small price to pay IMO to allow the different use-cases namely, cmake installs, pip installs, pep517 installs, submodules.

@ax3l
Copy link
Collaborator

ax3l commented Nov 27, 2019

Okay, sounds good to me.
I actually meant CMake-only mode. But I can use the USE_PYTHON_INCLUDE_DIR option for that, too I guess...
Update: just tested, works as well.

@wjakob
Copy link
Member

wjakob commented Nov 28, 2019

@isuruf : I'll go ahead and merge this then, or did you plan to make additional changes?

@isuruf
Copy link
Contributor Author

isuruf commented Nov 28, 2019

No. This is ready from my end.

@wjakob
Copy link
Member

wjakob commented Nov 28, 2019

Ok -- merged!

@wjakob wjakob merged commit 3735249 into pybind:master Nov 28, 2019
@isuruf isuruf deleted the headers branch November 28, 2019 08:26
@marscher
Copy link

Many thanks to all contributes/reviewers to fix this long outstanding issue! 👍

@SylvainCorlay
Copy link
Member

pep517 is causing so much breakage, especially with the user isolation...

@ax3l
Copy link
Collaborator

ax3l commented Dec 4, 2019

But the pyproject.toml is so nice :)
Ok, enough spamming...

@isuruf
Copy link
Contributor Author

isuruf commented Jan 28, 2020

@wjakob, any plans for a new release using this?

@xunkai55
Copy link

+1 for the release. It would help a lot :-)

@wjakob
Copy link
Member

wjakob commented Mar 31, 2020

FYI: I've finally release v2.5.0, which includes this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants