Skip to content

bpo-41011: venv -- add more variables to pyvenv.cfg (GH-30382) #30382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Jan 7, 2022

Conversation

akulakov
Copy link
Contributor

@akulakov akulakov commented Jan 4, 2022

I'm not sure if these should be added to the docs -- there's no pyvenv.cfg section and two of the vars are currently documented in separate places and another two are not documented. It seems like someone needing them would look in pyvenv.cfg and find them; and their values are self-explanatory.

https://bugs.python.org/issue41011

@akulakov akulakov marked this pull request as ready for review January 4, 2022 05:32
@@ -178,6 +178,8 @@ def create_configuration(self, context):
f.write('version = %d.%d.%d\n' % sys.version_info[:3])
if self.prompt is not None:
f.write(f'prompt = {self.prompt!r}\n')
f.write('executable = %s\n' % context.env_exec_cmd)
f.write('command = %s\n' % sys.executable)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That (sys.executable) is not the command (-line) which was used to create the environment - it would be the python -m venv ... or equivalent command line which need to be recorded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would something like this work?

            args = ' '.join(sys.argv[1:])
            f.write(f'command = {sys.executable} -m {context.env_name} {args}\n')

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would something like this work?

Not necessarily. The code to create a venv might be called programmatically through the EnvBuilder API. The intention is to provide a command which, if run, produces a venv with the current settings. So the thing to do is:

  1. Run python3 -m venv -h to see what the command-line options are.
  2. Look at the existing code to see how those options map to attributes of the EnvBuilder which are used in making the venv.
  3. Looking at the EnvBuilder attributes of the current instance, work out what command-line would give those attributes, and synthesize the command line from there.

@@ -0,0 +1,3 @@
Added two new variables to *pyvenv.cfg* which is generated by :mod:`venv`
module: *executable* for the executable within the environment and *command*
for the command used to create the environment.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

command -> command line

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@@ -178,6 +178,8 @@ def create_configuration(self, context):
f.write('version = %d.%d.%d\n' % sys.version_info[:3])
if self.prompt is not None:
f.write(f'prompt = {self.prompt!r}\n')
f.write('executable = %s\n' % context.env_exec_cmd)
f.write('command = %s\n' % sys.executable)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would something like this work?

Not necessarily. The code to create a venv might be called programmatically through the EnvBuilder API. The intention is to provide a command which, if run, produces a venv with the current settings. So the thing to do is:

  1. Run python3 -m venv -h to see what the command-line options are.
  2. Look at the existing code to see how those options map to attributes of the EnvBuilder which are used in making the venv.
  3. Looking at the EnvBuilder attributes of the current instance, work out what command-line would give those attributes, and synthesize the command line from there.

@akulakov akulakov marked this pull request as draft January 5, 2022 03:44
@brettcannon brettcannon self-requested a review January 5, 2022 23:11
…t flag; expand test to check that no invalid flags are present when no args are passed to EnvBuilder
if self.upgrade_deps:
args.append('--upgrade-deps')
if self.orig_prompt is not None:
args.append(f'--prompt={self.orig_prompt}')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

                    prompt = f'"{self.orig_prompt}"' if ' ' in self.orig_prompt else self.orig_prompt
                    args.append(f'--prompt={prompt}')

to handle the case where self.orig_prompt has spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not enough to quote it on spaces, I quoted it unconditionally in case of chars like '*'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I just realized I did not commit that change.. pushing it now.

@akulakov
Copy link
Contributor Author

akulakov commented Jan 6, 2022

@vsajip I know the test got to be ugly and confusing, lmk if I can make it clearer...

@akulakov akulakov marked this pull request as ready for review January 6, 2022 15:29
@vsajip
Copy link
Member

vsajip commented Jan 6, 2022

I know the test got to be ugly and confusing, lmk if I can make it clearer...

Do you mean test_config_file_command_key?

@akulakov
Copy link
Contributor Author

akulakov commented Jan 6, 2022

Do you mean test_config_file_command_key?

Yes. It's also quite slow (creating new env for each flag), but checking all flags in one run would be less thorough, but it's an option we can consider.

@vsajip
Copy link
Member

vsajip commented Jan 6, 2022

Yes. It's also quite slow

How about mocking the pip installation part? Does that speed things up?

@akulakov
Copy link
Contributor Author

akulakov commented Jan 7, 2022

How about mocking the pip installation part? Does that speed things up?

Yep it does! Much faster now..

@vsajip vsajip changed the title bpo-41011: venv -- add more variables to pyvenv.cfg bpo-41011: venv -- add more variables to pyvenv.cfg (GH-30382) Jan 7, 2022
@vsajip vsajip merged commit f4e325c into python:main Jan 7, 2022
@akulakov
Copy link
Contributor Author

akulakov commented Jan 7, 2022

@vsajip thanks for many rounds of quick reviews!! :)

@@ -178,6 +179,29 @@ def create_configuration(self, context):
f.write('version = %d.%d.%d\n' % sys.version_info[:3])
if self.prompt is not None:
f.write(f'prompt = {self.prompt!r}\n')
f.write('executable = %s\n' % os.path.realpath(sys.executable))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I appreciate that this PR is long since merged, but just following-up on a surprising pyvenv.cfg file that I saw recently, and git-blamed it back to here)

I'm not at all sure why realpath is important here - the sys.executable should be good enough when being called directly. When called through the EnvBuilder, I don't see a reason for this either. 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it is a difference when you're not on Windows. But thinking about it, you could view the config as recording the actual interpreter used instead of the path that could change on you in the future (e.g., some symlink later gets changed to point to another interpreter).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this perspective (I hadn't considered it)! I'm not sure that using a config file to log information about how a venv was created is a good idea, but can appreciate the idea. Since this is a machine readable file, I would be concerned that it eventually gets used for non-log/audit purposes.

I've been digging into symlink issues with venv a lot recently, and think calling realpath is quite problematic in other context (i.e. when determining home). More details of that in #106045.

No follow-up needed here - just flagging it, and depending on the outcome of the linked issue, perhaps in the future I may request that this information become non realpath, or under a "history of how this venv was created" section of the config (perhaps in the form of a comment).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants