Skip to content

Add ability to access env vars that are changed in test-related subprocesses #7082

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

Closed
duckinator opened this issue Sep 25, 2019 · 6 comments
Closed
Labels
auto-locked Outdated issues that have been locked by automation

Comments

@duckinator
Copy link
Contributor

duckinator commented Sep 25, 2019

What's the problem this feature will solve?

While I was working on #6391 to add a pip cache command, I ran into an issue where environment variables are changed in subprocesses started when a test uses script.pip(...) and similar, but I have no simple way to get this information. See #6391 (comment).

Here's @chrahunt's explanation from #6391 (comment):

The basic issue appears to be that changes to APPDATA and LOCALAPPDATA in the environment of the current process do not impact the returned value of ctypes.windll.shell32.SHGetFolderPathW. Only for sub-processes.

One option could be to take the current value of os.environ['LOCALAPPDATA'] and append \pip\Cache to it, duplicating the logic from appdirs.user_cache_dir.

Another option could be to get the cache dir from a sub-process itself, like script.run('python', '-c', 'from pip._internal.locations import USER_CACHE_DIR; print(USER_CACHE_DIR). This seems like more of a hack, but does avoid the duplication.

In either case, it may be good to make it a property of PipTestEnvironment so it's accessible from the required tests or any others that might need the cache directory.

Describe the solution you'd like

TBD — I'm not sure what approach would be best, and was hoping to get some input on that.

Additional context

PR where this problem came to my attention: #6391

Code where the environment variables are changed: https://github.com/pypa/pip/blob/master/tests/conftest.py#L98-L166

File where PipTestEnvironment, a potential location for tracking this information, is defined: https://github.com/pypa/pip/blob/master/tests/lib/__init__.py


cc @chrahunt

@ghost ghost added the S: needs triage Issues/PRs that need to be triaged label Sep 25, 2019
@duckinator
Copy link
Contributor Author

One approach would be to have PipTestEnvironment have a function which returns a dict mapping environment variable names to the new values, for anything that'd be overridden. Then, the current code in tests/conftest.py could iterate over the returned dict and apply the actual environment changes there.

I'm not sure if this is a good approach, however.

@duckinator duckinator changed the title Add ability to access environment changes in test-related subprocesses Add ability to access env vars that are changed in test-related subprocesses Sep 25, 2019
@duckinator duckinator mentioned this issue Sep 25, 2019
10 tasks
@pradyunsg
Copy link
Member

Whee! Thanks for filing a new issue for dedicated discussion! :)

get the cache dir from a sub-process itself, like script.run('python', '-c', 'from pip._internal.locations import USER_CACHE_DIR; print(USER_CACHE_DIR). This seems like more of a hack, but does avoid the duplication.

Let's just do this, I don't mind the slightly-fragile nature of this and we can revisit this later if needed.

@duckinator
Copy link
Contributor Author

@pradyunsg would it make sense to create a helper function that runs that, or do you think that should be delayed until it's needed at least twice?

@pradyunsg
Copy link
Member

pradyunsg commented Sep 27, 2019

Let's delay that -- I'm fairly averse to over-engineering things so I don't think there's much point in doing this eagerly.

@duckinator
Copy link
Contributor Author

@pradyunsg sounds good to me. In that case, should I go ahead and close this issue?

@pradyunsg
Copy link
Member

I did that. ;)

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Oct 28, 2019
@ghost ghost removed the S: needs triage Issues/PRs that need to be triaged label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

No branches or pull requests

2 participants