GeezerVison

Why was this merged with a failing check?

Why was this merged with a failing check?

The failing test it unrelated to my PR. Victor reverted the commit that caused the test regression in GH-30632.

The failing test it unrelated to my PR. Victor reverted the commit that caused the test regression in GH-30632.

I'd recommend always saying why you are merging if a check is failing, it helps narrow down the failure when looking at the merge history.

I'll note that the last change here (GH-9053) specifically moved away from doing imports based on sys.platform == 'win32', so it's a bit disappointing to see that come back :)

Yeah, based on Zach's older PR and issue that resolved this should probably be reworked to a different approach.

Yeah, based on Zach's older PR and issue that resolved this should probably be reworked to a different approach.

The old approach gave a confusing exception when neither msvcrt nor _posixsubprocess is available. Should I revert my change until we have a better solution?

I won't push for a revert yet, but I also wouldn't like to see this version in a final release.

I'm afraid there's not a perfect solution here: the behavior should be based on what can be imported, which means at least one platform is going to have to raise and handle a ModuleNotFoundError. We could go a little crazy and guess at which imports should be tried first based on sys.platform (or 'msvcrt' in sys.builtin_module_names?), but I'm not sure how much effort it's worth to avoid the exception.

What was the exception when both are missing, and how was it confusing?

The exception was confusing because it was a nested exception that referred to msvcrt as primary cause of an import error.

How do you like #30930 ?