Skip to content

gh-118234: Document Python subprocess requirement on SystemRoot env, add RuntimeWarning #134435

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jhohm
Copy link
Contributor

@jhohm jhohm commented May 21, 2025

Document thatSystemRoot is also required for a Python subprocess that uses socket or asyncio on Windows, and add a RuntimeWarning if an env supplied to subprocess.Popen lacks it.

Resolves #118234.


📚 Documentation preview 📚: https://cpython-previews--134435.org.readthedocs.build/

@jhohm
Copy link
Contributor Author

jhohm commented May 21, 2025

Apparently some regression tests trigger this: https://github.com/python/cpython/actions/runs/15171090613/job/42661034135

Even though test.libregrtest.worker.create_worker_process initilaizes env = dict(os.environ).

Does the Windows CI runner not set SystemRoot?

@vstinner
Copy link
Member

Does the Windows CI runner not set SystemRoot?

You can check "Display build info" which says:

os.environ[SYSTEMROOT]: C:\Windows

@zooba
Copy link
Member

zooba commented May 21, 2025

Does the Windows CI runner not set SystemRoot?

I'd be suspicious of Lib\test\libregrtest\save_env.py and get_os_environ and restore_os_environ. A bit of logging in those might show a place where it's not resetting things properly (even if it resets them properly later).

@gpshead gpshead assigned zooba and unassigned gpshead May 22, 2025
@@ -1545,6 +1545,10 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
if cwd is not None:
cwd = os.fsdecode(cwd)

if env is not None and not any(
k.upper() == 'SYSTEMROOT' and v for k, v in env.items()):
warnings.warn("env lacks a valid 'SystemRoot'.", RuntimeWarning)
Copy link
Member

Choose a reason for hiding this comment

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

I dislike this RuntimeWarning. It's ok to run programs without SystemRoot env var, or even in an empty environment. It just works fine currently for some programs. Example:

import subprocess, sys
cmd = [sys.executable, '-c', 'print("Hello World")']
subprocess.run(cmd, env={})

This program just works, it displays "Hello World" as expected.

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'm not sure how to synthesize this feedback with #134363 (comment).

How do we prevent the problem, if not warning about it, and not setting it in the subprocess? Should we implicitly override the caller's choices and clone SystemRoot into the subprocess env?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that we have to prevent the problem. We can document it and explain how to avoid it.

Copy link
Member

Choose a reason for hiding this comment

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

Well, it works since Windows decided that using PATH to locate runtime dependencies (like an LD_LIBRARY_PATH equivalent) was such a terrible idea that it bypasses it for most OS stuff, but that "fix" upsets people as well.

I'm not particularly strong either way on this fix. It bites people all the time, and it's practically always a pointless thing to do (pass an insufficient environment). At the same time, there are 5-6 other variables that also ought to be included or things will break weirdly (e.g. TEMP), and this change doesn't check for those. But I don't think we have any good way to only warn when things break.

Is there a silent-by-default-enabled-with-X-dev warning that could be used instead? Would that be okay, @vstinner?

Copy link
Member

Choose a reason for hiding this comment

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

If we add a warning, it may be better to emit the warning in the socket module, no?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a silent-by-default-enabled-with-X-dev warning that could be used instead? Would that be okay, @vstinner?

A warning only emitted in the Python Developer Mode (-X dev) sounds more acceptable to me, yes.

Copy link
Member

Choose a reason for hiding this comment

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

it may be better to emit the warning in the socket module, no?

The user's system could be configured to avoid that error, even without the environment variable, while plenty of other parts could also break if they are reconfigured.

Launching an application with an empty environment is the bad idea, so that's where the warning should be. I've diagnosed that as the root cause of a lot of issues.

Do we have an existing warning? Or do we need to invent a new one? This isn't a deprecation, and that's the only warning I'm aware of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sockets and asyncio on Windows depend on SYSTEMROOT env var
4 participants