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

Python 3 support #2351

Open
MisterErwin opened this issue Nov 7, 2018 · 11 comments

Comments

Projects
None yet
8 participants
@MisterErwin
Copy link

commented Nov 7, 2018

With the soon(ish) deprecation of python 2 (January 2020) a python 3 supporting module would be very nice.
Be it by plain replacing the python 2 module or adding a python 3 one. But py2to3 should be usable for most cases (imho).

Apparently there already have been PRs (e.g. #1686 ) but those were closed due to inactivity.

@herwinw

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2018

It feels like the current rlm_python has support for Python 3, but needs a bit of work to make it really work.

Configure works (output truncated to relevant parts):

~/freeradius-server/src/modules/rlm_python$ ./configure --with-rlm-python-bin=$(which python3.6)
configure: Python sys.prefix "/usr"
configure: Python sys.exec_prefix "/usr"
configure: Python sys.version "3.6"
checking for Python.h in /usr/include/python3.6/... yes
checking for Py_Initialize in -lpython3.6m in /usr/lib/python3.6/config... yes

Make generates a few warnings that need fixing (but it might be Python 3.6 is too new)

src/modules/rlm_python/rlm_python.c:228:19: warning: implicit declaration of function ‘PyString_AsString’; did you mean ‘PyBytes_AsString’? [-Wimplicit-function-declaration]
src/modules/rlm_python/rlm_python.c:287:9: warning: implicit declaration of function ‘PyString_CheckExact’; did you mean ‘PyLong_CheckExact’? [-Wimplicit-function-declaration]
src/modules/rlm_python/rlm_python.c:303:15: warning: implicit declaration of function ‘PyInt_Check’; did you mean ‘PySet_Check’? [-Wimplicit-function-declaration]
src/modules/rlm_python/rlm_python.c:304:10: warning: implicit declaration of function ‘PyInt_AsLong’; did you mean ‘PyLong_AsLong’? [-Wimplicit-function-declaration]
src/modules/rlm_python/rlm_python.c:367:10: warning: implicit declaration of function ‘PyString_FromFormat’; did you mean ‘PyBytes_FromFormat’? [-Wimplicit-function-declaration]
src/modules/rlm_python/rlm_python.c:996:18: warning: implicit declaration of function ‘Py_InitModule3’; did you mean ‘Py_Initialize’? [-Wimplicit-function-declaration]
@arr2036

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

Not closed due to inactivity, closed due to v4.0.x being renamed if the OP could reopen against master branch that'd be great.

@Deisuan

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

It feels like the current rlm_python has support for Python 3, but needs a bit of work to make it really work.

Well, it has been admittedly about two years since I've worked on the PR, but as far as I can recall, the changes are quite not trivial. The internal cpython implementation for - among others - strings and global interpreter lock have changes substantially. The work of porting the rlm_python to Python3 is basically done (see PR #1686 ), however it fails with more than one instance of the module at the same time. I recall there being similar (or even the same?) issue with rlm_python for Python2. With one module it has been running quite stable for about two years in our production system ;-)

@arr2036 Would it be an acceptable intermediate solution to change the travis checks such that only one instance is made, merge the PR #1686 (after a rebase by me of course), and expand to two module support at a later time? Just

@alandekok

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

I would suggest just creating two modules. rlm_python3 and rlm_python2.

If we want to have just an rlm_python, it should likely be python3.

@herwinw

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

Before forking the module to create a rlm_python3, please keep in mind that #2334 contains some very useful changes for rlm_python.

@alejandro-perez

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2018

Indeed, if we went for rlm_python3, I would vote for getting rid of the tuple-based interface in favor of the dict-based one, taking advantage that backwards compatibility would not be an issue.

@cipherboy

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2018

It'd be nice if these changes were considered for the v3.0.x branch as well. It is unlikely we'll be ship v4.0.x any time soon at Red Hat or in Fedora, so we'll need Python 3 support in the v3.0.x...

@arr2036

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

I'm not sure we should really concern ourselves with the released schedules of RedHat and Fedora, especially as we provide our own packages. I'm generally against adding new functionality to older branches, and am definitely against maintaining two versions of rlm_python. Python's C API is poorly documented and there are existing threading issues with the current rlm_python module. IMHO this should go into v4.0.x, and we should stick with one python version. People are going to be reworking their configs during the upgrade anyway.

@cipherboy

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2018

Duly noted. I'm not trying to suggest a time frame to have this done by, merely that I'm watching and will fit it into our releases as it gets done. I'll keep this as something I'll likely have to backport.

Thanks!

@cfrademan

This comment has been minimized.

Copy link

commented Feb 10, 2019

Hi there. We implementing some third party software with FreeRadius. We running a bit low on time here and happy to go forward using python2 for time being. However what is the plans moving forward with python3 support? I do not see in the milestones for the 4x branch? We got less than a year left on python2. It makes sense that 4.x only supports python3 and sticking with one module rlm_python.

@alandekok

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

If you can verify PR #1686, we can pull it into v4. With a note that rlm_python in v4 is only Python3.

If anyone has time to create an rlm_python3 for v3, we're happy to look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.