avocado-vt icon indicating copy to clipboard operation
avocado-vt copied to clipboard

Remove passfd files on python3 rpmbuild

Open skycastlelily2 opened this issue 4 years ago • 8 comments

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

skycastlelily2 avatar Mar 13 '20 14:03 skycastlelily2

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

skycastlelily2 avatar Mar 20 '20 10:03 skycastlelily2

passfd is in pypi now^^

skycastlelily2 avatar Mar 20 '20 12:03 skycastlelily2

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

ldoktor avatar Mar 24 '20 12:03 ldoktor

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

lgtm-com[bot] avatar Mar 25 '20 09:03 lgtm-com[bot]

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.

skycastlelily2 avatar Mar 25 '20 10:03 skycastlelily2

Omitting '\n' by mistake,sigh...,thanks a lot for your time on helping me check this,works well now.

skycastlelily2 avatar Apr 03 '20 11:04 skycastlelily2

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.

skycastlelily2 avatar Apr 22 '20 03:04 skycastlelily2

@clebergnu @wainersm would anyone of you please review this? It's been a long time and I believe it's ready to go...

ldoktor avatar May 25 '20 10:05 ldoktor