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

Don't fail with error when bash is not found or too old #2574

Closed
superatomic opened this issue Jul 29, 2023 · 8 comments
Closed

Don't fail with error when bash is not found or too old #2574

superatomic opened this issue Jul 29, 2023 · 8 comments
Milestone

Comments

@superatomic
Copy link
Contributor

As discussed in #2152 and #2200, click does not support bash shell autocompletion for bash versions older than version 4.4. I understand this and the rational behind this decision, and I am not requesting that this should be changed. However, the way that click makes developers aware of this version limitation has caused an issue with the ability for autocompletion generation to be scripted.

I have developed a CLI tool using click and I have tried to package it for the Homebrew package manager. As a part of the installation script, autocompletion scripts for bash, zsh, and fish must be generated at installation time. Homebrew isolates builds by removing /usr/local/bin and all user PATHs that are not essential for the build. This means that a newer version of bash (>4.4) will not be present at build time, even if it is present on the system, which causes click to fail to generate bash autocompletion, even if a user has a newer version of bash installed on their system:

Traceback (most recent call last):
  File "/Users/Olivia/Code/tldr-man/.venv/bin/tldr", line 6, in <module>
    sys.exit(cli())
  File "/Users/Olivia/Code/tldr-man/.venv/lib/python3.10/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/Users/Olivia/Code/tldr-man/.venv/lib/python3.10/site-packages/click/core.py", line 1050, in main
    self._main_shell_completion(extra, prog_name, complete_var)
  File "/Users/Olivia/Code/tldr-man/.venv/lib/python3.10/site-packages/click/core.py", line 1125, in _main_shell_completion
    rv = shell_complete(self, ctx_args, prog_name, complete_var, instruction)
  File "/Users/Olivia/Code/tldr-man/.venv/lib/python3.10/site-packages/click/shell_completion.py", line 45, in shell_complete
    echo(comp.source())
  File "/Users/Olivia/Code/tldr-man/.venv/lib/python3.10/site-packages/click/shell_completion.py", line 326, in source
    self._check_version()
  File "/Users/Olivia/Code/tldr-man/.venv/lib/python3.10/site-packages/click/shell_completion.py", line 314, in _check_version
    raise RuntimeError(
RuntimeError: Shell completion is not supported for Bash versions older than 4.4.

This error is caused by the following function, which will make bash autocompletion simply fail if it detects that an incompatible version is being used:

def _check_version(self) -> None:
import subprocess
output = subprocess.run(
["bash", "-c", 'echo "${BASH_VERSION}"'], stdout=subprocess.PIPE
)
match = re.search(r"^(\d+)\.(\d+)\.\d+", output.stdout.decode())
if match is not None:
major, minor = match.groups()
if major < "4" or major == "4" and minor < "4":
raise RuntimeError(
_(
"Shell completion is not supported for Bash"
" versions older than 4.4."
)
)
else:
raise RuntimeError(
_("Couldn't detect Bash version, shell completion is not supported.")
)

It makes sense for bash autocompletion generation to always succeed, like it does for both zsh and fish (zsh and fish autocompletion generation will succeed no matter what, even if neither shell is present on the system). While it is important to notify the user if they are using a version of bash that is too old to be supported, it is a bug for this to cause autocompletion generation to fail entirely. I propose for these error messages to be changed into warning messages that are still displayed, but do not prevent the generation from succeeding.

I am willing to submit a pull request myself with the proposed changes if these changes are wanted.

Environment:

  • Python version: 3.10.12
  • Click version: 8.1.3
@davidism
Copy link
Member

davidism commented Jul 29, 2023

autocompletion scripts for bash, zsh, and fish must be generated at installation time.

No they don't, they can be generated at build time and installed to the correct locations. That's actually the intended workflow when distributing an application.

@superatomic
Copy link
Contributor Author

superatomic commented Jul 29, 2023

autocompletion scripts for bash, zsh, and fish must be generated at installation time.

No they don't, they can be generated at build time and installed to the correct locations. That's actually the intended workflow when distributing an application.

Thank you for your response!

With Homebrew, packages are built and installed in the same place. Unless I misunderstand you?

Regardless, it seems unintended for bash to behave differently than zsh and fish.

@davidism
Copy link
Member

I didn't realize it was built on the user's machine. I'm fine with modifying the check to be a warning at generation and an error at runtime. Happy to review aPR.

@superatomic
Copy link
Contributor Author

Thank you! I'll develop and submit a PR for review tomorrow.

@superatomic
Copy link
Contributor Author

A quick clarification: by "error at runtime", do you mean checking the bash version at the start of the bash script itself, and displaying the error message then?

@davidism
Copy link
Member

The completion script calls click to produce the results, there's a method in the bash completer class that can be modified.

@superatomic
Copy link
Contributor Author

superatomic commented Jul 29, 2023

I moved the version check from the source(...) method into the get_completion_args(...) method, and it works. However, there is a problem with this implementation: Since bash completion does not work with versions <4.4, the completion script isn't able to call click to produce the results, since the script fails to call complete with the error bash: complete: nosort: invalid option name. This means that the check will not run in common usage. Notably, this change does resolve the issue (Homebrew will now generate Bash autocompletion), since the call to _check_version() no longer occurs at build time.

I see two solutions to this problem:

  • Write the BASH_VERSION check inside the Bash script itself by changing _SOURCE_BASH.
  • Have _check_version() print a warning to stderr instead of raising an error, and don't raise any error at runtime, allowing for the autocompletion to fail with the error bash: complete: nosort: invalid option name.

Option 2 is what I currently have implemented, and it works pretty well. It prints warnings when generating Bash autocompletion with an insufficient version at build time, and resolves this issue.

It's also worth mentioning that that the click shell completion documentation details the Bash version requirement in the first paragraph.

@superatomic
Copy link
Contributor Author

Draft implementation in #2576.

@davidism davidism added this to the 8.1.7 milestone Aug 17, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants