Disable readline if stdin is not attached to a terminal #4495
Conversation
fingolfin
added
topic: kernel
release notes: not needed
|
SageMath certainly builds GAP with readline enabled. While we are phasing out pexpect interface for GAP in favour of libgap, it is not complete. I will need to verify whether this does not break things for us |
|
@dimpase I am sure you build it with readline enabled, but that's not the point. But you don't e.gy send "arrow-up enter" in order to execute the previous command again, do you? |
|
no, I don't think such contortions are done by Sage. Although perhaps users do it, one never knows (I would not worry about it, though). |
fingolfin
force-pushed
the
mh/no-tty-then-no-readling
branch
from
84561cd
to
3bcd162
Compare
|
@hulpke Thanks for trying. At first I was very confused by this, but I just had another look and I think I know what I missed... I've updated this PR, could you give it another try, please? |
fingolfin
force-pushed
the
mh/no-tty-then-no-readling
branch
from
3bcd162
to
994a3ec
Compare
Sure. Still, same ESC sequence remains and same error. (Since it is not my computer, I unfortunately cannot give you a login, but I'm happy to continue testing.) |
|
@hulpke thanks! This is rather vexing. OK, one more: could you just try compiling GAP with the configure option |
|
If I configure --without-readline all is fine (even in the master branch) |
|
Responding belatedly to ping: Yes, this should not affect XGAP or Gap.app, as the -p flag turns off readline. Anyway, both use openpty to establish communication. Doesn't that mark stdin as being a terminal? As far as the root problem: would adding the -E flag to the invocation of gap in the gac script fix it? (Probably this has already been tried...) |
|
@hulpke this is really weird, esp. since I can't reproduce it locally at all... I now think it may not be related to those settings in I just pushed another HACK; could you please build GAP from this commit, and see if (a) you see any of the |
|
Sorry, @fingolfin for all the trouble. I did:
Still the same extra characters.
Then calling bin/gap.sh gives while gap -E gives |
fingolfin
force-pushed
the
mh/no-tty-then-no-readling
branch
from
baec09a
to
add0781
Compare
|
@hulpke thanks for trying again. At this point I am out of ideas (perhaps @ChrisJefferson has one, though)? That said, I think this PR is useful on its own, as it really disables use of readline fully, while before we still called |
... by reverting a previous change that made us disable readline when stdin is not connected to a terminal (see gap-system#4495). While that still seems useful, and avoids certain issues when piping GAP output into files, it needs a better implication that avoids the issues described in gap-system#5014 Also add a separate CI test suite target "testworkspace" for testing for this issue, and potentially other workspace related issues.
... by reverting a previous change that made us disable readline when stdin is not connected to a terminal (see gap-system#4495). While that still seems useful, and avoids certain issues when piping GAP output into files, it needs a better implication that avoids the issues described in gap-system#5014 Also add a separate CI test suite target "testworkspace" for testing for this issue, and potentially other workspace related issues.
... by reverting a previous change that made us disable readline when stdin is not connected to a terminal (see #4495). While that still seems useful, and avoids certain issues when piping GAP output into files, it needs a better implication that avoids the issues described in #5014 Also add a separate CI test suite target "testworkspace" for testing for this issue, and potentially other workspace related issues.
This PR should fix #4466 and I hope @hulpke can verify that. It is not an alternative to PR #4494 (which also fixes that issue) but rather complements it: IMHO this PR here fixes the "real" issue; but that's debatable. In any case, ideally both would be merged.
I hope nobody is doing weird stuff that involves piping text into GAP (such that stdin is not a terminal) while also requiring GNU readline support to be enabled. Indeed, things like the pexpect interface by SageMath most likely would want to disable readline support anyway (perhaps @dimpase can confirm this). And the GUI frontends XGAP and GAP.app by @RussWoodroofe use special control codes, by enabling so called "window mode", which already disables GNU readline integration. So all in all I am hopeful this is safe, and does not even need to be mentioned in the release notes.