linux-nvme / libnvme

C Library for NVM Express on Linux
GNU Lesser General Public License v2.1
167 stars 125 forks source link

compare between `mesonpy` and `mesonpep517` python build backends #271

Closed glimchb closed 2 years ago

glimchb commented 2 years ago

Issue/reminder for myself @glimchb to learn more and compare:

igaw commented 2 years ago

We can't upload the binaries with mesonpep517, because we don't do the repair step. This leads to the situation that PyPi says linux_x86_64 is not supported. The repair step would fix this up to manylinux_.

The repair step doesn't work because mesonpep517 is not packaging up libnvme.so only the Python c binding library is in the dist package. There is also no way to workaround in tricking meson to install libnvme.so into the python side-config directory because config_settings is not supported in v0.2. And there was no release for over one year. I guess we can say this project is not ready for this tasks.

mesonpy is not working either, at least not out of the box and I am running out of steam. I think we can't really upload precompiled binaries at this point. Uploading just the source code to PyPi is currently the only working solution I suppose.

(I've spend way too much time today to figure this out.)

igaw commented 2 years ago

And after playing with build_sdist, I found out that we have the exact problem described here: https://github.com/pybind/cmake_example/issues/11

Basically, we would need to build libnvme as a static library and link it into the python binding library to get this working correctly. Unless you ask the user to install libnvme.so and then use pip to install the python binding. If the user already installed libnvme via a package manager, why not just not installing the Python binding as well via the same means. And it's not installed via a package manager instead via source, just let the user also install the binding in the same way.

This seems all pretty much a big mess and I think at this point there is little point in trying to upload anything to PyPi. The Python binding to a shared library use case is not directly supported (which I do understand why) and can only make working with some serious workarounds. And I am not willing to invest the time to do this.

First of, we would need to be able to build libnvme completely statically (not possible at this point, we are missing libuuid as wrap) and this needs to be linked into the Python binding library. I haven't seen any hooks how to do it or at least not with mesonpep517 v0.2.

Given this, I am going to remove the whole PyPi integration attempt. If the upstream situation changes and all fits into place nicely I don't mind having it added back. But at it is right now, it's just not helping anyone.

glimchb commented 2 years ago

thanks @igaw indeed a lot o work you put into this and I put originally about the same amount of time I don't object to your decision to remove PyPi integration attempt for now We can re-introduce it once we figure out how to do it properly

igaw commented 2 years ago

So the only thing I am still pondering around, if uploading just the sdist would actually make sense nonetheless. I am sure we are not the only one in this situation. Are there any projects which just upload the Python binding?

glimchb commented 2 years ago

yes, I know many projects just upload sdist without wheels This means it will be built during pip install wheels is just an optimization for example https://pypi.org/project/PyGObject/#files

igaw commented 2 years ago

Okay, I don't mind doing the sdist part. I thought it's not that useful.

igaw commented 2 years ago

FWIW, I've uploaded the binding to PyPi manually (yeah, I know, sneaky thing...) but I wanted to get the project name claimed on PyPi before someone else is getting it. So the next release should work through the normal CI means.

glimchb commented 2 years ago

just FYI, I was struggling last time with wheels as well, that's why I left CIBW_REPAIR_WHEEL_COMMAND_LINUX explicitly empty I was hoping that using LD_LIBRARY_PATH to pint to libnvme.so location in CIBW_REPAIR_WHEEL_COMMAND_LINUX we can make wheels to work

just documenting here for later... once we come back to this problem...

glimchb commented 2 years ago

FWIW, I've uploaded the binding to PyPi manually (yeah, I know, sneaky thing...) but I wanted to get the project name claimed on PyPi before someone else is getting it. So the next release should work through the normal CI means.

Yay. looks good ! https://pypi.org/project/libnvme/

igaw commented 2 years ago

I think the proper way is to get this kind of setup supported is to teach mesonpep517 to handle it internally. Having to maintain LD_LIBRARY_PATH on our level is a 'awkward'...

glimchb commented 2 years ago

just for documentation this looks ok

ubuntu@localhost:~$ pip install libnvme
Collecting libnvme
  Downloading libnvme-1.0.tar.gz (337 kB)
     |████████████████████████████████| 337 kB 2.5 MB/s
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
    Preparing wheel metadata ... done
Building wheels for collected packages: libnvme
  Building wheel for libnvme (PEP 517) ... done
  Created wheel for libnvme: filename=libnvme-1.0-cp38-cp38-linux_aarch64.whl size=76671 sha256=3e1555e9be79b9fb2fc32f5c0b9bfb0a1316f14c3a235585c511dce84310a3bc
  Stored in directory: /home/ubuntu/.cache/pip/wheels/ea/33/1f/c50122cba5ecefc0b094cac6511caaeceb3d449696d1c2c122
Successfully built libnvme
Installing collected packages: libnvme
Successfully installed libnvme-1.0
igaw commented 2 years ago

Hmm, I see 'wheel'. Do we need to keep the wheel requeriment in pyproject.toml?

glimchb commented 2 years ago

non-no-no, this is excellent. Exactly as expected. wheel is always built on the machine during the pip install phase. If we upload wheels to pypi, then during installation they are just downloaded instead of build in place...

glimchb commented 2 years ago

Hmm, I see 'wheel'. Do we need to keep the wheel requeriment in pyproject.toml?

yes, we can leave wheel as dependency inside pyproject.toml

eli-schwartz commented 2 years ago

First of, we would need to be able to build libnvme completely statically (not possible at this point, we are missing libuuid as wrap)

https://github.com/util-linux/util-linux/pull/1654

igaw commented 2 years ago

@eli-schwartz wow! Thanks for helping out. It was on my TODO list and even started with it but got nowhere...

eli-schwartz commented 2 years ago

It is now merged upstream, please give it a try.

glimchb commented 2 years ago

something like this https://github.com/linux-nvme/libnvme/pull/349 ? not sure...