Fix: support newer conda #290

lmmarsano · 2018-11-21T10:15:46Z

pyenv-virtualenv had issues listing conda environments, activating conda environments, and cleaning up pyenv virtualenv runs on error.
The issue is primarily explained in #178: conda and activate files no longer exist under derived conda environment bin directories, so pyenv-virtualenv can't use them to detect conda.
To resolve this, I add criteria to detect a conda-meta subdirectory.
Obtaining VIRTUALENV_PREFIX_PATH uses an implementation of realpath that would normalize paths across multiple scripts (though this might also be achieved with cd in a subshell).
(Moreover, I'm puzzled that substitutes for readlink and related commands are redefined throughout several scripts instead of defined in a single file for inclusion via sourceing. Is there a reason for this?)
A pyenv-which hook redirects conda invocations to its new location in the base environment.
Is this the best way to go about it?

I'd like to include functional tests, though I'm not sure how to best do that.
I welcome suggestions/solutions.

closes pyenv/pyenv#1230
closes #178
and probably others

yyuu

Thanks for your patch! Looks good in general 👍

yyuu · 2018-11-21T10:27:28Z

libexec/pyenv-virtualenv-realpath

+
+if ! {
+    enable -f "${BASH_SOURCE%/*}"/../libexec/pyenv-realpath.dylib realpath ||
+      type realpath


Do we need to have this condition? At least this is something new and I'd like to know your intention of this new condition.

The purpose is to skip defining unneeded substitutes.
realpath is commonly available: coreutils & busybox have it, so it's basically standard in Linux systems.
BSD systems commonly have it, too.
Is there something wrong with the native commands?

It's fine to use GNU coreutils feature as long as we have fallback code for where the platform without it. My point was that there was no such condition before you split the code into library condition and would like to know your intention for the change.

yyuu · 2018-11-21T10:32:57Z

libexec/pyenv-virtualenv-realpath

+
+  realpath() {
+    local cwd="$PWD"
+    local path="$(normalize "$1")"


Is this normalization something nice to have, or a requirement to make ot working...? To be honest, the normalization code looks little complicated and took me awhile to understand the bahaviour. Plus, I think the realpath.dynlib won't behave equally.

realpath "$(realpath "${PYENV_PREFIX_PATH}")"/../.. didn't work with that standard behavior missing.
(The previous realpath definition breaks down at the resolve_link .. step.)
There might be better alternatives.
I'll try to simplify it if possible.

How do you get realpath.dynlib?
If it doesn't follow standards, then it needs to be brought into compliance.

pyenv-virtualenv/libexec/pyenv-virtualenv-realpath

Lines 24 to 49 in e0a1e9b

realpath() {

local f="$*" \

name dir

[[ $f ]] || {

>&2 echo ${FUNCNAME[0]}: missing operand

return

}

while [[ -L $f ]]; do

f="$(resolve_link "$f")"

done

if [[ ! -d $f ]]; then

name="/${f##*/}"

# parent?

dir="${f%/*}"

if [[ $dir == $f ]]; then

#lacks /: parent is current directory

f="$PWD"

else

f="$dir"

fi

fi

#absolute directory

dir="$(cd "$f"

pwd)"

echo "$dir$name"

}

Simplified it: normalize is now gone.
(My previous version was wrong: normalize should happen after link resolution, not before.)
Simply realpath "${PYENV_PREFIX_PATH}"../.. should work (as it does with standard realpath).
Is this better?

yyuu · 2018-11-21T10:33:48Z

libexec/pyenv-virtualenv-realpath

+#!/usr/bin/env bash
+# Summary: Substitute realpath if unavailable as a builtin or file
+# Usage: . pyenv-virtualenv-realpath
+


I prefer to have "set -e" in all scripts.

I can do that.
However, are you aware this is an inline shell file and not a proper script?
It's never run on it's own.
It's always run inline/sourced inside another script already doing set -e.
It's really like a reusable, incomplete section of larger script, like the files under pyenv.d.
Should I rename it?

Fair enough. I was overlooking that this script was placed umder libexec dir. It's fine without -e.

pyenv-virtualenvs could not list conda environments & pyenv shell would only activate the base conda environment the conda detection criteria of testing the presence of `conda` or `activate` files under `$(pyenv root)/versions/$version/bin` appears to be the culprit, since newer environments no longer include these files: those files reside in the base conda environment - add detection criteria of `$(pyenv root)/versions/$version/conda-meta` - compute the real prefix to the base environment from `realpath $(realpath $(pyenv root)/versions/$version)/../..` - to allow that, enhance substitute `realpath` in `pyenv-virtualenvs` to reduce relative paths `.` & `..`, and factor that code out to a file under `libexec` for reuse - hook `which` to locate conda from the real prefix

also cleanup compatibility paths

yyuu · 2018-11-23T12:50:45Z

libexec/pyenv-virtualenv-realpath

+#!/usr/bin/env bash
+# Summary: Substitute realpath if unavailable as a builtin or file
+# Usage: . pyenv-virtualenv-realpath
+


Fair enough. I was overlooking that this script was placed umder libexec dir. It's fine without -e.

yyuu · 2018-11-23T12:54:04Z

libexec/pyenv-virtualenv-realpath

+
+if ! {
+    enable -f "${BASH_SOURCE%/*}"/../libexec/pyenv-realpath.dylib realpath ||
+      type realpath


It's fine to use GNU coreutils feature as long as we have fallback code for where the platform without it. My point was that there was no such condition before you split the code into library condition and would like to know your intention for the change.

non-base conda environments lack bin/conda apply same solution as pyenv#290

guesswh0 · 2020-03-03T16:27:53Z

For macOS users don't forget to install:

brew install coreutils

yyuu reviewed Nov 21, 2018

View reviewed changes

lmmarsano added 2 commits November 21, 2018 22:00

cleanup pyenv virtualenv on ERR

ed779fe

also cleanup compatibility paths

lmmarsano force-pushed the fix-conda branch from e9ab7f5 to ed779fe Compare November 22, 2018 03:01

yyuu approved these changes Nov 23, 2018

View reviewed changes

yyuu merged commit 6d90f61 into pyenv:master Nov 23, 2018

lmmarsano mentioned this pull request Nov 23, 2018

Conda 4.4.4 breaks pyenv virtual environments #246

Open

taschini mentioned this pull request Nov 26, 2018

Can not access Anaconda/Miniconda environment as virtualenv #178

Closed

NathanDotTo mentioned this pull request Nov 26, 2018

pyenv activate/deactivate not working #13

Closed

jxxiao mentioned this pull request Dec 14, 2018

can't use pyenv activate a anaconda-virtualenv #293

Open

zeertzjq mentioned this pull request Jan 30, 2019

Maybe a new release for the conda fix? #299

Closed

yyuu added a commit that referenced this pull request Feb 13, 2019

Install libexec as well under the given prefix (#290, #302)

e39b112

lmmarsano added a commit to lmmarsano/pyenv-virtualenv that referenced this pull request Oct 9, 2019

detect conda environments

9c23313

non-base conda environments lack bin/conda apply same solution as pyenv#290

lmmarsano mentioned this pull request Oct 9, 2019

detect & create conda environments #327

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: support newer conda #290

Fix: support newer conda #290

lmmarsano commented Nov 21, 2018

yyuu left a comment

yyuu Nov 21, 2018

lmmarsano Nov 21, 2018

yyuu Nov 23, 2018

yyuu Nov 21, 2018

lmmarsano Nov 21, 2018 •

edited

Loading

lmmarsano Nov 22, 2018

yyuu Nov 21, 2018

lmmarsano Nov 21, 2018

yyuu Nov 23, 2018

yyuu Nov 23, 2018

yyuu Nov 23, 2018

guesswh0 commented Mar 3, 2020

	realpath() {
	local f="$*" \
	name dir
	[[ $f ]] \|\| {
	>&2 echo ${FUNCNAME[0]}: missing operand
	return
	}
	while [[ -L $f ]]; do
	f="$(resolve_link "$f")"
	done
	if [[ ! -d $f ]]; then
	name="/${f##*/}"
	# parent?
	dir="${f%/*}"
	if [[ $dir == $f ]]; then
	#lacks /: parent is current directory
	f="$PWD"
	else
	f="$dir"
	fi
	fi
	#absolute directory
	dir="$(cd "$f"
	pwd)"
	echo "$dir$name"
	}

Fix: support newer conda #290

Fix: support newer conda #290

Conversation

lmmarsano commented Nov 21, 2018

yyuu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmmarsano Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guesswh0 commented Mar 3, 2020

lmmarsano Nov 21, 2018 •

edited

Loading