msg333030 - (view) |
Author: Brett Cannon (brett.cannon) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2019-03-13 10:37 |
Looks like the buildbot is happy now. Thank you everyone!
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:09 | admin | set | github: 79842 |
2019-03-13 10:37:42 | cheryl.sabella | set | status: open -> closed
resolution: fixed messages:
+ msg337841 keywords:
patch, patch, patch |
2019-03-13 00:18:52 | steve.dower | set | keywords:
patch, patch, patch
messages:
+ msg337825 |
2019-03-13 00:15:49 | cheryl.sabella | set | messages:
+ msg337823 |
2019-03-13 00:14:33 | cheryl.sabella | set | keywords:
patch, patch, patch
messages:
+ msg337822 |
2019-03-12 23:18:33 | steve.dower | set | keywords:
patch, patch, patch
messages:
+ msg337817 |
2019-03-12 22:41:18 | cheryl.sabella | set | keywords:
patch, patch, patch assignee: cheryl.sabella -> brett.cannon messages:
+ msg337815
stage: patch review -> resolved |
2019-03-12 22:33:11 | cheryl.sabella | set | stage: resolved -> patch review pull_requests:
+ pull_request12274 |
2019-03-12 19:59:09 | brett.cannon | set | keywords:
patch, patch, patch assignee: brett.cannon -> cheryl.sabella |
2019-03-12 18:50:02 | steve.dower | set | keywords:
patch, patch, patch
messages:
+ msg337788 |
2019-03-12 18:48:59 | steve.dower | set | keywords:
patch, patch, patch
messages:
+ msg337787 |
2019-03-12 16:50:28 | cheryl.sabella | set | keywords:
patch, patch, patch
messages:
+ msg337777 |
2019-03-12 16:06:12 | steve.dower | set | keywords:
patch, patch, patch
messages:
+ msg337770 |
2019-03-12 16:02:09 | vstinner | set | status: closed -> open
nosy:
+ vstinner messages:
+ msg337769
keywords:
patch, patch, patch resolution: fixed -> (no value) |
2019-03-08 22:02:44 | cheryl.sabella | set | keywords:
patch, patch, patch status: open -> closed resolution: fixed stage: patch review -> resolved |
2019-03-08 22:01:29 | cheryl.sabella | set | messages:
+ msg337538 |
2019-02-14 20:58:27 | brett.cannon | set | keywords:
patch, patch, patch
messages:
+ msg335562 |
2019-01-21 20:13:50 | cheryl.sabella | set | keywords:
patch, patch, patch
messages:
+ msg334174 |
2019-01-15 18:04:27 | steve.dower | set | keywords:
patch, patch, patch
messages:
+ msg333721 |
2019-01-15 17:44:28 | brett.cannon | set | keywords:
patch, patch, patch
messages:
+ msg333717 |
2019-01-14 23:10:16 | cheryl.sabella | set | keywords:
patch, patch, patch
messages:
+ msg333647 |
2019-01-11 04:39:54 | steve.dower | set | keywords:
patch, patch, patch
messages:
+ msg333435 |
2019-01-10 13:08:51 | cheryl.sabella | set | keywords:
patch, patch, patch
messages:
+ msg333377 |
2019-01-09 23:50:33 | steve.dower | set | keywords:
patch, patch, patch nosy:
+ cheryl.sabella, steve.dower messages:
+ msg333354
|
2019-01-05 21:21:55 | cheryl.sabella | set | pull_requests:
- pull_request10883 |
2019-01-05 21:21:47 | cheryl.sabella | set | pull_requests:
- pull_request10882 |
2019-01-05 21:19:20 | cheryl.sabella | set | keywords:
+ patch stage: test needed -> patch review pull_requests:
+ pull_request10883 |
2019-01-05 21:19:15 | cheryl.sabella | set | keywords:
+ patch stage: test needed -> test needed pull_requests:
+ pull_request10882 |
2019-01-05 21:19:09 | cheryl.sabella | set | keywords:
+ patch stage: test needed -> test needed pull_requests:
+ pull_request10881 |
2019-01-05 00:58:27 | brett.cannon | set | versions:
+ Python 3.8 |
2019-01-05 00:58:17 | brett.cannon | set | priority: normal -> low |
2019-01-05 00:58:02 | brett.cannon | create | |