This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: PyOS_Readline
Type: Stage:
Components: Interpreter Core Versions: Python 2.5
process
Status: closed Resolution: duplicate
Dependencies: Superseder:
Assigned To: Nosy List: dalcinl, greglielens
Priority: normal Keywords: patch

Created on 2005-07-04 18:03 by dalcinl, last changed 2022-04-11 14:56 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
patch.readline dalcinl, 2005-07-04 18:03 PyOS_Readline patch (obtained with 'cvs diff -c')
Messages (3)
msg48569 - (view) Author: Lisandro Dalcin (dalcinl) Date: 2005-07-04 18:03
Greg Lielens submitted some time ago a patch [id 955928] about
'PyOS_Readline()' behavior with non-interactive tty's. Basically,
there is no way to properly override 'PyOS_ReadlineFunctionPointer' 
as
'PyOS_Readline()' calls 'PyOS_StdioReadline()' when 'stdin' or
'stdout' are not tty's. A snippet of "Parser/myreadline.c":

...

if (!isatty (fileno (sys_stdin)) || !isatty (fileno (sys_stdout)))
        rv = PyOS_StdioReadline (sys_stdin, sys_stdout, prompt);
else
        rv = (*PyOS_ReadlineFunctionPointer)(sys_stdin, sys_stdout,
                                             prompt);
...

Greg Lielens is completely right about the problem, but his patch is
perhaps a bit crude, it also modifies "Python/bltinmodule.c" to solve
the same issue with 'raw_input'.

I think I have found a not so crude solution, and completely 
backward
compatible.  Basically, I moved 'isatty()' test from 'PyOS_Readline()'
in file "Parser/myreadline.c" to 'call_readline()' in file
"Modules/readline.c". In order to do that, I believe
'PyOS_StdioReadline' have to be added to file "Include/pythonrun.h".

All those changes will not affect in any way the behavior in
interactive sessions. Now 'PyOS_ReadlineFunctionPointer' can be
properly overrode and users of 'readline' module will not see any
change: in non-interactive tty's 'PyOS_StdioReadline()' will be called
anyway.  The problem in 'input' and 'raw_input' builtins remains, but
its solution is not so clear to me, I think this part should not be
touched right now.

This patch passes 'Lib/test/regrtest.py' (of course, all those tests
are non-interactive!). I use Python interpreter every day in the
standard way and also in MPI-parallelized interactive sessions with
this patch applied, and I never have any problem.

I send my patch obtained with 'cvs diff -c' from current sources.
msg48570 - (view) Author: Gregory Lielens (greglielens) Date: 2005-08-06 19:14
Logged In: YES 
user_id=1044428

Hi Lisandro, sorry that I did not follow this matter, we had
other short term focus in our company and I had trouble
finding time for it. Good that you subject this though, I
have checked it fast and from what I understand it would
have the same effect as my older patch...
However, I used some features of my patch that are not
present in yours:

in Parser/myreadline.c, I removed the test about
PyOS_ReadlineFunctionPointer being null to instead
initialising it at creation to the correct value depending
to system: This was important because it allows me in my
specific MPI Readline function to use this defautl functio
for the process 0...In general, it allows to re-use the
default in the new Readline function, incrementing
functionalities instead of re-implementing full
functionality. You will have the same mechanism with the
inclusion of PyOS_StdioReadline in the pythonrun API, but I
personally find this solution less appealing: it increase
the size of the API, and you do not have the correct
readline on VMS...That's why I have done it like this, even
if it may seems hackish ;) Of sourse manual has to be
updated to say that PyOS_ReadlineFunctionPointer has a
meaningfull defautl value...but it would also have to be
updated in your approach, to tell about PyOS_StdioReadline...

Regarding your modif of the Modules/readline.c, I have the
feeling that your approach is cleaner...because It seems
nicer to check for tty inside the new readline function,
instead of installing the new readline function only if
stdin/stdout are tty....You have to use  PyOS_StdioReadline
though, that would not be accessible if one does not add it
to the pythonrun.h API, and that is not correct on vms
system anyway (at least that's how I understand it...but I
have no vms system to check ;) ). Maybe a better approach
would be to cache the original PyOS_ReadlineFunctionPointer
in the readline module, and re-use it if stdin/stdout are
non-tty? This would be close to the approach I used for
implementing my MPI readline: cache
PyOS_ReadlineFunctionPointer and re-use it for modif...

If this is considerd unclean, the approach putting
PyOS_StdioReadline in the API can be kept, but It would have
to be renamed something like PyOS_ReadlineFunction and be
defined as PyOS_StdioReadline/vms__StdioReadline depending
on the environment...


Now for my patch on "Python/bltinmodule.c" to solve
the same issue with 'raw_input', I think it was necessary to
be able to use ipython in an interractive MPI session...Our
embedded python interpreter had the possibility to use this
(it gives a far nicier shell than the standard one), but I
think It uses raw_input as entry mechanism...Could you test
your patch to see if it work with this?

The only drawback I see with this is that it could slow down
a bit the parsing when using this function on a
non-interractive file...but I doubt this is significative
and could be optimized by keeping the call to PyOS_Readline
but stripping out the prompt treatment in this case...

We now are moving from an embbedded python interpreter to
using a standard python...which make the acceptance of the
patch more interresting: it would allows us to uses stock
python interpreters > 2.x, no need to distribute our own
interpretter anymore!!! :)
msg48571 - (view) Author: Gregory Lielens (greglielens) Date: 2005-08-06 19:35
Logged In: YES 
user_id=1044428

Oups: please forget my previous message, I clicked on submit
too fast and there is no correction/unsubmit option on
sourceforge...

Hi Lisandro, sorry that I did not follow this matter, we had
other short term focus in our company and I had trouble
finding time for it. 
Good that you subject this though, I have checked it fast
and from 
what I understand it would have the same effect as my older
patch...

However, I still prefer some aspects of my patch over your’s ;)

in Parser/myreadline.c, I removed the test about
PyOS_ReadlineFunctionPointer being null: instead I
initialize it at creation (to the correct value depending
to system, vms or not): 
This initialization was important because it allows me
 to use this default function in my parallel  Readline
function...

In general, ithe idea is to allow re-using the
default in the new Readline function, incrementing
functionalities instead of re-implementing full
functionality. 

You will have the same mechanism with the
inclusion of PyOS_StdioReadline in the pythonrun API, but I
personally find this solution less appealing: it increase
the size of the API, and you do not have the correct
readline on VMS...That's why I have done it like this, even
if it may seems hackish ;) Of course manual has to be
updated to say that PyOS_ReadlineFunctionPointer has a
meaningful default value...but it would also have to be
updated in your approach, to tell about PyOS_StdioReadline...

Well, a matter of taste I guess, but if the general opinion is 
that this initialization is unclean, I would then advice for
renaming 
of PyOS_StdioReadline in the API to something like 
PyOS_ReadlineFunction, that would defined as 
PyOS_StdioReadline/vms__StdioReadline depending
on the environment...

Regarding your modify of the Modules/readline.c, I have the
feeling that your approach is cleaner...because It seems
nicer to check for tty inside the new readline function,
instead of installing the new readline function only if
stdin/stdout are tty....You have to use  PyOS_StdioReadline
though, again that would not be accessible if one does not
add it
to the pythonrun.h API (again that is not correct on vms
system, but could be corrected taking my previous remark
into account). 

An alternative approach would be to cache the original 
PyOS_ReadlineFunctionPointer in the readline module, 
and re-use it if stdin/stdout are non-tty? 
This would be close to the approach I used for
implementing my MPI readline: cache
PyOS_ReadlineFunctionPointer and re-use it for modif...
Again, a matter of choice, I guess, more experienced python 
developers may have something to say about it.


Now for my patch on "Python/bltinmodule.c" to solve
the same issue with 'raw_input', I think it was necessary to
be able to use ipython in an interactive MPI session...Our
embedded python interpreter had the possibility to use this
(it gives a far nicer shell than the standard one), but I
think It uses raw_input as entry mechanism...
Could you test your patch to see if it work also with ipython? 
If not then this modify has to stay I think...

The only drawback I see with this is that it could slow down
a bit the parsing when using this function on a
non-interactive file...but I doubt this is significative
and could be optimized by keeping the call to PyOS_Readline
but stripping out the prompt treatment in this case...

We now are moving from an embedded python interpreter to
using a standard python...which make the acceptance of the
patch all the more interesting for us: it would allows to
use stock
python interpreters > 2.x, no need to distribute our own
interpreter anymore!!! :)
History
Date User Action Args
2022-04-11 14:56:12adminsetgithub: 42160
2005-07-04 18:03:29dalcinlcreate