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

Created on 2019-01-05 00:58 by brett.cannon, last changed 2019-01-21 20:13 by cheryl.sabella.

Pull Requests
URL Status Linked Edit
PR 11440 open cheryl.sabella, 2019-01-05 21:19
Messages (8)
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 triager) 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) ')
        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 triager) 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 triager) 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).
Date User Action Args
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