[packaging] using pybind11 in setuptools.setup_requires or pyproject.toml in a dependent project does not work.
Closed this issue · 19 comments
If pybind11 is dragged in with setuptools.setup_requires, the method to install pybind11 will create an (temporary) egg file, which will not invoke install_headers. IMHO the Python package should contain the headers it self and not install to $INCLUDE_DIR.
By changing the packaging to simply include the headers as package data and let pybind11.get_include() point to this location, we circumvent these problems. However I'm not aware of the implications of this. I guess one would be, that people would have to use get_include to extend the include_dirs of their Extensions, because at the moment $INCLUDE_DIR is included by default.
cc @dean0x7d
If I understand correctly, this would mean replacing the existing headers=[...] in setup.py with:
data_files=[
("include/pybind11", glob("include/pybind11/**/*.h")),
]And then pybind11.get_include() would need to return this directory.
Note that there was a semi-related discussion in #652 where it was suggested that headers is preferable to data_files. I'm not entirely sure about the pros and cons here, so if anyone would like to chime in, feel free.
Just a couple of points:
-
If any change is made, it should not break compatibility. As long as dependent projects have been using
pybind11.get_include()this should not be an issue, but this might not always be the case. There is also compatibility with the CMake install onconda-forgewhich was introduced with #652. -
Not completely related, but at some point it would be nice to bundle the CMake files with the PyPI package, similarly to what has been done for the
conda-forgepackage. I'm not sure if these could be included as anything other thandata_files.
cc @SylvainCorlay Any thoughts on packaging and header vs data_files? I see that you've used data_files to add the headers to setup.py for xtensor.
I would suggest to do both installation methods in a transition and deprecation phase. In the long term having to use get_include seems to be the most rational solution.
Note that include/pybind11/ is different from the location where distutils' install_headers directive places headers. It will be the same directory in most cases, but not in other such as with the homebrew python installation. This is the reason why we use the pip.location module to get this information.
Also, I would advise against having two copies of the headers in the installation.
The point of the PR pointed to by @dean0x7d was to generate the cmake project files without changing the actual installation directory. I did this so that users of the conda package can both use the cmake and python examples consistently.
I don't think that there is a perfect answer to this quite yet in the current state of Python packaging, and that the current approach in pybind makes the best of the state of the tools.
There is a PEP opened to add installation directories: https://www.python.org/dev/peps/pep-0491/#install-paths
I agree with @marscher, using package_data would enable using pybind11 in setup_requires and also remove the hack in get_include. Here are some issues with it
-
First
user=Falseis used and thenuser=Trueis used. If the order is changed, then this would use a different header file than the package. -
Only standard installation dir and
--useris supported. For example--prefixis not. Also when the package is installed by package-managers like debian to dist-packages, the path is wrong. (Although, since the headers are installed to a path in the compiler's path, this works okay.) -
setup_requiresis not supported and therefore sdists of downstream packages don't work properly.
My suggestion is to use both headers and package_data
Expanding on 1., current recommendation at https://github.com/pybind/python_example/blob/master/setup.py#L29 makes user directory of lower priority, but python has user directory in higher priority which makes the compiler use system installation headers which may be from an outdated version of pybind11
Expanding on 1., current recommendation at https://github.com/pybind/python_example/blob/master/setup.py#L29 makes user directory of lower priority, but python has user directory in higher priority which makes the compiler use system installation headers which may be from an outdated version of pybind11
This should probably be inverted.
Also, user installs are generally evil.
Also, user installs are generally evil.
They are better than installing to a system directory using sudo
If all python installs looked like conda, I would be in favor of placing the headers in the general $PREFIX/include/pybind11 using package_data instead of the python's include directory. This seems much simpler than using the seldom used install_headers distutil directive.
The issue is that not all python installations seem to work that way. Using package_data to place the headers under some subdirectory of sys.prefix assuming that it is a regular installation prefix in which python was installed may result in them being placed in some location that is not a standard include directory.
Another interesting fact: distutils' install_headers directive generally places things in a subdirectory of the Python include directory, but in some cases like homebrew's python, the distutils scheme results in install_headers placing things elsewhere. This is why we have this complex logic to retrieve where this goes.
At the moment, in the conda feedstock for pybind, we tried to accomodate both cmake and distutils users of pybind with a single copy of the headers
- headers are installed in the same directory as they would be with the install_headers distutils directive, but with cmake, so that
projectConfig.cmakeare properly generated. This is achieved with the cmake optioncmake -DUSE_PYTHON_INCLUDE_DIR=ON. - then we run the python package installation, the
PYBIND11_USE_CMAKEenvironment variable prevents the reinstallation of the headers.
I'm not suggesting installing in $PREFIX/include. I'm suggesting installing in site-packages/pybind11/include/.
(This is what numpy does. Numpy installs in site-packages/numpy/core/include)
I'm not suggesting installing in $PREFIX/include. I'm suggesting installing in site-packages/pybind11/include/.
In this case, how can this directory be found without calling Python (with e.g. a cmake project depending on pybind11?)
In this case, how can this directory be found without calling Python (with e.g. a cmake project depending on pybind11?)
Same way numpy's directory is found, i.e. by calling python. (You anyways have to call python to get its include directories). You can also install using headers and using package_data twice to support both.
Has anyone tested yet, if the game changes when using pep517 to install pybind11 as build dependency? If not I can do it later.
It is not possible to use the pybind11.get_include(True|False) function to obtain the include dir from a temporary pep517 install. The returned include dir points to the Python interpreter used to invoke pip.
It is not possible to use the pybind11.get_include(True|False) function to obtain the include dir from a temporary pep517 install. The returned include dir points to the Python interpreter used to invoke pip.
I can confirm this. It seems to me this is the only issue which prevents pybind11 from being used correctly with pep517 build-system requires.
I'm experimenting with a workaround in setup.py which attempts to construct the right path. (I wonder if there is a way to detect whether you're inside a pep517 environment.)
My goal is to ship a source distribution that can builds correctly using pip, without pybind11 being installed beforehand, nor having to declare it in install_requires (so that the resultant wheels don't require it).
I totally understand that the need of a Python invocation in CMake to get the headers is undesirable. On the other hand, when the c++ bridge is done and you only want to ship the headers, a submodule is also not very favorable, since it tends to be out of date quickly.
My suggestion to solve this now, is to do copy the headers both to include and Numpy site-package style to satisfy both worlds. I mean the headers a only a few kb in size, it doesn't hurt anybody, but solves both problems.
Sorry I didn't read all posts, this was basically @isuruf suggestion. Should I open up a PR?
One hack is to simply copy the header files to a pybind11/ directory within the tmp install directory:
class get_pybind_include(object):
"""Helper class to determine the pybind11 include path
The purpose of this class is to postpone importing pybind11
until it is actually installed, so that the ``get_include()``
method can be invoked. """
def __init__(self, user=False):
self.user = user
def __str__(self):
import pybind11
# /private/var/folders/7n/6_pkgj9x4n1d81sq7d0mq4tm0000gn/T/pip-req-build-fwej8wpf/.eggs/pybind11-2.2.4-py3.7.egg/pybind11/__init__.py
f = pybind11.__file__
# /private/var/folders/7n/6_pkgj9x4n1d81sq7d0mq4tm0000gn/T/pip-req-build-fwej8wpf/.eggs/pybind11-2.2.4-py3.7.egg/pybind11/
d = os.path.dirname(f)
# /private/var/folders/7n/6_pkgj9x4n1d81sq7d0mq4tm0000gn/T/pip-req-build-fwej8wpf/.eggs/pybind11-2.2.4-py3.7.egg
# /private/var/folders/7n/6_pkgj9x4n1d81sq7d0mq4tm0000gn/T/pip-req-build-fwej8wpf/.eggs/pybind11-2.2.4-py3.7.egg/*.h
dd = os.path.dirname(d)
# /private/var/folders/7n/6_pkgj9x4n1d81sq7d0mq4tm0000gn/T/pip-req-build-fwej8wpf/.eggs
ddd = os.path.dirname(dd)
# check if not a half-install
if not os.path.exists(os.path.join(dd, 'pybind11.h')):
return pybind11.get_include(self.user)
# if it *is* a half-install
# Then copy all files to
# /private/var/folders/7n/6_pkgj9x4n1d81sq7d0mq4tm0000gn/T/pip-req-build-fwej8wpf/.eggs/pybind11
if not os.path.isdir(os.path.join(ddd, 'pybind11')):
import shutil
shutil.copytree(dd, os.path.join(ddd, 'pybind11'))
return dddThanks to @isuruf for helping with this.
I've taken a somewhat different approach:
class get_pybind_include(object):
"""Helper class to determine the pybind11 include path
The purpose of this class is to postpone importing pybind11
until it is actually installed, so that the ``get_include()``
method can be invoked. """
def __init__(self, user=False, pep517=False):
self.user = user
self.pep517 = pep517
def __str__(self):
import os
import pybind11
interpreter_include_path = pybind11.get_include(self.user)
if self.pep517:
# When pybind11 is installed permanently in site packages, the headers
# will be in the interpreter include path above. PEP 517 provides an
# experimental feature for build system dependencies. When installing
# a package from a source distribvution, first its build dependencies
# are installed in a temporary location. pybind11 does not return the
# correct path for this condition, so we glom together a second path,
# and ultimately specify them _both_ in the include search path.
# https://github.com/pybind/pybind11/issues/1067
return os.path.abspath(
os.path.join(
os.path.dirname(pybind11.__file__),
"..",
"..",
"..",
"..",
"include",
os.path.basename(interpreter_include_path),
)
)
else:
return interpreter_include_path
...
setuptools.Extension(
include_dirs=[
# Support `build_ext` finding pybind 11 (provided it's permanently
# installed).
get_pybind_include(),
get_pybind_include(user=True),
# Support building from a source distribution finding pybind11 from
# a PEP 517 temporary install.
get_pybind_include(pep517=True),
]
)(see pybind/python_example#45)
@paulmelnikow, that only works in a pep517 project with a pyproject.toml. It doesn't work in a new virtualenv.