Skip to content

Conversation

JoshStern
Copy link
Contributor

Thank you for contributing to Pipenv!

The issue

Shell detection checks the entire path when looking for shell substrings. This can cause the wrong branch to get tripped if shells are installed in a path with another shell in the tree. In my case a li[nu]xbrew-installed zsh goes down the nu shell path.

The fix

This updates the check to only look at the end of cmd when detecting shells.

The checklist

  • Associated issue: Linuxbrew shells are detected as nu shell #6197
  • A news fragment in the news/ directory to describe this fix with the extension .bugfix.rst, .feature.rst, .behavior.rst, .doc.rst. .vendor.rst. or .trivial.rst (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

Addresses issue: pypa#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.
Copy link
Contributor

@oz123 oz123 left a comment

Choose a reason for hiding this comment

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

Actually, I prefer this over the split over .linuxbrew.
This solution will cause future maintainers to raise eyebrows and wrinkle their foreheads trying to figure it out without reading the original issue.

This approach is much more maintainable. However, I have a small suggestion:

Change:

cmd.endswith("<shell name>")

to:

cmd.endswith("/<shell name>"):

This will allow to for shortcutting the if ...elif at the head in favor of the most common shells (bash, zsh, sh):

if cmd.endswith("/sh","/bash","/zsh")":
   suffix = ""
  command = "."
elif cmd.endswith("/csh"):
  ...
  
else:
     raise ValueError("unknown shell ...") ....

This works for all three:

>>> cmd="/bin/sh"
>>> cmd.endswith(("/bash","/zsh", "/sh"))
True
>>> cmd="/bin/zsh"
>>> cmd.endswith(("/bash","/zsh", "/sh"))
True
>>> cmd="/bin/bash"
>>> cmd.endswith(("/bash","/zsh", "/sh"))
True
>>> 

@oz123
Copy link
Contributor

oz123 commented Jul 30, 2024

@JoshStern thank you for your effort so far (immediately adding news file, is great! ).

I gave your PR some consideration and I think that your solution is better than what I initially suggested.
Adding a shortcut for the most common shells is only possible with your approach.

@JoshStern
Copy link
Contributor Author

JoshStern commented Jul 30, 2024

@oz123 updated! Thank you for the feedback and quick turnaround. Want me to also update the branch ordering?

@oz123
Copy link
Contributor

oz123 commented Jul 30, 2024

Yes. Please. The zsh, bash and sh bash should be at the top. Unknown shells should not fall silently to imaginary default. They should raise exceptions.

Updates the if else block to only allow an explicit set of shells instead of defaulting to the `activate` script.
@JoshStern
Copy link
Contributor Author

JoshStern commented Jul 30, 2024

Updated, I'm a little concerned about other sh or bash compatible shells out there that will get sent down the ValueError branch. ksh comes to mind.

I do think an explicit list is better though. I defer to you on the right balance.

@oz123 oz123 mentioned this pull request Jul 31, 2024
@oz123
Copy link
Contributor

oz123 commented Jul 31, 2024

Thanks! I merged your commits with one on top!

@oz123 oz123 closed this Jul 31, 2024
@JoshStern
Copy link
Contributor Author

Thank you for the help with this! Glad I could contribute 😄

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.

2 participants