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

New shell integration not playing nicely with zsh plugins #4428

Closed
sQVe opened this issue Jan 4, 2022 · 31 comments · Fixed by #4437
Closed

New shell integration not playing nicely with zsh plugins #4428

sQVe opened this issue Jan 4, 2022 · 31 comments · Fixed by #4437
Labels

Comments

@sQVe
Copy link

sQVe commented Jan 4, 2022

Describe the bug

When running kitty with the new shell integrations some zsh plugins start to error maximum nested function level reached.

To Reproduce
Steps to reproduce the behavior:

  1. Use kitty @ 0.24.0.
  2. Make sure shell integration is enabled.
  3. Install zsh-autosuggestions.
  1. Error like maximum nested function level reached should be echoed on every new call.

Screenshots

image

Environment details

Key action failed

debug_config
(25, 'Inappropriate ioctl for device')
@sQVe sQVe added the bug label Jan 4, 2022
@kovidgoyal
Copy link
Owner

Works fine for me with ~/.zshrc

source "/usr/share/zsh/plugins/zsh-autosuggestions/zsh-autosuggestions.zsh"
return

and shell integration enabled, with the following steps

  1. Type ls -l and press enter
  2. type ls
  3. -l appears in gray as an autosuggestion

@sQVe
Copy link
Author

sQVe commented Jan 4, 2022

My bad. I loaded a default zimfw (input) plugin too that seems to be problematic. Sorry for the bad first report.

I'm able to consistently replicate it, with this minimal .zshrc:

source "$HOME/code/input/init.zsh"
source "$HOME/code/zsh-syntax-highlighting/zsh-syntax-highlighting.zsh"

Clone the repos at:

My assumption is that input some how clashes with the shell integration?

@kovidgoyal
Copy link
Owner

kovidgoyal commented Jan 4, 2022

This will likely be hitting some zsh internal limit or the other. I am not familiar enough with zsh to guess what, but offhand have you tried the suggestion in the error and increased FUNCNEST?

@kovidgoyal kovidgoyal reopened this Jan 4, 2022
@sQVe
Copy link
Author

sQVe commented Jan 4, 2022

@kovidgoyal Yes. I've tried increasing FUNCNEST incrementally til 1000, but by that point the terminal just crashes.

@ldelossa
Copy link

ldelossa commented Jan 4, 2022

Can confirm, editing FUNCEST does not help.

@kovidgoyal
Copy link
Owner

well I can see nothing wrong in either the kitty code or the zimfw code,
so most likely it is something in syntax highlighting, which is too big
for me to review.

@ldelossa
Copy link

ldelossa commented Jan 4, 2022

What change would occur between v0.23.1 to v0.24.0?

@sQVe
Copy link
Author

sQVe commented Jan 4, 2022

@kovidgoyal Sadly it seems that this problem isn't isolated to zsh-syntax-highlighting though. I can replicate it with:

source "$HOME/code/input/init.zsh"
source "$HOME/code/zsh-autosuggestions/zsh-autosuggestions.zsh"
source "$HOME/code/zsh-history-substring-search/zsh-history-substring-search.zsh"

Relevant repos:

What makes this worse is that the repos involved here are extremely common within the zsh community - so this breaking behavior will be very noticable.

@sQVe
Copy link
Author

sQVe commented Jan 4, 2022

@ldelossa You should read the changelog. Version 0.24.0 introduced an awesome shell intregration - which will be a great feature once issues are ironed out.

@kovidgoyal
Copy link
Owner

somebody will need to do a deep dive on those plugins to see what it is they are doing, I really dont have the time for that, at least for now. But maybe later.

@sQVe
Copy link
Author

sQVe commented Jan 4, 2022

I tried commenting out blocks of code and have so far found that I'm not seeing any errors after commenting out:

https://github.com/kovidgoyal/kitty/blob/master/shell-integration/zsh/kitty-integration#L235

@sQVe
Copy link
Author

sQVe commented Jan 4, 2022

Removing and commenting out https://github.com/kovidgoyal/kitty/blob/master/shell-integration/zsh/kitty-integration#L204-L205 obviously fixes it too.

@ldelossa
Copy link

ldelossa commented Jan 4, 2022

@ldelossa You should read the changelog. Version 0.24.0 introduced an awesome shell intregration - which will be a great feature once issues are ironed out.

Ah, I see. Yes feature looks very cool, but terminal breaking my zsh env is a big problem. Will stick to v0.23.1 until fix. Thanks for new features Kovid, they do look aweome!

@kovidgoyal
Copy link
Owner

I tried commenting out blocks of code and have so far found that I'm not seeing any errors after commenting out:

https://github.com/kovidgoyal/kitty/blob/master/shell-integration/zsh/kitty-integration#L235

yes but there isnt anything actually wrong with that line. Its something the plugins are doing to zle-add-widget

@kovidgoyal
Copy link
Owner

Ah, I think I know the issue.

@ldelossa
Copy link

ldelossa commented Jan 4, 2022

@kovidgoyal I just built from master, doesn't seem to be fixed for me.

@page-down
Copy link
Contributor

page-down commented Jan 4, 2022

I tried the three plugins mentioned above and still have problems. kitty --config=NONE -o shell=zsh --hold

I tried disabling shell integration and the kitty window was still closed instantly after startup.

kitty --config=NONE -o shell_integration=disabled -o shell=zsh --hold

I'm more curious as to why --hold doesn't seem to work.

@kovidgoyal
Copy link
Owner

Yeah with all three as opposed to just two its not fixed, sadly.

@kovidgoyal kovidgoyal reopened this Jan 4, 2022
@page-down
Copy link
Contributor

Is it possible that --hold is not working:

  • The escape code was sent during the execution of zsh rc, and then it exited with an error.
  • At this point, the kitty window is waiting for any character to be entered.
  • But the kitty itself sends another character to the window, causing --hold to appear not to work at all.

Because it was closed by kitty itself.

@kovidgoyal
Copy link
Owner

kovidgoyal commented Jan 4, 2022

--hold works fine

kitty --hold false

what other character is kitty sending to the window?? You mean something in response to an escape code? You can check by modifying the hold() function in __main__.py

@page-down
Copy link
Contributor

page-down commented Jan 4, 2022

You mean something in response to an escape code?

Yes.

I mean when used with the problematic zsh rc. The program sends an escape code, then exits immediately due to an error, and the escape code returned by kitty is sent to the window and causes it (kitty -o shell=zsh --hold) to close.

I checked and confirmed that stdin received the data here, then exited.

def hold(args: List[str]) -> None:
    # ...
    with suppress(BaseException):
        # ...
        sys.stdin.buffer.read(1)
source /path/to/zimfw-input/init.zsh
source /path/to/zsh-history-substring-search.zsh

@sQVe
Copy link
Author

sQVe commented Jan 4, 2022

I just built from master and it's now working great on my setup.

@page-down
Copy link
Contributor

I just built from master and it's now working great on my setup.

Do you mean that all three plugins mentioned above are working correctly? I can only run zsh if I comment the following lines in the plugin.

    # if is-at-least 5.3; then
    #   autoload -Uz add-zle-hook-widget && \
    #       add-zle-hook-widget -Uz line-init start-application-mode && \
    #       add-zle-hook-widget -Uz line-finish stop-application-mode
    # else
      zle -N zle-line-init start-application-mode
      zle -N zle-line-finish stop-application-mode
    # fi

@sQVe
Copy link
Author

sQVe commented Jan 4, 2022

Everything seems to be working fine on my setup with 69482b6.

@sQVe
Copy link
Author

sQVe commented Jan 4, 2022

Ah, but I have shell_integration no-cursor set. If I remove that, and fallback to the default, I'm getting errors again.

@kovidgoyal
Copy link
Owner

Basically, the syntax/autosuggest/substring search plugins all rewrap all zle widgets. And that rewrap logic is broken by both zimfw and kitty calling add-zle-hook-widget. If both calls are present, then the rewrap logic creates an infinite recursion. If either one of those calls is removed, there is no infinite recursion.

I dont know of a good way to fix this sorry. I guess you have to use either zimfw or kitty shell integration. Someday I might read that rewrap logic and try to figure out a workaround, but frankly shell script makes my eyes bleed.

The only thing I can think of is to not dynamically create the functions used for zle widgets and instead set the at load time just once. This means a bit of a performance loss, but given the craziness that is the zsh ecosystem, maybe its the only way. Perhaps @romkatv can comment, since this is his code.

@kovidgoyal
Copy link
Owner

And actually setting them at load time wont work since various other plugins clear the zle hooks on initialization, le sigh. So yeah totally out of ideas here.

@romkatv
Copy link
Contributor

romkatv commented Jan 4, 2022

add-zle-hook-widget is broken in a rather terrible way. If at least one plugin uses it, then all must use it or risk this sort of meltdown. There is a workaround though. I'll send you a PR.

P.S.

I really should've at least flagged the use of add-zle-hook-widget when I was rewriting zsh integration code in Kitty. Or, better yet, implemented the workaround.

@kovidgoyal
Copy link
Owner

Cool, thanks.

@romkatv
Copy link
Contributor

romkatv commented Jan 4, 2022

Sent #4437.

@ldelossa
Copy link

ldelossa commented Jan 5, 2022

Everything is working. Thanks a lot. The shell integration features are HUGELY helpful, and something I've been looking for, for a very long time. 🚀

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

Successfully merging a pull request may close this issue.

5 participants