avocado-vt
avocado-vt copied to clipboard
Remove passfd files on python3 rpmbuild
We should avoid to introduce c files on python project, replace the function provided by passfd.c with socket.sendmsg and socket.recvmsg on systems with python3.3 or later
Hello @skycastlelily2, to be honest I never looked at this code before (which is good as it probably works well :-) ). Anyway after a review and some attempts I do understand why are you trying to remove it, but I'd suggest a different approach. I looked at pypi and it seems there is a python2 backport of the sendmsg: https://pypi.org/project/sendmsg/ How about using that on py2 and the python3 implementation on py3? As for the 3.0 - 3.3 versions we don't really need to treat them as Avocado requires python >= 3.4 so we can actually do that on Avocado-vt as well.
Thanks a lot for your review ,and helpful suggest.
Note: I haven't actually tried the module, just imported it, it provides sendmsg and recvmsg interface so I assume it should do what py3 implementation does.
help(sendmsg) doesn't show anything ,so I download the tar.gz file from pip, and try to figure out how to use it by myself,and then I find if I replace sendmsg.c with passfd.c, and modify the file accordingly,I will be able to have a passfd module installed on my system by running "pip install .",then I'm trying to put it to pypi.And that's the start of depressed two hours. paver is too old,I replace it with newer version,and modify the files ,add LICENSE.txt,finally successfully get the dist/*,but twine upload failed with" Binary wheel 'passfd-1.0.1-cp27-cp27mu-linux_x86_64.whl' has an unsupported platform tag 'linux_x86_64'".Look like I have to build manylinux wheel if I want to have passfd uploaded successfully. sigh...
passfd is in pypi now^^
Hello @skycastlelily2 was it necessary to create a new pypi package? I thought the existing passfd
would do, otherwise I don't think it's worth supporting it at all as it's just py2 which is slowly fading out. Anyway since you already did that, we could just rely on it provided it works (I haven't tried yet).
This pull request introduces 1 alert and fixes 1 when merging 0831d621d4cb69a406ddc7a4f28041083546f77e into 5e8a4668e5595208a4aa453724138b41ca80bbea - view on LGTM.com
new alerts:
- 1 for Syntax error
fixed alerts:
- 1 for Unnecessary 'else' clause in loop
Hello @skycastlelily2 was it necessary to create a new pypi package? I thought the existing
passfd
would do, otherwise I don't think it's worth supporting it at all as it's just py2 which is slowly fading out. Anyway since you already did that, we could just rely on it provided it works (I haven't tried yet).
Thought it won't take much effort,and would make the code a little clean,while make fedora package reviewer happy(the sendmsg in pypi doesn't work for me,as it seems that you are unable to pass sock param into the code).It works well,I have 100% reused the existing passfd.c,and I have tested it.But then I found there is already a python-passfd in pypi, which looks more professional and we are actually using that one here.
Omitting '\n' by mistake,sigh...,thanks a lot for your time on helping me check this,works well now.
Hi Lukas, I have updated it,and thanks a lot for your approval.
Hi Cleber, Would you please spare some time to do the 2nd review,so that we can let it go?Thanks a lot.
@clebergnu @wainersm would anyone of you please review this? It's been a long time and I believe it's ready to go...