-
Notifications
You must be signed in to change notification settings - Fork 99
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
Add check for nmstate C library on pip install #2627
base: base
Are you sure you want to change the base?
Conversation
Hi @jona42-ui. Thanks for your PR. I'm waiting for a nmstate member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
c17b2db
to
609d0f2
Compare
does this seem to address the issue?? cc @ihuguet |
c17b2db
to
0be45b0
Compare
I don't know how is this normally done. I would bet that setuptools has a way to do this, but as I say, I don't know. Let's see what others think. |
thanks @ihuguet for the feedback. @cathay4t @ffmancera are you having sometime to look at this |
Please provide proof that setup.py can actually install C library of nmstate. From what I see, it will never works |
oops seems am getting the workflow here wrongly, |
@cathay4t you can correct me if am still wrong |
what would be the best appraoch here. |
Pip should not compile C library, so I think the installation should fail with error message guilding user to install nmstate C library. I recall python gobject binding required gobject C binding been installed, so I guess you may check how their setup.py looks like. |
thanks on it |
from the reference you gave me https://github.com/pygobject/pgi/blob/master/setup.py (I hope its the one) so yes I think your suggestion would work best to allow the user install the nmstate c lib separately |
Yes. I was shocked when you have Enjoy hacking. |
BTW, to check whether nmstate C library is installed, you may call |
yes I do |
Can you do Notice that
|
Please use this command to install |
okay |
8aa0d78
to
fb275e3
Compare
its like I have no lib4 directory, I have lib with the libnmstate.so.2 file |
After running
Please let me know if you encountered the same error |
in the shell after importing ctypes, is this what we expected? |
I also tried ; try: it says |
Then you did not encounter the error anymore. |
I cant tell why the CI really fails |
You can try to run the command |
the first command terminates with error; |
@liangwen12year can we jump on call and get this off the radar |
Sure, give me your availability. I will schedule the call/meeting. |
am available now. |
Let us follow up in |
okay. |
@jona42-ui , I ran the CI locally, indeed, I can not find |
oops! |
Added check for the nmstate C library . If the library is not found,error occurs, guiding the user to install the nmstate C library separately before installing the Python package. This enhancement ensures the pypi package is efficiantly usabel on install. Signed-off-by: jona42-ui <jonathanthembo123@gmail.com>
adc3a62
to
47356e8
Compare
rpm_build (el9) CI failure seems to be expected, if you look at the script |
thanks. makes sense. so we need a tweak to the workflow script?? |
we need to tell whether we are in make install or pip install. For So you need to find a way to tell whether the command invoker is using pip or not. Meanwhile, the setup.py install is deprecated, any changes to it should move to new python suggested way: PEP-518 https://peps.python.org/pep-0518/ and PEP 517 https://peps.python.org/pep-0517/ I do not have urgent need on changes to any python code, so take your time and read PEP 517 and 518. |
You can take a look Gris's comment, for |
Added check for the presence of the nmstate C library in the setup.py script.
This commit modifies the setup.py script to check for the presence of the nmstate C library using ctypes.cdll.LoadLibrary("libnmstate.so.2"). If the library is not found, a
RuntimeError
is raised, guiding the user to install the nmstate C library separately before installing the Python package. This enhancement ensures that users are informed about the dependency on the nmstate C library before proceeding with the installation.Resolves: #2575