https://pantsbuild.org/ logo
Powered by
w

witty-crayon-22786

03/03/2021, 12:06 AM
oh, hello there:
Copy code
def get_pyenv_root():
    try:
        return subprocess.check_output(["pyenv", "root"]).decode().strip()
    except (OSError, subprocess.CalledProcessError):
        return None
i guess that explicitly setting PYENV_ROOT is optional …?
…yea.
h

hundreds-father-404

03/03/2021, 12:10 AM
Where is this from?
w

witty-crayon-22786

03/03/2021, 12:10 AM
src/python/pants/python/python_setup.py
👍 1
but the default is well defined.
e

enough-analyst-54434

03/03/2021, 12:30 AM
But you definitely don't need it either (I use pyenv - I don't use shims):
Copy code
$ echo $PYENV_ROOT

$ pyenv root
/home/jsirois/.pyenv
w

witty-crayon-22786

03/04/2021, 9:24 PM
i’ve changed this lookup to:
Copy code
def get_pyenv_root(pants_env: PantsEnvironment) -> str:
    """See <https://github.com/pyenv/pyenv#environment-variables>."""
    from_env = pants_env.env.get("PYENV_ROOT")
    if from_env:
        return from_env
    home_from_env = pants_env.env.get("HOME")
    if home_from_env:
        return os.path.join(home_from_env, ".cache")
    raise ValueError(
        f"The environment {pants_env} does not contain enough information to locate pyenv."
    )
… and that seems fine.
but i’m looking at this as part of the capstone of https://github.com/pantsbuild/pants/issues/7654 , and it occurs to me that how we find pythons in tests is probably … purely/implicitly 
PATH
based?
i use 
interpreter_search_path: [<PYENV>]
via 
~/.pantsrc
… and that’s triggering under unit tests (which is a bug that i need to look into independently).
but if i were to fix the accidental 
.pantsrc
usage, the next question is: how do we want to ensure that we’re able to find pythons in Pants’ tests?
i think that the answer is that we need to mark Pants’ own integration tests as passing through some relevant environment variables via https://www.pantsbuild.org/docs/python-test-goal#setting-environment-variables (although both $HOME and $PYENV_ROOT would be not-great from a caching perspective).
i might be over-thinking this though. will see if fixing the pantsrc leak leaves me with enough relevant interpreters to run unit tests.
e

enough-analyst-54434

03/04/2021, 9:37 PM
yeah - I'm not sure why we can't use 
pyenv root
. Clearly we can run that later in a rule if PYENV_ROOT is not in the env but pyenv is on the PATH - right?
w

witty-crayon-22786

03/04/2021, 9:38 PM
to use 
pyenv root
correctly, we’d need to pass through both of these env vars anyway
it works in master because the env was leaking implicitly via 
subprocess
(afaict)
e

enough-analyst-54434

03/04/2021, 9:39 PM
OK - I was not following the details just wanted to make sure you weren't pulling the feature back and making me set PYENV_ROOT un-necessarily.
w

witty-crayon-22786

03/04/2021, 9:40 PM
yea, the pantsrc thing muddies the water. let me come back once i’ve got that figured out.
@enough-analyst-54434: no: see the bit about their default value: https://github.com/pyenv/pyenv#environment-variables … subprocessing doesn’t appear to be necessary
e

enough-analyst-54434

03/04/2021, 9:42 PM
I do not follow. If I have pyenv installed/here and PATH=installed/here:... then the default does not work for me and yet I don;t need PYENV_ROOT in my env.
This is my setup on my machine today.
w

witty-crayon-22786

03/04/2021, 9:43 PM
that’s the 
.pantsrc
aspect. this is relevant to me because
Copy code
$ cat ~/.pants.rc
[python-setup]
# Avoid system python.
interpreter_search_paths = ["<PYENV>"]
e

enough-analyst-54434

03/04/2021, 9:44 PM
Again - I did not follow your rubber ducks. I just wanted to make sure 
pyenv root
would still be used if I had <PYENV> but no PYENV_ROOT exported.
w

witty-crayon-22786

03/04/2021, 9:44 PM
…and it’s ending up used in some tests (bug #1). but thinking about that, i wonder whether i will have useful pythons available after fixing this
@enough-analyst-54434: it will not. we will use HOME. see my first snippet from today. https://pantsbuild.slack.com/archives/C0D7TNJHL/p1614893058059300?thread_ts=1614729992.056700&amp;cid=C0D7TNJHL
e

enough-analyst-54434

03/04/2021, 9:45 PM
Exactly! My complaint is you're killing functionality.
Can the 
pyenv root
call not be moved off to a rule as a fallback?
w

witty-crayon-22786

03/04/2021, 9:46 PM
@enough-analyst-54434: not at all. 
pyenv root
requires PYENV_ROOT or HOME
if you run it without those, you will get the wrong answer (or it will fail.)
so AFAICT, i’m just making those lookups explicit and avoiding a subprocess call.
sorry. i should have raised the new point in a new thread.
if you run it without those, you will get the wrong answer (or it will fail.)
it appears that the result is: “wrong answer”:
Copy code
$ env -i /usr/local/bin/pyenv root
/.pyenv
e

enough-analyst-54434

03/04/2021, 9:51 PM
Yeah - I did not realize pyenv was broken this way.
I have 2 pyenvs and it reports the wrong root now that I look.
👀 1
Jesus
w

witty-crayon-22786

03/04/2021, 11:29 PM
OH. this is not a pantsrc leak (pheew). it’s just that the default 
--interpreter-search-paths
is 
["<PYENV>", "<PATH>"]
Powered by