ofek / userpath

Cross-platform tool for adding locations to the user PATH, no elevated privileges required!
MIT License
151 stars 20 forks source link

Python decode error during show_path_commands in fish #35

Closed itsayellow closed 3 years ago

itsayellow commented 3 years ago

pipxproject/pipx#644

We recently had a user who got a stacktrace when trying to use pipx ensurepath which uses userpath. He was using shell fish. The actual python error seemed to be in the middle of userpath, in utils.py when executing this command:

output = process.communicate()[0].decode('utf-8').strip()

The error:

UnicodeDecodeError: 'utf-8' codec can't decode byte 0x90 in position 210: invalid start byte

He was using the fish shell, and seemed to verify that his locale (LANG) was set to utf-8. When using bash to run pipx ensurepath it worked fine.

Here's the two caught stacktraces:

Traceback (most recent call last):
  File "/home/yasser/.local/lib/python3.9/site-packages/pipx/main.py", line 258, in run_pipx_command
    return commands.ensure_pipx_paths(force=args.force)
  File "/home/yasser/.local/lib/python3.9/site-packages/pipx/commands/ensure_path.py", line 103, in ensure_pipx_paths
    (path_added_current, need_shell_restart_current) = ensure_path(
  File "/home/yasser/.local/lib/python3.9/site-packages/pipx/commands/ensure_path.py", line 60, in ensure_path
    need_shell_restart = userpath.need_shell_restart(location_str)
  File "/home/yasser/.local/lib/python3.9/site-packages/userpath/core.py", line 22, in need_shell_restart
    return not in_current_path(location) and interface.location_in_new_path(location)
  File "/home/yasser/.local/lib/python3.9/site-packages/userpath/interface.py", line 101, in location_in_new_path
    new_path = get_flat_output(show_path_command)
  File "/home/yasser/.local/lib/python3.9/site-packages/userpath/utils.py", line 33, in get_flat_output
    output = process.communicate()[0].decode('utf-8').strip()
UnicodeDecodeError: 'utf-8' codec can't decode byte 0x90 in position 210: invalid start byte

and

Traceback (most recent call last):
  File "/home/yasser/.local/lib/python3.9/site-packages/pipx/main.py", line 258, in run_pipx_command
    return commands.ensure_pipx_paths(force=args.force)
  File "/home/yasser/.local/lib/python3.9/site-packages/pipx/commands/ensure_path.py", line 103, in ensure_pipx_paths
    (path_added_current, need_shell_restart_current) = ensure_path(
  File "/home/yasser/.local/lib/python3.9/site-packages/pipx/commands/ensure_path.py", line 60, in ensure_path
    need_shell_restart = userpath.need_shell_restart(location_str)
  File "/home/yasser/.local/lib/python3.9/site-packages/userpath/core.py", line 22, in need_shell_restart
    return not in_current_path(location) and interface.location_in_new_path(location)
  File "/home/yasser/.local/lib/python3.9/site-packages/userpath/interface.py", line 101, in location_in_new_path
    new_path = get_flat_output(show_path_command)
  File "/home/yasser/.local/lib/python3.9/site-packages/userpath/utils.py", line 33, in get_flat_output
    output = process.communicate()[0].decode('utf-8').strip()
UnicodeDecodeError: 'utf-8' codec can't decode byte 0x90 in position 210: invalid start byte

And here is his PATH:

/home/yasser/node_modules/ /usr/local/bin /usr/bin /bin /usr/local/sbin /usr/lib/jvm/default/bin /usr/bin/site_perl /usr/bin/vendor_perl /usr/bin/core_perl /var/lib/snapd/snap/bin
itsayellow commented 3 years ago

The full pipx log file: cmd_2021-03-08_17.13.54.log

itsayellow commented 3 years ago

I'm guessing this is a mismatch between the locale's encoding and 'utf-8'.

Probably this could be fixed by changing the appropriate lines in utils.py to:

def get_flat_output(command, sep=os.pathsep, **kwargs):
    process = subprocess.Popen(command, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True, **kwargs)
    output = process.communicate()[0].strip()

universal_newlines is valid for python > 3.3 (https://docs.python.org/3/library/subprocess.html#frequently-used-arguments)

Changed in version 3.3: When universal_newlines is True, the class uses the encoding locale.getpreferredencoding(False) instead of locale.getpreferredencoding(). See the io.TextIOWrapper class for more information on this change.

Or if you don't want to use the universal_newlines keyword, (still supporting python 2.7?) equivalently:

import locale

def get_flat_output(command, sep=os.pathsep, **kwargs):
    process = subprocess.Popen(command, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, **kwargs)
    output = process.communicate()[0].decode(locale.getpreferredencoding(False)).strip()
ofek commented 3 years ago

Thanks! Mind making a PR?

itsayellow commented 3 years ago

Hi @ofek, yes I was thinking about it! 😄 Just got a bit busy.

Do you have a preference to the approach? Using universal_newlines with subprocess.Popen is probably great for all the currently-active Python 3 versions. It exists for Python 2.7 but I'm not sure it does the right thing with respect to encodings.

Are you still trying to maintain Python 2.7 compatibility? If so then I'll probably do the second version with import locale.

ofek commented 3 years ago

Still py2, yes