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

Bash completion install doc improvements #3177

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

scop
Copy link
Contributor

@scop scop commented Jan 22, 2023

See individual commits for details.

@rbtcollins
Copy link
Contributor

I've never looked particularly closely at completions. What does bash do to avoid running that command on every completion request?

@scop
Copy link
Contributor Author

scop commented Jan 23, 2023

What does bash do to avoid running that command on every completion request?

https://www.gnu.org/software/bash/manual/html_node/Programmable-Completion.html has the background details. There's no anchor to link to on that page, but search for "There is some support for dynamically modifying completions" towards the end of it.

More specifically for this particular case, the https://github.com/scop/bash-completion project that I co-maintain is "in charge" of loading completions dynamically from the mentioned .../bash-completion/completions dirs (system and user wide), contains a completion loader that implements the above: https://github.com/scop/bash-completion/blob/fd7eadc541ef15fdb897911613d6939bcc1bb742/bash_completion#L2713-L2724

@scop scop force-pushed the bash-completion-install branch from ccd0f0e to 129f159 Compare November 18, 2023 18:25
@scop
Copy link
Contributor Author

scop commented Nov 18, 2023

Rebased/resolved conflicts.

@rami3l
Copy link
Member

rami3l commented Nov 25, 2023

@scop In order to make the lights green again, you still have to update the UI test to match the new output. Please have a look at files in tests/suite/cli-ui/ and change them accordingly (like what was done in e.g. #3542).

@scop scop force-pushed the bash-completion-install branch from 129f159 to 1892727 Compare November 26, 2023 17:10
scop added 3 commits November 26, 2023 19:25
…etions

/etc/bash_completion.d works too, but the downside is that everything
there is loaded eagerly at bash_completion load time, slowing down shell
startup.

From /usr/share/bash-completion/completions completion files are loaded
on demand. This mechanism is available as of bash-completion version 2
and later, just like the ~/.local/share/bash-completion/completions one
that is already suggested for user-specific completions.
It's not that much about whether a command is system-wide or
user-specific, but more about whether the completion is.

In particular, the user-specific completions file dir is used to
override system-wide completion, or to add completion for a command that
does not have a system-wide completion installed. This is orthogonal to
whether the command is installed system-wide or user-specific.
There should be no reason to append to existing ones. Other similar
completion file install recipes overwrite, too.
@scop scop force-pushed the bash-completion-install branch 2 times, most recently from 67c319e to ac48ae1 Compare November 26, 2023 17:46
The downside of installing completions in their expanded form is that
they should be re-installed on respective tool updates to keep them in
sync. Generating on demand fixes that, in exchange for a few
milliseconds on first load.
@scop scop force-pushed the bash-completion-install branch from ac48ae1 to 65058ba Compare November 26, 2023 18:03
@scop
Copy link
Contributor Author

scop commented Nov 26, 2023

Updated, but no success yet: something seems to be changing backslash escapes to slashes in the outputs, don't know what should be done about that.

@rami3l rami3l self-assigned this Dec 20, 2023
@rami3l rami3l added this to the 1.28.0 milestone Jan 17, 2024
@rami3l
Copy link
Member

rami3l commented Jan 17, 2024

Updated, but no success yet: something seems to be changing backslash escapes to slashes in the outputs, don't know what should be done about that.

@scop Sorry for the late reply!

I'll paste the original error below for the sake of convenience:

---- expected: stdout
++++ actual:   stdout
   1    1 | ...
   2    2 | Generate tab-completion scripts for your shell
   3    3 | 
   4    4 | Usage: rustup[EXE] completions [shell] [command]
   5    5 | 
          ⋮
  27   27 |     `/usr/share/bash-completion/completions`, and user-specific ones
  28   28 |     can be stored in `~/.local/share/bash-completion/completions`.
  29   29 |     Run the command:
  30   30 | 
  31   31 |         $ mkdir -p ~/.local/share/bash-completion/completions
  32      -         $ printf 'eval -- "$("$1" completions bash)"\n' \
       32 +         $ printf 'eval -- "$("$1" completions bash)"/n' /
  33   33 |               > ~/.local/share/bash-completion/completions/rustup
  34   34 | 
  35   35 |     This installs the completion script. You may have to log out and
  36   36 |     log back in to your shell session for the changes to take effect.
  37   37 | 
          ⋮
  39   39 | 
  40   40 |     Homebrew stores bash completion files within the Homebrew directory.
  41   41 |     With the `bash-completion` brew formula installed, run the command:
  42   42 | 
  43   43 |         $ mkdir -p $(brew --prefix)/etc/bash_completion.d
  44      -         $ printf 'eval -- "$(rustup completions bash)"\n' \
       44 +         $ printf 'eval -- "$(rustup completions bash)"/n' /
  45   45 |               > $(brew --prefix)/etc/bash_completion.d/rustup.bash-completion
  46   46 | 
  47   47 |     Fish:
  48   48 | 
  49   49 |     Fish completion files are commonly stored in
          ⋮
 125  125 |     toolchain. Not all shells are currently supported. Here are examples for
 126  126 |     the currently supported shells.
 127  127 | 
 128  128 |     Bash:
 129  129 | 
 130      -         $ printf 'eval -- "$(rustup completions bash cargo)"\n' \
      130 +         $ printf 'eval -- "$(rustup completions bash cargo)"/n' /
 131  131 |               > ~/.local/share/bash-completion/completions/cargo
 132  132 | 
 133  133 |     Zsh:
 134  134 | 
 135  135 |         $ rustup completions zsh cargo > ~/.zfunc/_cargo

IIRC this has something to do with after_help's markup support.
I'll look into it and hopefully find a solution real soon.

(cc @epage)

Update: As it turns out, the replacement of \s is made by trycmd, as explained in #3177 (comment).

@epage
Copy link

epage commented Jan 17, 2024

trycmd changes backslashes to forward slashes so that paths can be represented in a more neutral way across platforms. This was modeled off of cargo-test-support.

@rami3l
Copy link
Member

rami3l commented Jan 18, 2024

@epage Thanks for your quick response!

This seems to be assert-rs/snapbox#91 then.
@djc Before resolving that issue, maybe we should just accept the hack and use / in the snapshots?

@djc
Copy link
Contributor

djc commented Jan 23, 2024

This seems to be assert-rs/trycmd#91 then. @djc Before resolving that issue, maybe we should just accept the hack and use / in the snapshots?

Seems fine?

@@ -73,10 +73,10 @@ simple as using one of the following:

```console
# Bash
$ rustup completions bash > ~/.local/share/bash-completion/completions/rustup
$ printf 'eval -- "$("$1" completions bash)"\n' > ~/.local/share/bash-completion/completions/rustup
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the value of using $1 here? It seems rather obfuscated to me.

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.

5 participants