mesonbuild / meson-python

Meson PEP 517 Python build backend
https://mesonbuild.com/meson-python/
MIT License
120 stars 59 forks source link

ENH: always require patchelf on Linux #490

Closed dnicolodi closed 10 months ago

dnicolodi commented 10 months ago

patchelf is required on Linux when executables, libraries, or extension modules in the package link to a shared library itself also distributed with the package.

To determine whether patchelf is required, the project to be built into a wheel needs to be configured. Because ephemeral build directories are used, the project needs to be configured a second time when the wheel is actually built. For all but the simplest projects the time spent in meson setup is significant.

This patch removes the check and thus the requirement for configuring the project twice, and assumes that patchelf is always required on Linux: dependency on the patchelf package is added when a suitable patchelf executable is not found in the environment.

Configuring the project requires ninja to be available. When ninja is not available the it was assumed that patchelf is required. The check was also implemented incorrectly and did not actually check whether a shared library is included in the wheel. Therefore, this patch changes whether the patchelf package is installed only when projects that do not include any native code components that are build on systems where ninja is available an patchelf is not.

The impact of this change is largest for pure Python projects. To measure it, I timed the time required to build wheels for meson-python itself with ninja and patchelf installed or not in the environment:

  ninja  patchelf  main          this
  -----  --------  ------------  ------------
  no     no        6.5 +- 0.3 s  6.5 +- 0.3 s
  yes    no        6.0 +- 0.3 s  5.8 +- 0.3 s
  yes    yes       5.6 +- 0.6 s  5.6 +- 0.6 s

Timings are from 7 consecutive runs of python -m pip wheel. All the required dependencies are in the pip cache. Using python -m build -w the build time is around 9 seconds and is dominated by the time requires to install pip in the isolated build environment, so the run time differences less pronounced. Scatter in the data is dominated by the network round trip times of pip checking the package index for the dependencies latest versions.

Even in the least favorable case, this change does not have a negative impact on the wheel build time.

dnicolodi commented 10 months ago

@rgommers I've added an extended commit message with some timings.

rgommers commented 10 months ago

Thank you @dnicolodi, this description is convincing. I'll verify with a larger package on my machine, but I like how this looks.

dnicolodi commented 10 months ago

@rgommers Thanks for the review. Let me know if you want me to test it with other packages. I've also piled one more commit on top with a small simplification to the code.

dnicolodi commented 10 months ago

And I broke the tests doing so... Fixing them.

rgommers commented 10 months ago

Let me know if you want me to test it with other packages.

No, should be all good. I just want to confirm with numpy, which has a very expensive configure stage (~10 seconds). I'm pretty sure it'll look better with this PR when I uninstall patchelf locally.

rgommers commented 10 months ago

Tested on numpy, this is looking good. With /usr/bin/patchelf (reproducible to ~ +/-2 seconds):

$ time python -m build
real    1m21,879s
user    7m43,739s
sys     0m29,339s

After sudo pacman -R patchelf:

$ time python -m build
real    1m35,664s
user    7m55,182s
sys     0m32,964s

After then incorporating the changes from this PR:

real    1m21,873s
user    7m43,664s
sys     0m29,545s

After reinstalling patchelf:

real    1m20,177s
user    7m44,990s
sys     0m29,050s

So this is a significant gain when patchelf isn't installed on the system. And the improvement matches the time taken by meson setup builddir:

real    0m13,220s
user    0m9,947s
sys     0m3,903s