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

compare between mesonpy and mesonpep517 python build backends #271

Closed
glimchb opened this issue Mar 2, 2022 · 18 comments · Fixed by #339
Closed

compare between mesonpy and mesonpep517 python build backends #271

glimchb opened this issue Mar 2, 2022 · 18 comments · Fixed by #339
Assignees
Labels
enhancement New feature or request

Comments

@glimchb
Copy link
Contributor

glimchb commented Mar 2, 2022

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

@glimchb glimchb changed the title self: compare between mesonpy and mesonpep517 python build backends compare between mesonpy and mesonpep517 python build backends Mar 2, 2022
@igaw igaw added the enhancement New feature or request label Mar 17, 2022
@igaw igaw added this to the 1.1 milestone Apr 8, 2022
@igaw
Copy link
Collaborator

igaw commented Apr 11, 2022

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
Copy link
Collaborator

igaw commented Apr 12, 2022

And after playing with build_sdist, I found out that we have the exact problem described here: pybind/cmake_example#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
Copy link
Contributor Author

glimchb commented Apr 12, 2022

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
Copy link
Collaborator

igaw commented Apr 12, 2022

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
Copy link
Contributor Author

glimchb commented Apr 12, 2022

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

@glimchb glimchb closed this as completed Apr 12, 2022
@glimchb glimchb reopened this Apr 12, 2022
@igaw
Copy link
Collaborator

igaw commented Apr 12, 2022

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

@igaw
Copy link
Collaborator

igaw commented Apr 12, 2022

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
Copy link
Contributor Author

glimchb commented Apr 12, 2022

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
Copy link
Contributor Author

glimchb commented Apr 12, 2022

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
Copy link
Collaborator

igaw commented Apr 12, 2022

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'...

@igaw igaw removed this from the 1.1 milestone Apr 12, 2022
@glimchb
Copy link
Contributor Author

glimchb commented Apr 12, 2022

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
Copy link
Collaborator

igaw commented Apr 12, 2022

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

@glimchb
Copy link
Contributor Author

glimchb commented Apr 12, 2022

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
Copy link
Contributor Author

glimchb commented Apr 12, 2022

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

@igaw igaw closed this as completed in #339 Apr 12, 2022
@eli-schwartz
Copy link
Contributor

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

util-linux/util-linux#1654

@igaw
Copy link
Collaborator

igaw commented Apr 13, 2022

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

@eli-schwartz
Copy link
Contributor

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

@glimchb
Copy link
Contributor Author

glimchb commented Apr 13, 2022

something like this #349 ? not sure...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants