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

feat: setup.py redesign and helpers #2433

Merged
merged 28 commits into from Sep 16, 2020
Merged

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Aug 24, 2020

Part 1 of ~3 for the Python packaging work (the main part). The other two will be very small (possibly with no code changes in part 3).

Requirements

The requirements to build from source have increased significantly, to build from the sdist have increased slighly, and the universal wheel requirements have not changed.

  • From source: requires pip 10+ OR cmake 3.15+, ninja or make, and setuptools 30.3.0+; valid compilers.
  • From SDist: setuptools 30.3.0+ (8 Dec 2016) OR pip 10+
  • From universal wheel: no change

Design:

  • pybind11 is now a "nice" python package, with files only in the site-packages/pybind11 dir.
  • The headers slot and (new for pybind11) data slot that install to your environment root are now only installed if you ask for pybind11[global].

The core implementation is that there is a develop setup.py and pyproject.toml that work with CMake, but sdists get a much simpler, CMake-free tools/setup_main.py (or tools/setup_global.py). (note this trick is required for CMake/non-CMake requirement, not for global/regular. See the first commit if you want to see a version that did not do this, but was much more complex, and didn't solve the pyproject.toml cmake dependency issue either). Single-sourcing the version depends on this infrastructure, too.

Features:

Key highlights are CMake configuration support, setuptools helpers, including new Windows support (when compared to the official example repository), sdist/wheel building on CI, and packaging & setup tests.

  • Setuptools helper file (can be used standalone or from project in PEP 518 mode / preinstalled / setup_requires)
    • Full Windows support, mostly follows the CMake default flags
    • Based on the example, improved by studying Matplotlib's Cario module and others and lots of testing. Refactored twice from feedback - now Extension focused.
  • Better separation of Python components into modules (avoids tricks to hide items in __init__.py scope)
  • PEP 440 3 digit versioning for development versions too.
  • Version checked against source by main setup.py
  • Usage of setup.cfg for static metadata (fix several missing components, like python_requires)
  • Using PEP 518 pyproject.toml to ensure CMake is available to build the sdist / wheel from source (note that building from sdist does not require cmake, just from GitHub source)
  • Include the CMake configure files in the wheel and sdist, running cmake to create and copy files in (accessed through a helper function in the normal install, get_cmake_dir(), see below)
  • Black formatting for the source directory (not tests)
  • Globing checks to throw errors if list of source files is incorrect - moved to tests
  • SDists and wheels are being made and can be downloaded directly from GitHub Actions (direct publish to PyPI will be added in a later PR)
  • check-manifest helps warn if a file is added that does not go into the sdist.
  • Explicit tests verify every file in the two sdists and in the two wheels.

IMG_1891

Aside on where to put the files

There are three possible places to put the includes (and CMake header files):

  1. In the package. This is currently done in pybind11, and is the best place for access from Python.
  2. In pybind11-2.6.0.dev1.data/headers. This is done in versions < 2.6, these are where Python sort-of expects headers (For testing mostly so far). However, since version 2.5 or so, pybind11's function return method 1 locations, since these have some issues. I'm proposing in this PR to move this location to a separate package "pybind11-global" (or other name)
    • Files are installed at <prefix>/include/Python3.8m/pybind11 usually
    • Causes warnings in brew in the current version if you pip install pybind11 without a virtual environment.
  • In pybind11-2.6.0.dev1.data/data. This allows "normal" nesting, that is, CMake works correctly. It also is in the ideal place for discovery or manual includes. Added to pybind11-global
    • Files installed at <prefix>/include/pybind11 and <prefix>/share/cmake/pybind11.

The problem with methods 2 & 3 are that they install to <prefix>, which is not always a good idea if you do not use PEP 518 or virtual environments to install pybind11. Brew, for example, throws warnings when unbrewed files are in /user/local. In part 3, I want to make this work correctly with CMake and Scikit-build-like installs, so having this be discoverable by CMake is important. So I've created a way to produce an alternate package, pybind11-global, which has methods 2 & 3 and is installed as an extra, pybind11[global].

Further work

I have part 2 basically ready, which adds a shiny new feature on the shared distutils infrastructure (privately previewed to @YannickJadoul). Part 3 will add testing / instructions for CMake builds based on the python package (possibly with no changes to pybind11 itself). I will also add publish to PyPI to GHA later.

Warning example with pybind11 2.5.0:

$ pip3 install pybind11
$ brew doctor

Warning: Unbrewed header files were found in /usr/local/include.
If you didn't put them there on purpose they could cause problems when
building Homebrew formulae, and may need to be deleted.

Unexpected header files:
  /usr/local/include/python3.8/pybind11/attr.h
  /usr/local/include/python3.8/pybind11/buffer_info.h
  /usr/local/include/python3.8/pybind11/cast.h
  /usr/local/include/python3.8/pybind11/chrono.h
  /usr/local/include/python3.8/pybind11/common.h
  /usr/local/include/python3.8/pybind11/complex.h
  /usr/local/include/python3.8/pybind11/detail/class.h
  /usr/local/include/python3.8/pybind11/detail/common.h
  /usr/local/include/python3.8/pybind11/detail/descr.h
  /usr/local/include/python3.8/pybind11/detail/init.h
  /usr/local/include/python3.8/pybind11/detail/internals.h
  /usr/local/include/python3.8/pybind11/detail/typeid.h
  /usr/local/include/python3.8/pybind11/eigen.h
  /usr/local/include/python3.8/pybind11/embed.h
  /usr/local/include/python3.8/pybind11/eval.h
  /usr/local/include/python3.8/pybind11/functional.h
  /usr/local/include/python3.8/pybind11/iostream.h
  /usr/local/include/python3.8/pybind11/numpy.h
  /usr/local/include/python3.8/pybind11/operators.h
  /usr/local/include/python3.8/pybind11/options.h
  /usr/local/include/python3.8/pybind11/pybind11.h
  /usr/local/include/python3.8/pybind11/pytypes.h
  /usr/local/include/python3.8/pybind11/stl.h
  /usr/local/include/python3.8/pybind11/stl_bind.h

Questions

Is pybind11-inplace a good name? pybind11-global, pybind11-alt, and pybind11-cpp are some other ideas. pybind11-core is not a good idea, as that implies that pybind11 depends on it.

Currently pybind11-global seems to be winning, switched to that.

@henryiii henryiii force-pushed the feat/setup branch 4 times, most recently from af17c99 to b2d0928 Compare August 24, 2020 18:25
@henryiii henryiii force-pushed the feat/setup branch 19 times, most recently from cc035a8 to 2337cfe Compare August 26, 2020 16:43
@henryiii henryiii marked this pull request as ready for review August 26, 2020 16:57
@henryiii
Copy link
Collaborator Author

Question for others now or for me later: Are there any things in https://github.com/matplotlib/mplcairo 's setup.py that we might want to include? Is it easy to further adjust the helper we provide to add the specific parts (I think so)?

@henryiii
Copy link
Collaborator Author

(I force-pushed a rebase + a few typo corrections in docs)

@henryiii henryiii requested a review from wjakob August 26, 2020 18:20
Copy link
Member

@wjakob wjakob left a comment

Choose a reason for hiding this comment

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

I had another look at this PR. @ktbarrett, and @henryiii, thanks for these very detailed and helpful explanations above. I think I understand the problem now, and why these changes are necessary.

I feel like the documentation still needs some work, especially the parts in CONTRIBUTING.md -- this file is full of tooling and packaging jargon. I haven't been working much with these fancy new tools and find parts of the explanation just incomprehensible. It don't think this is a big problem though -- much of the explanation on the PR discussion here explains the subtleties, and that information can be put into a linear structure and replace some of the text that is currently in CONTRIBUTING.md.

.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
.github/CONTRIBUTING.md Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
docs/changelog.rst Outdated Show resolved Hide resolved
docs/compiling.rst Outdated Show resolved Hide resolved
docs/compiling.rst Outdated Show resolved Hide resolved
docs/compiling.rst Outdated Show resolved Hide resolved
docs/compiling.rst Show resolved Hide resolved
@@ -25,6 +25,13 @@ Usage of the ``PYBIND11_OVERLOAD*`` macros and ``get_overload`` function should
be replaced by ``PYBIND11_OVERRIDE*`` and ``get_override``. In the future, the
old macros may be deprecated and removed.

The ``pybind11`` package on PyPI no longer fills the wheel "headers" slot - if
you were using the headers from this slot, they are available by requesting the
``global`` extra, that is, ``pip install "pybind11[global]"``. (Most users will
Copy link
Member

Choose a reason for hiding this comment

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

Really stupid question: does this mean that there will be two separate packages on PyPI, or is [global] some kind of flag that affects the behavior of Pip when installing the package?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. The thing in brackets is called an "extra", short for extra requirements, and is supported everywhere in the Python packaging ecosystem; requirements lists, pip, pyproject.toml, etc. They are commonly used to add [test] requirements, for example - see Pandas. You'll also see setuptools_scm[toml], which is how you ensure you have the toml parser so you can use pyproject.toml + setuptools_scm. It can only enable extra packages as install requirements, not arbitrary changes. The extra requirement it enables in this case is a package called pybind11-global, which I'll explain above. But this is the public suggestion for adding it.

@henryiii
Copy link
Collaborator Author

@YannickJadoul and @wjakob, here's an example of what a dual-source version would look like. You'd have to update the version twice instead of once, but you do get a pretty immediate error in CI or when running setup.py if the versions don't match. I generally like reducing duplication on something that tends to get changed, but it does make things a bit simpler.

henryiii@15ab79e

two packages; the "nice" pybind11 package that stores the includes and CMake
files inside the package, that you get access to via functions in the package,
and a `pybind11-global` package that can be included via `pybind11[global]` if
you want the more invasive but discoverable file locations.
Copy link
Member

Choose a reason for hiding this comment

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

this intro paragraph is so much better now, thanks for that!

Copy link
Member

Choose a reason for hiding this comment

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

One more stupid question: what does pip install pybind11[global] do if pybind11 is already installed?

Copy link
Collaborator Author

@henryiii henryiii Sep 16, 2020

Choose a reason for hiding this comment

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

It just installs pybind11-global. Just as if you wrote pip install pybind11 pybind11-global and already have pybind11 installed. The only special feature is that they are version-locked (with the bracket syntax) and you get both of them.

Copy link
Member

Choose a reason for hiding this comment

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

Great, thank you for the clarification.

docs/compiling.rst Outdated Show resolved Hide resolved
Copy link
Member

@wjakob wjakob left a comment

Choose a reason for hiding this comment

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

approved by me (modulo having the version number in two places, as discussed on Slack)

@henryiii henryiii merged commit fd61f50 into pybind:master Sep 16, 2020
@YannickJadoul
Copy link
Collaborator

Thanks a lot, @henryiii! Especially for the patience and numerous times you re-explained stuff :-)

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