Support virtualenvwrapper with / without pyenv virtualenv-init or virtualenvwrapper plugins

[jeffwidman]

The desired logic is:

For the pyenv plugins virtualenv-init and virtualenvwrapper:

1. If either plugin is present, activate it
2. If virtualenvwrapper plugin is not present, then fallback to standard virtualenvwrapper.
3. If virtualenvwrapper plugin is present, then don't fallback to
   standard virtualenvwrapper, regardless of whether virtualenv-init is present.

Previously, if the virtualenv command was present but pyenv was
missing, then the fallback wouldn't be hit. This bug was introduced by
#1981 which ensured that
the pyenv virtualenvwrapper plugin was activated if present,
regardless of the presence of the virtualenv-init plugin.

As an optimization, the check for the pyenv plugins are skipped if
pyenv itself isn't found.

Since we only want to fallback if the pyenv virtualenvwrapper plugin
is missing, but that's buried within the pyenv logic and we also need
to handle when pyenv itself is missing, this switches to using a flag
variable.

I also renamed the virtualenv_sources var to
virtualenvwrapper_sources as virtualenv is distinct from
virtualenvwrapper, so using one name for a var that is really about
the other is confusing.

Looking at git blame, there's a lot of prior art here around trying
to support all the permutations of pyenv and various plugins:

• virtualenvwrapper stopped working #1413
• [python] Detect availability of virtualenv pyenv plugins as well as pyenv #1414
• [python] Edit pyenv virtualenv-init check to support non-brew installations #1433
• [python] Refine pyenv based virtualenv management #1434

So we need to be extremely careful to continue to support all these permutations.

Fix #2022

[jeffwidman]

The only change in this block is indenting within the pyenv check + setting the pyenv_virtualenvwrapper_plugin_found var.

[jeffwidman]

The only change in this block of logic is the rename of virtenv_sources to virtualenvwrapper_sources

[jeffwidman]

Testing revealed this line isn't necessary... but I was unclear if it's more idiomatic to declare the var since otherwise it's only set deep in nested if, but then tested at a higher level of scope.

Also unclear if when declaring it's best to declare it as a local or not... I don't understand why pyenv_plugins is declared as a local but virtenv_sources isn't...

[belak]

I think pyenv_plugins is declared locally so we can specify that it's an array. It seems reasonable to also declare pyenv_virtualenvwrapper_plugin_found up here.

[jeffwidman]

@zbirenbaum @diraol @zachwhaley since you were all involved in prior PR's around this section of code logic, can you take this for a test drive to ensure I'm not accidentally breaking something in your use cases with this.

Also @indrajitr / @belak I'd appreciate some sanity check reviews.

[belak]

I really like this change - I think it's a more formalized version of the logic we wanted in the first place and starts removing some of the magic/inconsistencies.

It's way easier to say "pyenv's virtualenv plugin will be used when available, otherwise we fall back to the plain virtualenvwrapper".

[belak]

I think pyenv_plugins is declared locally so we can specify that it's an array. It seems reasonable to also declare pyenv_virtualenvwrapper_plugin_found up here.

[zbirenbaum]

I like this too. My change was a quick one to prevent people from getting locked out of their systems since that’s a really bad bug to have. This is a better formalization.

[jeffwidman]

Thanks all!

I'm going to merge given the positive reception. If we find further bugs/unhandled edge cases, I'm happy to take another look.