In reinstall-all if shared_lib python is invalid, create it instead of upgrading to avoid error.

[itsayellow]

• I have added an entry to docs/changelog.md

Summary of changes

Closes #633.

#554 introduced a regression where pipx reinstall-all would try to upgrade the shared_libs without verifying that they were still valid (i.e. their python link still existed). If the python did not exist, this would output an error.

This PR first checks if the shared_libs are valid: if they are, it forces an upgrade, if they are not, it instead creates (or re-creates) the shared libs.

Test plan

Tested by running

rm ~/.local/pipx/shared/bin/python3
ln -s /path/that/does/not/exist ~/.local/pipx/shared/bin/python3
pipx reinstall-all

This PR:
(no error)

pipx 0.16.0.0:

Failed to upgrade shared libraries
Traceback (most recent call last):
  File "/usr/local/Cellar/pipx/0.16.0.0/libexec/lib/python3.9/site-packages/pipx/shared_libs.py", line 92, in upgrade
    upgrade_process = run_subprocess(
  File "/usr/local/Cellar/pipx/0.16.0.0/libexec/lib/python3.9/site-packages/pipx/util.py", line 123, in run_subprocess
    completed_process = subprocess.run(
  File "/usr/local/Cellar/python@3.9/3.9.1_8/Frameworks/Python.framework/Versions/3.9/lib/python3.9/subprocess.py", line 501, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/usr/local/Cellar/python@3.9/3.9.1_8/Frameworks/Python.framework/Versions/3.9/lib/python3.9/subprocess.py", line 947, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/local/Cellar/python@3.9/3.9.1_8/Frameworks/Python.framework/Versions/3.9/lib/python3.9/subprocess.py", line 1819, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: '/Users/mclapp/.local/pipx/shared/bin/python'

[...]

[cs01]

What do you think about moving this check to the internals of the upgrade method? That way any other calls to upgrade wouldn’t be subject to this issue either.

[itsayellow]

Yeah I thought about that. It's probably good to do.

I hesitated to do that because it makes the code inside of shared_libs.py look almost like a circular reference (even though it's not) because create ends with upgrade, and then with that code change upgrade would fall back to create. It's actually not a circular reference, just worried it might read funny.

[itsayellow]

OK, made that fix. 👍

[uranusjr]

is_valid would still return True if a Windows venv’s underlying interpreter goes away. I wonder if we should actually try to execute the virtual environment instead (e.g. run python -c "import sys; print(sys.executable)"). This can probably be fixed separately though (when someone complains about it).

Also create() already has an is_valid guard.

[itsayellow]

Also create() already has an is_valid guard.

I think that should be ok, do you see a problem? I put this one in upgrade for when external code calls upgrade itself, not for when create() calls upgrade().

[uranusjr]

If you ask me, create() probably shouldn’t contain an is_valid guard, since it feels weird to me calling create() may not actually create anything. But removing that guard probably has consequences.

[itsayellow]

Ah that's a different matter. I think it was meant as an optimization to save time. We use shared_libs.create() every time we create a new venv. I think probably this is meant to prevent having to recreate the shared_libs if it's already fine.

We could move the guard to the calling code in venv.py, which would functionally be the same thing just factored differently. Personally I'm ok either way.

[uranusjr]

Let’s keep this guard here and aim to remove the one in create() then.