classification
Title: Store the venv prompt in pyvenv.cfg
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: brett.cannon, cheryl.sabella, steve.dower, vinay.sajip, vstinner
Priority: low Keywords: patch, patch, patch

Created on 2019-01-05 00:58 by brett.cannon, last changed 2019-03-13 10:37 by cheryl.sabella. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 11440 merged cheryl.sabella, 2019-01-05 21:19
PR 12297 merged cheryl.sabella, 2019-03-12 22:33
Messages (21)
msg333030 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-01-05 00:58
When creating the pyvenv.cfg file, the prompt setting should be stored there so that tools can introspect on it (e.g. VS Code could read the value to tell users the name of the venv they have selected in the status bar).
msg333354 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-01-09 23:50
Adding my comment from the PR review:

Dealing with leading/trailing whitespace could get interesting here. All the other values can be trimmed at both ends, while this one (currently) must have one space removed from the start.

Perhaps we should insert the repr of the prompt instead? Or at least double-quote it and escape internal quotes and backslashes?
msg333377 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2019-01-10 13:08
Currently, the prompt is enclosed in parentheses, both in the venv and when storing:

PS N:\projects\cpython> python -m venv testvenv --prompt="  prompt with spaces   "
Running Release|Win32 interpreter...
PS N:\projects\cpython> .\testvenv\Scripts\activate
(  prompt with spaces   ) PS N:\projects\cpython>

Contents of pyvenv.cfg:
home = N:\projects\cpython\PCbuild\win32
include-system-site-packages = false
version = 3.8.0
prompt = (  prompt with spaces   ) 

I also tested using quotes in the prompt, which lead to opening issue 35667.  

Here's a prompt that itself has parentheses:
PS N:\projects\cpython> python -m venv testvenv --prompt="(my prompt)"
Running Release|Win32 interpreter...
PS N:\projects\cpython> .\testvenv\Scripts\activate
((my prompt)) PS N:\projects\cpython>

It seemed to me that the parenthesis would allow the retrieval of both the original prompt definition and how it looked in display.  Maybe my tests didn't demonstrate this clearly?
msg333435 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-01-11 04:39
Your tests actually went the other way. This one puts a trailing space after the prompt and expected it to be there in the file, but also ensured that exactly one space was added at the start.

        self.assertEqual(context.prompt, '(My prompt) ')
        builder.create(self.env_dir)
        data = self.get_text_file_contents('pyvenv.cfg')
        self.assertIn('prompt = (My prompt) \n', data)

In the line "prompt = (My prompt) ", we need to define which of the spaces at either end of the value should be retained by tools trying to use the same prompt. The obvious assumption is to strip both ends, which would break your test. The next most obvious is to quote the string value (e.g. write repr(context.prompt) instead of the prompt directly).

Also, I'm pretty sure I saw some advice once to use multiple lines worth of prompt, which probably breaks many shells but presumably works okay in Bash. You'll need to handle that case as well - again, repr() will do it naturally by encoding newlines an "\n".

Anyone reading the string will have to do the equivalent of Python's literal_eval(), but that's pretty manageable in exchange for avoiding a whole lot of edge cases.
msg333647 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2019-01-14 23:10
Steve, thanks for explaining that.  I understand what you're saying now.  It's funny but because the context prompt has the space added at the end when it's created (`context.prompt = '(%s) ' % prompt`), I added it to the test, but completely forgot about it.  I was thinking about just getting back what was inside the parentheses without considering at all what the whole line looked like.
msg333717 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-01-15 17:44
First, Cheryl, thanks for taking this on!

I think one way to potentially simplify this whole situation about the whitespace for the prompt is to actually store the raw value that gets passed into EnvBuilder instead of the prompt as formatted for the activation scripts. I personally only want that initial value anyway and not the formatted version for the prompt for us in VS Code. Plus if we document that the value that we save in the pyvenv.cfg will be stripped then that should help with this.

Otherwise I say go with the repr as Steve suggested, but I would still like to have access to the unformatted value (and probably not bother setting it if a custom value isn't provided to facilitate relocating venvs).
msg333721 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-01-15 18:04
One other aspect of this may be the confusion that ensues when changing the setting doesn't change the prompt when you activate it.

It would be possible (though not necessarily trivial) to update the activate scripts to read the prompt from the file, though I don't think it's necessary. But we'll get a bug report sooner or later anyway.

Maybe if the setting is called "provided-custom-prompt" that will imply enough that it's a optional record of the prompt rather than an active configuration setting?
msg334174 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2019-01-21 20:13
I've made the changes suggested by Brett to store the raw value instead of the formatted prompt.  

I also have been looking into issues Steve raised about using the prompt value from `pyvenv.cfg` instead of having the prompt hard-coded into the activate scripts.  Since these scripts are .bat, bash, Powershell, etc scripts, I didn't know if there was anything special that needed to be taken into account (such as encoding) to use the values from the config file.  As Steve said, I could see this being opened as a bug in the future, so I don't mind working on it now in this issue or a separate one, whichever is the better option.  I just don't have much experience with those types of scripts, so I'm sure any changes I make will require some time and a lot of code review.  Plus, those changes add complexity to this change that Brett said he doesn't really need for his initial request.  Because of the possibility of changing those scripts soon, I've left the key name as `prompt` for now.  Fixing the activate scripts seems to be the right way to go instead of changing the name so it's not misused.

One thing that I did find which may be a current bug is:
> PS N:\projects\cpython> python -m venv myvenv --prompt='this is my prompt'
> Running Release|Win32 interpreter...
> PS N:\projects\cpython> .\myvenv\Scripts\activate
> (this is my prompt) PS N:\projects\cpython> deactivate
> PS N:\projects\cpython> python -m venv myvenv --upgrade --prompt='new prompt'
> Running Release|Win32 interpreter...
> PS N:\projects\cpython> .\myvenv\Scripts\activate
> (this is my prompt) PS N:\projects\cpython>

--upgrade changes the prompt on `pyvenv.cfg`, but doesn't update activate scripts, so the prompt doesn't really change.

And for reference, there is another bug issue for storing the --prompt value in an environment variable (#35328).
msg335562 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-02-14 20:58
I think upgrading the scripts to read from pyvenv.cfg is a separate issue per activation script (e.g. an issue for PowerShell, an issue for fish, etc.) as this could be done piecemeal.

One benefit to doing this is it would help move towards the activation scripts becoming static and thus skipping the string substitution when writing the files to disk.
msg337538 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2019-03-08 22:01
New changeset d5a70c6b0355f247931f6be80b78a0ff1869c56f by Cheryl Sabella in branch 'master':
bpo-35661: Store the venv prompt in pyvenv.cfg (GH-11440)
https://github.com/python/cpython/commit/d5a70c6b0355f247931f6be80b78a0ff1869c56f
msg337769 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-03-12 16:02
This issue broke x86 Gentoo Installed with X 3.x buildbot:

https://buildbot.python.org/all/#/builders/103/builds/2208

ERROR: test_prompt (test.test_venv.BasicTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.8/test/test_venv.py", line 123, in test_prompt
    builder.create(self.env_dir)
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.8/venv/__init__.py", line 70, in create
    self.setup_scripts(context)
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.8/venv/__init__.py", line 278, in setup_scripts
    self.install_scripts(context, path)
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.8/venv/__init__.py", line 354, in install_scripts
    with open(dstfile, 'wb') as f:
PermissionError: [Errno 13] Permission denied: '/buildbot/tmp/tmpdir/tmph7_oob5e/bin/Activate.ps1'

Please either fix or revert the change.
msg337770 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-03-12 16:06
Presumably the issue is in the test and not the main change itself.

Is this another case where Linux can't run venv tests inside a venv?
msg337777 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2019-03-12 16:50
Sorry about that.  I'll work on a fix and test it on the buildbots first.  

There are some tests with pyvenv.cfg that are skipped if it happens in a venv (@skipInVenv decorator), but the test right above this one (test_defaults) does not use the decorator and it checks pyvenv.cfg, so I think test_prompt might just need to do some cleanup first.
msg337787 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-03-12 18:48
Maybe you can just call builder.create_configuration() from the test and pass in enough context that it doesn't have to create a real venv? That would also make the test faster.
msg337788 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-03-12 18:50
It may also be that the directory exists already - the previous test explicitly removes it before calling create().
msg337815 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2019-03-12 22:41
Steve, thanks for the suggestions.  

Looking at the other tests, `test_prompt` was the only one (prior to the original change) that didn't create a venv, so maybe actually creating the venv instead of just instantiating the class were testing that everything worked together?  For example, see `test_isolation` and `test_symlinking`.  

I've added the code to explicitly remove the directory and I also slightly reorganized the create.  Not all the tests use the `run_with_capture`, but at the same time, none of the ones that use it do anything with the output, so I thought it wouldn't hurt to add it.

I tried to submit the tests just to the buildbots based on the devguide, but they were failing for other reasons, so I made the PR.  I don't think we'll know if this fixes the bots until the PR is merged, but if one of you could take a look and approve it, I'd appreciate it.  And then I'll keep my fingers crossed.   :-)
msg337817 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-03-12 23:18
The PR looks good to me.

Are you able to test this test running from in a venv? I'm not at all clear why it would work there when the others fail (there's another issue looking at this, since it's only Linux that doesn't support venv-in-venv, but the tests are mostly all skipped anyway)
msg337822 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2019-03-13 00:14
Thanks Steve.

The venv tests run for me from inside a venv on Windows 10 from a command prompt.  From Powershell, all the tests fail with the following when run from inside a venv:
FileNotFoundError: [Errno 2] No such file or directory: 'N:\\projects\\cpython\\lib\\venv\\scripts\\nt\\python.exe'

I haven't tried on linux.
msg337823 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2019-03-13 00:15
New changeset 839b925f6347222de560cdb767924c502b4039aa by Cheryl Sabella in branch 'master':
bpo-35661: Fix failing test on buildbot (GH-12297)
https://github.com/python/cpython/commit/839b925f6347222de560cdb767924c502b4039aa
msg337825 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-03-13 00:18
Yeah, that failure on Windows is due to not correctly detecting a venv made from a build tree, rather than a venv from a proper install. I have a fix for that in the other bug. If it got that far, it means it used the correct prefix path, so it'll be fine.
msg337841 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2019-03-13 10:37
Looks like the buildbot is happy now.  Thank you everyone!
History
Date User Action Args
2019-03-13 10:37:42cheryl.sabellasetstatus: open -> closed

resolution: fixed
messages: + msg337841
keywords: patch, patch, patch
2019-03-13 00:18:52steve.dowersetkeywords: patch, patch, patch

messages: + msg337825
2019-03-13 00:15:49cheryl.sabellasetmessages: + msg337823
2019-03-13 00:14:33cheryl.sabellasetkeywords: patch, patch, patch

messages: + msg337822
2019-03-12 23:18:33steve.dowersetkeywords: patch, patch, patch

messages: + msg337817
2019-03-12 22:41:18cheryl.sabellasetkeywords: patch, patch, patch
assignee: cheryl.sabella -> brett.cannon
messages: + msg337815

stage: patch review -> resolved
2019-03-12 22:33:11cheryl.sabellasetstage: resolved -> patch review
pull_requests: + pull_request12274
2019-03-12 19:59:09brett.cannonsetkeywords: patch, patch, patch
assignee: brett.cannon -> cheryl.sabella
2019-03-12 18:50:02steve.dowersetkeywords: patch, patch, patch

messages: + msg337788
2019-03-12 18:48:59steve.dowersetkeywords: patch, patch, patch

messages: + msg337787
2019-03-12 16:50:28cheryl.sabellasetkeywords: patch, patch, patch

messages: + msg337777
2019-03-12 16:06:12steve.dowersetkeywords: patch, patch, patch

messages: + msg337770
2019-03-12 16:02:09vstinnersetstatus: closed -> open

nosy: + vstinner
messages: + msg337769

keywords: patch, patch, patch
resolution: fixed -> (no value)
2019-03-08 22:02:44cheryl.sabellasetkeywords: patch, patch, patch
status: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-03-08 22:01:29cheryl.sabellasetmessages: + msg337538
2019-02-14 20:58:27brett.cannonsetkeywords: patch, patch, patch

messages: + msg335562
2019-01-21 20:13:50cheryl.sabellasetkeywords: patch, patch, patch

messages: + msg334174
2019-01-15 18:04:27steve.dowersetkeywords: patch, patch, patch

messages: + msg333721
2019-01-15 17:44:28brett.cannonsetkeywords: patch, patch, patch

messages: + msg333717
2019-01-14 23:10:16cheryl.sabellasetkeywords: patch, patch, patch

messages: + msg333647
2019-01-11 04:39:54steve.dowersetkeywords: patch, patch, patch

messages: + msg333435
2019-01-10 13:08:51cheryl.sabellasetkeywords: patch, patch, patch

messages: + msg333377
2019-01-09 23:50:33steve.dowersetkeywords: patch, patch, patch
nosy: + cheryl.sabella, steve.dower
messages: + msg333354

2019-01-05 21:21:55cheryl.sabellasetpull_requests: - pull_request10883
2019-01-05 21:21:47cheryl.sabellasetpull_requests: - pull_request10882
2019-01-05 21:19:20cheryl.sabellasetkeywords: + patch
stage: test needed -> patch review
pull_requests: + pull_request10883
2019-01-05 21:19:15cheryl.sabellasetkeywords: + patch
stage: test needed -> test needed
pull_requests: + pull_request10882
2019-01-05 21:19:09cheryl.sabellasetkeywords: + patch
stage: test needed -> test needed
pull_requests: + pull_request10881
2019-01-05 00:58:27brett.cannonsetversions: + Python 3.8
2019-01-05 00:58:17brett.cannonsetpriority: normal -> low
2019-01-05 00:58:02brett.cannoncreate