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

Fixes for modern readline #120

Merged
merged 4 commits into from
Aug 22, 2022
Merged

Fixes for modern readline #120

merged 4 commits into from
Aug 22, 2022

Conversation

kdm9
Copy link
Collaborator

@kdm9 kdm9 commented May 24, 2022

This will automatically disable bracketed paste, which seems to break bash_kernel. I have verified that this fixes the problem with the latest released version of bash_kernel.

It should address issues described in #117, #116, & #115 (which are all the same AFAICT).

Additionally, I've added code to set the TERM_PROGRAM and TERM_PROGRAM_VERSION env vars so others can use this to perform customisation of bashrc as needed. This might not be ideal, as perhaps bashrc has executed before these are set. Please advise if that's the case. (removed this)

To use this code:

python3 -m pip uninstall bash_kernel
python3 -m pip install -U git+https://github.com/kdm9/bash_kernel.git@patch-1

kdm9 added 2 commits May 25, 2022 04:17
Should address issues described in takluyver#117 et al. 

Additionally, I've added code to set the TERM_PROGRAM and TERM_PROGRAM_VERSION env vars so others can set variables accordingly. This *might* not be ideal, as perhaps bashrc has executed before these are set. Please advise if that's the case.
@kdm9
Copy link
Collaborator Author

kdm9 commented May 24, 2022

Quite likely fixes #119's problem, and also #107 too

It's a dependency, and bash_kernel won't install from src without this
change.
@hcz
Copy link

hcz commented Jun 2, 2022

I've tested this code in my environment but it freezes bash execution.
Env details:

$ jupyter-lab --version
3.2.5
$ python --version
Python 3.9.5
$ uname -a
Linux my-ubuntu 5.13.0-41-generic #46-Ubuntu SMP Thu Apr 14 20:06:04 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
$ bash --version
GNU bash, version 5.1.8(1)-release (x86_64-pc-linux-gnu)

So it didn't help to fix #107 to me

@kdm9
Copy link
Collaborator Author

kdm9 commented Jun 2, 2022

@hcz Thanks for testing. That's strange, as I get no hang here across a quite wide diversity of linuxes and bashes. does manually running bind 'set enable-bracketed-paste off' hang for you, either in Jupyter or outside it?

@@ -105,9 +105,14 @@ def _start_bash(self):
finally:
signal.signal(signal.SIGINT, sig)

# Disable bracketed paste (see <https://github.com/takluyver/bash_kernel/issues/117>)
self.bashwrapper.run_command("bind 'set enable-bracketed-paste off' >/dev/null 2>&1 || true")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hcz and, do you get any output if you redirect this to some file rather than /dev/null? If you edit this file in a testing venv/conda env, perhaps try commenting out the line below (TERM_PROGRAM=...) to see if that's the cause.

@hcz
Copy link

hcz commented Jun 3, 2022

@kdm9 I've tried separately solution from the issue #107 : bind 'set enable-bracketed-paste off'

It worked.

@kdm9
Copy link
Collaborator Author

kdm9 commented Jun 3, 2022

@hcz how about the latest commit? does that hang still? (run python3 -m pip install -U git+https://github.com/kdm9/bash_kernel.git@patch-1 again)

I removed the part about TERM_PROGRAM, so this now should be identical to manually running bind 'set enable-bracketed-paste off'.

@hcz
Copy link

hcz commented Jun 3, 2022

@kdm9 thanks for the try, but unfortunately it doesn't hang and doesn't work

It looks like bind isn't applied immediately:
Screenshot from 2022-06-03 17-47-58

@kdm9
Copy link
Collaborator Author

kdm9 commented Jun 3, 2022

@hcz thanks for testing! Now I'm really confused :)

  1. Can you confirm that your venv/conda env's bash_kernel/kernel.py file has this line in it? (i.e there was no installation SNAFU that meant you got e.g. the main branch or the released version)
  2. Can you confirm that it no longer hangs without the line that sets TERM_PROGRAM? (in which case I'll keep the TERM_PROGRAM part excluded)

Unless anything obvious is wrong in 1. above, I'll have to defer to @takluyver as to why kernel.run_command("XXX") behaves differently from running the identical command in the first cell.

Best,
Kevin

@hcz
Copy link

hcz commented Jun 3, 2022

@kdm9

  1. There was no that line, really sorry.
    I missed the first necessary step: bash_kernel uninstall.
    I've tested your latest commits: a4c81c9 and d6a048b
    Both revisions worked really nice and fixed these problems:

    • ugly red alerts disappeared, exit codes became fine
    • bracketed input was disabled, additional symbol sequences disappeared
  2. I confirm.
    Looks like it was lags of my local environment

@kdm9
Copy link
Collaborator Author

kdm9 commented Jun 3, 2022

@hcz Wonderful news! Thanks so much for testing, i'm glad it fixes it too. I'll amend the instructions above to ensure folks uninstall it if they want to test/use this version until @takluyver has time to review/merge.

@kdm9
Copy link
Collaborator Author

kdm9 commented Jul 6, 2022

@takluyver a friendly ping about this PR. I'm teaching a workshop using jupyter & bash_kernel soon, and I'd much prefer the instructions to be pip install bash_kernel than the hack above to install this fix from my dev branch. Could you please review/merge soon?

If you need help maintaining bash_kernel, I'd be happy to step in as a co-maintainer, even if temporarily until you have more time. If so, just add me to the repo.

@andyneff
Copy link

andyneff commented Aug 18, 2022

As a cheap simple workaround, I found just unsetting my TERM when calling jupyter lab gets around the issue somehow:

TERM= jupyter lab ...

@takluyver
Copy link
Owner

Thanks @kdm9 et al for diagnosing and figuring out a fix for this, and sorry it's taken me so long to look at it.

I think the fix basically looks good, and I'll merge it. If I was being picky, I might suggest that this belongs 'upstream' in pexpect (this file tries to set bash up for being wrapped in a programmatic interface), but there's a working fix ready here, and I'd probably also need to review the Pexpect PR (though there are other maintainers there), so let's not hold this up for that.

Further in the future, I wonder if bash_kernel (or the pexpect replwrap API it uses) could make use of bracketed paste, rather than disabling it. Currently, if you have more than one line in a cell, bash_kernel sends them to bash one at a time, waiting for the prompt after each one. With bracketed paste, it should be possible to send the whole cell in one go. On the other hand, it probably doesn't simplify the code in replwrap unless we can assume that every REPL to be wrapped supports bracketed paste. 🤔

@takluyver
Copy link
Owner

I've released version 0.8 with this change - hopefully that fixes a lot of the issues people have been seeing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants