https://pantsbuild.org/ logo
Join Slack
Powered by
is there a reason we can't just rename the directo...
# announce
a
is there a reason we can't just rename the directory to 
'{}_py36'.format(pants_version)
?
☝️ 1
w
but yea, i had that thought as well. if the version you "guessed" is the same version you ended up pinning, they should match, and you wouldn't rebootstrap
...oh, is this because we're resolving 
latest
? and we don't know what that is without querying pip? ... we could do that
h
The venv name is a function of both the interpreter used and the pants version, which can be “unspecified”. I’m afk but think the issue is entirely with “unspecified” vs the actual pants version, not the python version
a
after a 
./pants generate-pants-ini
invocation, we have generated a 
pants.ini
with the correct pants version, so it seems reasonable that we have enough information to rename the unspecified-version cache dir to the correct version during the pants.ini generation
h
Are you proposing Pants makes this change or the Bash script? If Pants making the change, we would need to teach Pants how to locate the cache folder and what the cache naming scheme is.
a
but we're doing that location process somehow in the next bootstrap step where we end up generating the venv again
like if we're creating this directory it seems like that implies we can locate it
h
The Bash script is doing that location process. Pants has no idea
a
ok. we can have the bash script pass in the location to pants via an env var consumed in the 
generate-pants-ini
task then or something
w
see above re: pip.
a
i need to read up on what is happening to diagnose
ah ok
h
So we could teach Pants how to locate the cache folder, though. I do think that’s feasible. It smells wrong, but this is also the very first command new users ever run so I think it could be justified to make an exception. I have a strong aversion to modifying the Bash script at all (unless we’re going to use the completely Bash-centric approach, which I don’t advocate). The bash script is complex enough as is, and is directly exposed to users. It’s also ran every time, which would be unfortunate for a command that is only ever ran once per repository per device. Instead, the solution needs to be completely self-contained in Pants code imo
For example, here’s a very simple way to do this in `generate_pants_ini.py`:
Copy code
venv_folder = sys.exec_path
if venv_folder.endswith("unspecified_py*"}:
  os.rename(venv_folder, new_name)
Tbd if renaming the parent folder for the currently running 
sys.executable
is a recipe for disaster. Trying now..
w
bigger fish, i suspect
a
i can make an issue against the setup repo
h
Huh, proof of concept worked fine so far! @witty-crayon-22786, what do you mean by bigger fish?
a
to fry
w
just that it's probably a lower priority than a bunch of other things
which is not supposed to shut down conversation, heh... just increase perspective, i hope
h
I would move on if this were going to require modifying the bash script, but so far seems like we can implement it in 
generate_pants_ini.py
in only a few minutes. Bigger concern is how to test it
w
it would only work for virtualenvs probably? imo, not worth it.
"it" here being "the special case"
h
it would only work for virtualenvs probably? imo, not worth it.
You mean no need for a test? That would be nice to me. I can upload a draft PR in the next few minutes to see what you all think
a
i would appreciate a test and can write one myself
👍 1
w
er, no: sorry. i don't think special casing virtualenv bootstrap into pants is worth it
a
all we need to do is check the cache dir which we don't need heavy mocking to do i don't think
hm ok
well we would like to remove the second bootstrap and the first one occurs because it creates an 
unspecified_py36
dir
that could be fixed in a myriad of ways
h
Yeah this would fix the second bootstrap. Let’s hold this convo till I get the draft PR up so we’re all the on the same page, if that’s okay
https://github.com/pantsbuild/pants/pull/7607 Some key points: - if not using our script, has no impact. - if using our script but an outdated version that doesn’t use the new 
_py36
naming scheme, has no impact.
e
Played the devil on the draft PR. I actually think we can do the right thing from the bash script sanely - namely default to latest-stable pants release and never leave pants_version unspecified.
h
How? I think that would be great and cleaner, but thought there was a dependency issue that we can’t determine the version until using Pip, which requires a bootstrap folder
e
See the comment for the method
The pypi json api returns the latest stable version (as well as all others)
Right now I see the correct answer:
Copy code
$ curl -sSL <https://pypi.python.org/pypi/pantsbuild.pants/json> | python -c "import json, sys; print(json.load(sys.stdin)['info']['version'])"
1.15.0
💯 1
h
Much better! I like that more, as fun as this draft PR is to write
e
Excellent. Thanks for perservering through all the thrash on this stuff.
h
Will submit a PR tonight to make this change. It might impact our testing, which I’ll look into. More thanks to you all for putting up with the thrash - the Py3 migration is the source of it all after all. Thanks though 🙂
So with this change, we could change the order of how we resolve Python version. Now we’ll be guaranteed to always have a Pants version, meaning we could first resolve the Pants version and then use that to resolve the Python interpreter. The benefit is we don’t have to update the script every time we have a major release the next three months
a
that sounds like a nice benefit
h
I think the logic also makes sense. The Python version is purely a function of the Pants version. Should lead to fewer lines of code too
👍 2
Open in Slack
PreviousNext
Powered by