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

plugins: Add ble.sh plugin #1884

Merged
merged 1 commit into from
Oct 6, 2021
Merged

Conversation

NoahGorny
Copy link
Member

Description

Added a new plugin, which loads ble.sh
I ping @akinomyoga here, in case we should do more things in here except loading ble.sh plainly

Motivation and Context

ble.sh is a really interesting project, and seems like a nice addition as a Bash-it plugin 😄

How Has This Been Tested?

Locally

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
  • I have added tests to cover my changes, and all the new and existing tests pass.

@akinomyoga
Copy link
Contributor

I ping @akinomyoga here, in case we should do more things in here except loading ble.sh plainly

Sorry for the late reply! I have a question. Is ble.sh going to be loaded in the following source chain?

  • source bash_it.sh
    • source scripts/reloader.bash
      • source enabled/blesh.plugin.bash
        • source ~/.local/share/blesh/ble.sh

If so, there should be no problem unless some other plugins loaded after ble.sh erase the existing contents of PROMPT_COMMAND.

@NoahGorny
Copy link
Member Author

PROMPT_COMMAND

We touch PROMPT_COMMAND from the theme.. My PROMPT_COMMAND seems unrelated to ble.sh, yet I do not encounter any problems

@akinomyoga
Copy link
Contributor

akinomyoga commented May 23, 2021

PROMPT_COMMAND

We touch PROMPT_COMMAND from the theme.. My PROMPT_COMMAND seems unrelated to ble.sh, yet I do not encounter any problems

Maybe you are using Bash 5.1? In Bash 5.1, PROMPT_COMMAND has been escalated to an array variable, so ble.sh tries to register the attaching code in PROMPT_COMMAND[i] (where i >= 1) in Bash 5.1 and later. You cannot see ble.sh-related PROMPT_COMMAND entry after ble.sh attached because ble.sh immediately removes the entry from PROMPT_COMMAND when the attaching is successful. If you want to see what ble.sh is doing, you may put declare -p PROMPT_COMMAND >&2 at the end of .bashrc.

Edit: For Bash 5.0 and lower, the attaching code of ble.sh in PROMPT_COMMAND needs to be preserved after other plugins have also registered something to PROMPT_COMMAND. In the case of themes/base.theme.bash, it preserves the original contents of PROMPT_COMMAND in the following way so doesn't make a problem.

PROMPT_COMMAND="${1};${PROMPT_COMMAND}"

ble.sh will not attach when some other plugin has directly assigned its value to PROMPT_COMMAND like

PROMPT_COMMAND=something   # original value of PROMPT_COMMAND is lost

@NoahGorny
Copy link
Member Author

declare -p PROMPT_COMMAND >&2

Hmm, in any case, it seems to work just fine on my end.
I will let other folks take a look, and merge it in order to let more people know and use ble.sh 😄
Thanks for all the help @akinomyoga

@cornfeedhobo
Copy link
Member

You cannot see ble.sh-related PROMPT_COMMAND entry after ble.sh attached because ble.sh immediately removes the entry from PROMPT_COMMAND when the attaching is successful. If you want to see what ble.sh is doing, you may put declare -p PROMPT_COMMAND >&2 at the end of .bashrc.

I think this might be a show stopper, but maybe not...

We embed preexec which makes very clear that it expects absolute control over PROMPT_COMMAND, which it munges upon loading, which is why we do it last.

@akinomyoga
Copy link
Contributor

akinomyoga commented May 24, 2021

You cannot see ble.sh-related PROMPT_COMMAND entry after ble.sh attached because ble.sh immediately removes the entry from PROMPT_COMMAND when the attaching is successful. If you want to see what ble.sh is doing, you may put declare -p PROMPT_COMMAND >&2 at the end of .bashrc.

I think this might be a show stopper, but maybe not...

We embed preexec which makes very clear that it expects absolute control over PROMPT_COMMAND, which it munges upon loading, which is why we do it last.

Could you describe what kind of problem you are thinking of? The above description is the behavior of ble.sh for Bash 5.1+. As I have written in the previous sentence, ble.sh only uses PROMPT_COMMAND[i] with the index i greater or equal to 1 while bash-reexec.sh only uses the PROMPT_COMMAND which is equivalent to PROMPT_COMMAND[0]. To clarify further, ble.sh removes the entry only when the entry isn't modified by other codes. This is also the same for Bash 5.0 and lower for which ble.sh takes a similar approach with bash-preexec but immediately restores the value of PROMPT_COMMAND when it isn't modified by other plugins.

@cornfeedhobo
Copy link
Member

@akinomyoga okay. thanks for the explanation. I'll try to find some time to test this locally soon.

@NoahGorny
Copy link
Member Author

@cornfeedhobo @davidpfarrell gentle ping 😄

@NoahGorny
Copy link
Member Author

@cornfeedhobo if you have time today, I'll be glad if you could take a look at this PR

@cornfeedhobo
Copy link
Member

@NoahGorny Okay, I tested this fresh today. This broke several things, like the history search plugins and most importantly the built-in pre-exec library.

Without ble.sh:

$ echo $PROMPT_COMMAND
__bp_precmd_invoke_cmd __promptline;_fasd_prompt_func;_direnv_hook __bp_interactive_mode

With ble.sh:

$ echo $PROMPT_COMMAND
__promptline;_fasd_prompt_func;_direnv_hook;ble/function#lambda/0

Here is a sample of the error output upon loading a new shell:

ble.sh (bind): unsupported readline function 'history-search-backward' for keymap 'vi_nmap'.
ble.sh (bind): unsupported readline function 'history-search-forward' for keymap 'vi_nmap'.
ble.sh (bind): unsupported readline function 'set-mark' for keymap 'vi_nmap'.
ble.sh (bind): unsupported readline function 'set-mark' for keymap 'vi_nmap'.

It might be worth noting that I don't use the vi input method.


I guess in summary I think this is so beta that we shouldn't include it. The install instructions require so many custom steps that it seems perfectly fine to have the user add a source directive to their shell if they want to play with this.

@akinomyoga
Copy link
Contributor

akinomyoga commented Sep 20, 2021

and most importantly the built-in pre-exec library.

Without ble.sh:

$ echo $PROMPT_COMMAND
__bp_precmd_invoke_cmd __promptline;_fasd_prompt_func;_direnv_hook __bp_interactive_mode

With ble.sh:

$ echo $PROMPT_COMMAND
__promptline;_fasd_prompt_func;_direnv_hook;ble/function#lambda/0

@cornfeedhobo Hi, I'm the author of ble.sh. Actually the above rewrite of PROMPT_COMMAND is expected one, and the original calls of __bp_precmd_invoke_cmd and __bp_interactive_mode are supposed to be processed within ble/function#lambda/0. Did you really checked that __bp_precmd_invoke_cmd and __bp_interactive_mode are not processed?

If they are really unprocessed, there should be some problem in ble.sh that I have never experienced, so I'd like to ask the information on how to reproduce it. I just tried bash-it with the add-blesh-plugin branch at NoahGorny/bash-it.git and enabled blesh, but couldn't reproduce the above PROMPT_COMMAND.

Another thing is that ble.sh provides the first-class support for PRECMD, PREEXEC, and POSTEXEC through blehook, so I recommend using them when ble.sh is enabled rather than hacking PROMPT_COMMAND, DEBUG, and PS0 in unintended ways as bash-preexec does.

Here is a sample of the error output upon loading a new shell:

ble.sh (bind): unsupported readline function 'history-search-backward' for keymap 'vi_nmap'.
ble.sh (bind): unsupported readline function 'history-search-forward' for keymap 'vi_nmap'.
ble.sh (bind): unsupported readline function 'set-mark' for keymap 'vi_nmap'.
ble.sh (bind): unsupported readline function 'set-mark' for keymap 'vi_nmap'.

It might be worth noting that I don't use the vi input method.


I guess in summary I think this is so beta that we shouldn't include it.

I first need to clarify that ble.sh is an independent implementation of a line editor, and its goal is not the 100% emulation of Readline, but a better implementation. The reason that the above combination of readline functions and vi_nmap are not supported in ble.sh is not because ble.sh is beta but because I thought it is non-sense to bind set-mark and history-search-{for,back}ward in nmap. [ Note: ble.sh supports history-search-{for,back}ward and set-mark in emacs and vi_imap keymaps where I think it is appropriate. ] The original vi/vim doesn't have such operations but instead provides different ways to achieve the same purposes.

  • In Readline, set-mark is used, for example, for the emacs-like selection. On the other hand, the original vim provides the selection through different keymaps xmap (VISUAL mode) and smap (SELECT mode). These two different mechanisms of selection (mark vs xmap/smap) cannot be activated at the same time. ble.sh implements xmap/smap so cannot simply provide set-mark in nmap whereas Readline doesn't support the vim modes (xmap/smap) so instead provides set-mark as an Emacs-style selection.
  • Likewise, vi/vim uses ? and / for string searches instead of history-search-{for,back}ward. The operations ? and / are supported through ble widgets vi-command/search-{for,back}ward in ble.sh. So, I initially thought we don't need history-search-{for,back}ward in nmap, but they actually don't conflict with the existing features, so I may consider implementing them in nmap. Something that should be carefully considered before the implementation is how these history-search-{for,back}ward interact with . command and `[, `] registers.

The install instructions require so many custom steps

Could you explain in detail what this means?

it seems perfectly fine to have the user add a source directive to their shell if they want to play with this.

I kind of agree with this. Essentially, this PR merely adds the single line source /path/to/ble.sh (with the /path/to part fixed to be ~/.local/share/blesh). Unless bash-it provides some custom default configuration for ~/.blerc (or ~/.config/blesh/init.sh), I'm not sure if it's worth adding this single line "source ble.sh" as "a bash-it plugin".

Also, a more recommended way of setting ble.sh up in bashrc is actually the combination of source ble.sh --attach=none (around the beginning of bashrc) and ble-attach (at the end of bashrc).

@cornfeedhobo
Copy link
Member

cornfeedhobo commented Sep 20, 2021

The install instructions require so many custom steps

Could you explain in detail what this means?

@akinomyoga To be compatible with this PR, the install steps are something like:

$ git clone --recursive https://github.com/akinomyoga/ble.sh.git
$ make -C ble.sh
$ mv ble.sh/out ~/.local/share/blesh

@akinomyoga
Copy link
Contributor

akinomyoga commented Sep 20, 2021

@cornfeedhobo Oh, thank you for the clarification! You can actually simply run

$ git clone --recursive https://github.com/akinomyoga/ble.sh.git
$ make -C ble.sh install

The default install location is ${XDG_DATA_HOME:-$HOME/.local/share}/blesh. The details are described here. Edit: I guess the location of ble.sh is fixed to ~/.local/share/blesh in this PR just because that's the default of make install.

I have looked at the other plugins, but most of the plugins require additional installation by users. Is it a requirement for any bash-it plugin that it needs to be installed by a one-liner?

@cornfeedhobo
Copy link
Member

@akinomyoga Nice to meet you :)

I have looked at the other plugins, but most of the plugins require additional installation by users. Is it a requirement for any bash-it plugin that it needs to be installed by a one-liner?

No, they don't have to be a one-liner, but afaik most of the plugins we have can be installed by the various package managers out there. I was simply emphasizing that is a custom install, and leaving it to the user seems like not much more of a stretch once they are running make.

The reason that the above combination of readline functions and vi_nmap are not supported in ble.sh is not because ble.sh is beta but because I thought it is non-sense to bind set-mark and history-search-{for,back}ward in nmap.

I think some parts of my previous comment were mistaken. I was not speaking to the reasoning for your bindings. All I know is that I have a common plugin enabled that sets a readline setting, and this outputs a lot of error messages about it when starting the shell. I even disabled every plugin, completion, and alias, and disabled my theme. It was still very broken.

Maybe the word "beta" was too loaded, and my apologies for that. However, my stance is still that we work hard to make plugins work smoothly with all the other combinations of plugins we support. This isn't 100% possible in all cases, but I feel like this plugin invites a lot more trouble than it's worth.

Another thing is that ble.sh provides the first-class support for PRECMD, PREEXEC, and POSTEXEC through blehook, so I recommend using them when ble.sh is enabled rather than hacking PROMPT_COMMAND, DEBUG, and PS0 in unintended ways as bash-preexec does.

This sounds awesome, but bash-it is already tightly coupled to preexec and we consider it as part of the "core". I think that is a much larger discussion to be had.

If you would like to chat about this further, or maybe setup a screen share, I'm available on IRC.


@NoahGorny I'm not blocking this. This is a super cool project but I've cast my vote. I leave the decision to the rest of the team.

@cornfeedhobo
Copy link
Member

cornfeedhobo commented Sep 20, 2021

@akinomyoga Last follow up that I'll leave here: I disabled every plugin, completion, alias, and my theme, as well as stripped down my bashrc to a bare minimum. I still got a lot of errors when loading ble.sh. I don't run a very customized distro (suse leap 15.2), so I imagine someone should be able to reproduce this. If you reach out over IRC, I'll do my best to assist in reproducing this.

@akinomyoga
Copy link
Contributor

akinomyoga commented Sep 21, 2021

bash-preexec & ble.sh

I cannot still reproduce the bash-preexec problem. So far, everything seems to work correctly with my environment. @cornfeedhobo and I have been discussed the issue in IRC, but @cornfeedhobo seems to be busy and there is no reply now. Unfortunately, it seems the information I wrote there seems to have been lost before @cornfeedhobo's reply, so I here summarize the current situation.

As far as I remember, @cornfeedhobo's environment is bash-it (commit is unknown), ble.sh current master, bash-4.4, openSUSE 15.2. @cornfeedhobo tested with the minimal setup (I'm not sure the exact setup) of bash-it. @cornfeedhobo described preexec is broken with ble.sh, but I don't know the details on how it is broken.

In my environment, everything seems to work correctly with the blesh plugin with the minimal bash-it setup: I tried with my openSUSE Tumbleweed setup in VirtualBox where the Bash version is 5.1. I'm using the latest commit of branch master of ble.sh. I tried branch add-blesh-plugin in NoahGorny/bash-it, and also tried the commit rebased on top of the current master of Bash-it/bash-it. My minimal .bashrc contains essentially only two lines of export BASH_IT=... and source $BASH_IT/bash_it.sh. The directory bash-it/enabled/ only contains a file 250---blesh.plugin.bash. The result of echo "$PROMPT_COMMAND" contains the following two lines as expected ([RET] means pressing Enter or Return).

$ echo "$PROMPT_COMMAND"[RET]
__bp_precmd_invoke_cmd
__bp_interactive_mode

If I define functions precmd and preexec, both are called normally.

$ preexec() { echo $FUNCNAME; }[RET]
$ precmd() { echo $FUNCNAME; }[RET]
precmd
$ echo hello[RET]
preexec
hello
precmd

Though, there is a slight behavior difference on empty lines.

$ [RET]precmd

I also tried the same setup in Fedora with different versions of Bash including bash-5.0, 4.4, and 4.3, but the behaviors were all the same.

openSUSE /etc/inputrc & ble.sh (1) error messages

Here are other discussions that has been made on the IRC.

The other issue of error messages reported by @cornfeedhobo has turned out to be the same issue as reported at akinomyoga/ble.sh#89. I have already reported it at openSUSE/aaa_base#84
and has been fixed in openSUSE Tumbleweed (openSUSE of the rolling-release version). However, according to https://pkgs.org/download/aaa_base, the base configurations of the other openSUSE releases, 15.2 and 15.3, seem to be still based on the version of 20180409, so the fix is not yet available in these releases.

openSUSE /etc/inputrc & ble.sh (2) [home] and [end]

There was another issue by the interference of openSUSE /etc/inputrc and ble.sh: home and end doesn't work with ble.sh in openSUSE. This is caused by /etc/inputrc.keys:233..234 where openSUSE tries to bind history-search-backward (search) and set-mark (start selection) to find and select when TERM=xterm. The contemporary keybaords doesn't seem to have find and select, but DEC keyboards seems to have had find and select instead of home and end in 3x2 function keys above the arrow keys (image).

ble.sh has been treating find and select the same as home and end, respectively, because some terminal emulators treat them the same, and also because the support for find and select cannot be obtained from terminfo database (that doesn't have the specialized entries for these special keys). I'm now thinking of differentiating these keys find/select vs home/end by directly referencing TERM environment variable.

openSUSE /etc/inputrc & ble.sh (3) Slow initialization

Since openSUSE /etc/inputrc and /etc/inputrc.keys define so many key sequences (which I think are hardly used), the initialization of ble.sh, which analyzes the user-defined keybindings on initialization, becomes very slow. This can be evaded by disabling the initial analysis on user-defined keybindings by passing the --noinputrc flag to source ble.sh:

source /path/to/ble.sh --noinputrc

but this also disables the user settings in ~/.inputrc in ble.sh. Maybe I should consider caching the analysis results (because I expect .inputrc files are not modified so frequently).

history-search-{for,back}ward and set-mark in vi_nmap

I'm now thinking of implementing them with behavior modifications so that they can be naturally used in vi_nmap without problems.

@akinomyoga
Copy link
Contributor

akinomyoga commented Sep 21, 2021

Re: bash-preexec & ble.sh

I think I found an issue though I'm not sure if this is the issue @cornfeedhobo reported. This only happens when source ble.sh is manually executed after source bash_it.sh in ~/bashrc. When ble.sh is loaded through blesh.plugin.bash, there is no problem (at least with the minimal bash-it configuration).

The problem is that preexec and precmd are redundantly called as many times as the invocation of PROMPT_COMMAND so far. A minimal reproducer of the behavior is as follows:

# bashrc

source bash-preexec/bash-preexec.sh
preexec() { echo preexec; }
precmd() { echo precmd; }

_my_save_prompt_command=$PROMPT_COMMAND
PROMPT_COMMAND=_my_prompt_command
_my_prompt_command() {
  local PROMPT_COMMAND=$_my_save_prompt_command
  eval -- "$PROMPT_COMMAND"
  : do something
}

bash-preexec initially sets a temporary value of PROMPT_COMMAND, which tries to rewrite the PROMPT_COMMAND in the first call of PROMPT_COMMAND to add preexec/precmd settings. However, when the value of PROMPT_COMMAND is saved by another plugin, the temporary value of PROMPT_COMMAND is preserved and preexec/precmd settings are added every time PROMPT_COMMAND is evaluated.

I would later think whether I can work around this.

@akinomyoga
Copy link
Contributor

@cornfeedhobo I added related fixes in ble.sh

  • akinomyoga/ble.sh@39ebf53 (for bash-preexec) add a workaround for __bp_install (bash-preexec).
  • akinomyoga/ble.sh@c4d28f4 (for openSUSE) find and select are now distinct keys with TERM=xterm, etc.
  • akinomyoga/ble.sh@9a7c8b1 (for openSUSE) support set-mark and history-search in vi_nmap. The behaviors are different from those in Readline so that it suits the vi_nmap user experience. [ Note: There are still many unsupported readline functions (which are marked with "-" in lib/core-decode.vi_nmap-rlfunc.txt), but currently I don't have a plan to implement all of them unless there are future requests. ]
  • akinomyoga/ble.sh@994e2a5 (for openSUSE) Now the translation result of inputrc is cached.

Copy link
Contributor

@akinomyoga akinomyoga left a comment

Choose a reason for hiding this comment

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

@NoahGorny I leave several comments and suggestions in the PR.

I'm not good at the criterion for bash-it plugins, but this plugin seems to be essentially just source ble.sh. I'm not sure if it's worth making this single command an independent plugin. Maybe one benefit of making this a plugin is that a user can look up blesh from the list bash-it show plugins and can enable/disable it by bash-it enable/disable plugin blesh without directly editing .bashrc.

In this plugin, the install location of ble.sh is assumed to be a fixed location, ~/.local/share/blesh. However, ble.sh might be installed on a different place. For example, when one installed ble.sh from AUR (Arch User Repository) using AUR helpers, the install location is /usr/share/blesh. Shouldn't the install location be configurable by users? Or maybe an appropriate ble.sh path should be automatically detected and selected? Or, maybe this plugin can automatically download and install ble.sh on ~/.local/share/blesh when it doesn't find ble.sh in predefined locations.

plugins/available/blesh.plugin.bash Outdated Show resolved Hide resolved
plugins/available/blesh.plugin.bash Outdated Show resolved Hide resolved
plugins/available/blesh.plugin.bash Outdated Show resolved Hide resolved
@cornfeedhobo
Copy link
Member

@NoahGorny I just tested this again. It looks like this does play well with preexec now. I agree with @akinomyoga's comments above, and generally approve this PR now. If you want to make their requested changes, ping me for a re-review and I'll jump on it quickly.

@akinomyoga Amazing work here, with great attention to detail. Thank you for hunting down these bugs and even helping me with my environment. I look forward to playing more with ble.sh.

@akinomyoga
Copy link
Contributor

@cornfeedhobo Thank you for checking the updated ble.sh! I have added another workaround for openSUSE 15.{2,3}.

  • akinomyoga/ble.sh@e5b0c86 Now ble.sh checks whether /etc/inputrc.keys contains suspicious keysequences. When ble.sh detects the broken /etc/inputrc.keys, it skips the analysis of the readline state of keybindings and instead directly reads ~/.inputrc.

I look forward to playing more with ble.sh.

Thank you! If you find anything suspicious, please feel free to open issues!

@NoahGorny NoahGorny force-pushed the add-blesh-plugin branch 2 times, most recently from 05f0672 to 6d8a072 Compare September 28, 2021 12:35
@NoahGorny
Copy link
Member Author

see now @akinomyoga and @cornfeedhobo 😄

Copy link
Contributor

@akinomyoga akinomyoga left a comment

Choose a reason for hiding this comment

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

I'm not sure about the naming convention of the global variables used by bash-it, but I feel we can use some prefix for namespacing. /bash_it.sh uses BASH_IT_* and _bash_it_*.

plugins/available/blesh.plugin.bash Outdated Show resolved Hide resolved
plugins/available/blesh.plugin.bash Show resolved Hide resolved
@NoahGorny
Copy link
Member Author

I will add a note about it in the next release- ble.sh is awesome!

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.

3 participants