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

Linuxbrew shell fix #6215

Merged
merged 4 commits into from
Jul 31, 2024
Merged

Linuxbrew shell fix #6215

merged 4 commits into from
Jul 31, 2024

Conversation

oz123
Copy link
Contributor

@oz123 oz123 commented Jul 31, 2024

Add some cosmetic change to the comments.

Basically, all the work was done by @JoshStern.

See #6212 .

JoshStern and others added 4 commits July 29, 2024 17:45
Addresses issue: #6197

Previously these branches would trip if the shell name was present
anywhere in the cmd path. This updates the check to only look at the
end.
Updates the if else block to only allow an explicit set of shells instead of defaulting to the `activate` script.
@oz123 oz123 merged commit 6b133ef into main Jul 31, 2024
19 checks passed
@oz123 oz123 deleted the brew-shell-fix branch July 31, 2024 08:43
# Support for fish shell.
if "fish" in cmd:
# Suffix and source command for various shells.
if cmd.endswith("/sh", "/bash", "/zsh"):
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work on older python versions -- I am raising a PR:

matteius@matteius-VirtualBox:~/opensensor-api$ pipenv shell
Loading .env environment variables...
Launching subshell in virtual environment...
bash
Traceback (most recent call last):
  File "/home/matteius/.local/bin/pipenv", line 8, in <module>
    sys.exit(cli())
             ^^^^^
  File "/home/matteius/pipenv/pipenv/vendor/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/matteius/pipenv/pipenv/cli/options.py", line 52, in main
    return super().main(*args, **kwargs, windows_expand_args=False)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/matteius/pipenv/pipenv/vendor/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/home/matteius/pipenv/pipenv/vendor/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/matteius/pipenv/pipenv/vendor/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/matteius/pipenv/pipenv/vendor/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/matteius/pipenv/pipenv/vendor/click/decorators.py", line 92, in new_func
    return ctx.invoke(f, obj, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/matteius/pipenv/pipenv/vendor/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/matteius/pipenv/pipenv/cli/command.py", line 400, in shell
    do_shell(
  File "/home/matteius/pipenv/pipenv/routines/shell.py", line 48, in do_shell
    shell.fork_compat(*fork_args)
  File "/home/matteius/pipenv/pipenv/shells.py", line 111, in fork_compat
    c.sendline(_get_activate_script(self.cmd, venv))
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/matteius/pipenv/pipenv/shells.py", line 40, in _get_activate_script
    if cmd.endswith("/sh", "/bash", "/zsh"):
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: slice indices must be integers or None or have an __index__ method

Copy link
Member

Choose a reason for hiding this comment

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

Actually there are other issues with this change -- I recommend reverting it -- for example, my cmd is just "bash" not "/bash" so fixing that yields:

ValueError: unknown shell bash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened #6230 fixing this.

@oz123 oz123 mentioned this pull request Sep 1, 2024
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