Skip to content

bpo-45354: Skip obsolete device name tests on Windows 11 #28712

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 2 commits into from
Oct 5, 2021

Conversation

jkloth
Copy link
Contributor

@jkloth jkloth commented Oct 3, 2021

@bedevere-bot bedevere-bot added the tests Tests in the Lib/test dir label Oct 3, 2021
f.close()
# Windows 11 changed MS-DOS device name handling
if sys.getwindowsversion()[:3] < (10, 0, 22000):
f = open('C:/con', 'rb', buffering=0)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does nt._getfullpathname('C:/con').lower() return in Windows 11? The test should be skipped if it's not r"\\.\con". Similarly, if nt._getfullpathname(conout_path).lower() isn't r"\\.\conout$", assume that conout_path is a regular file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both calls return a normalized pathname, not the device object name.

That said, as the tests in question are originally from you, shouldn't the tests then be written in terms of _getfullpathname() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Meaning, instead of using open(), just check the results of _getfullpathname()

Copy link
Contributor

Choose a reason for hiding this comment

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

It's checking _PyIO_get_console_type() in "Modules/_io/winconsoleio.c", which may need to be modified. Please also check nt._getfullpathname() and nt._getfinalpathname() (may fail) on the unqualified base names "con", "conin$", and "conout$". These names resolve to the console in previous Windows versions, so _PyIO_get_console_type() skips calling GetFullPathNameW(). This optimization may need to be removed if the API no longer special cases the base names.

Copy link
Contributor

Choose a reason for hiding this comment

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

I verified the new behavior in Windows 11. If the path is qualified (i.e. contains slashes), only the "nul" device name appears to be reserved (e.g. "./nul" -> "\\.\nul"). The other DOS device names aren't reserved in a qualified path (e.g. "./com1" is just a regular file in the current working directory). DOS device names are still reserved in an unqualified relative path, but extensions are no longer ignored (e.g. "con.txt" is just a regular file).

This means that _PyIO_get_console_type() doesn't need to be modified. It only special cases an exact case-insensitive match of "con", "conin$", and "conout$", which are still reserved paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sooooo..... does this mean that these changes are suitable? Does it just need an OK from @zooba or @zware? I'm only slightly impatient here as this is blocking the new buildbot from being actually useful.

if sys.getwindowsversion()[:3] < (10, 0, 22000):
f = open('C:/con', 'rb', buffering=0)
self.assertIsInstance(f, ConIO)
f.close()
Copy link
Member

Choose a reason for hiding this comment

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

Can you please modify the test to use "with f: ..."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you notice, the rest of the test cases do not do that. Changing to context managers should probably be done as another issue. The only change here is moving the existing test into the if-block.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Leave it as it is.

f = open('C:/con', 'rb', buffering=0)
self.assertIsInstance(f, ConIO)
f.close()
# Windows 11 changed MS-DOS device name handling
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a reference to the bpo here and in the other modified test?

Suggested change
# Windows 11 changed MS-DOS device name handling
# bpo-45354: Windows 11 changed MS-DOS device name handling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in latest commit

@zooba zooba added the skip news label Oct 5, 2021
self.assertIsInstance(f, ConIO)
f.close()
# bpo-45354: Windows 11 changed MS-DOS device name handling
if sys.getwindowsversion()[:3] < (10, 0, 22000):
Copy link
Member

Choose a reason for hiding this comment

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

I suspect we'll want this to become a constant in the test suite soon enough, but doesn't have to be in this PR. Let's just keep an eye out for the next time it gets copied somewhere and then insist it gets centralised.

@zooba
Copy link
Member

zooba commented Oct 5, 2021

Ignoring the x86 test failure - it's clearly unrelated to this change.

@zooba zooba merged commit de4052f into python:main Oct 5, 2021
@zooba zooba added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Oct 5, 2021
@miss-islington
Copy link
Contributor

Thanks @jkloth for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @jkloth for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 5, 2021
@bedevere-bot
Copy link

GH-28736 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Oct 5, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 5, 2021
@bedevere-bot
Copy link

GH-28737 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Oct 5, 2021
zooba pushed a commit that referenced this pull request Oct 5, 2021
miss-islington added a commit that referenced this pull request Oct 5, 2021
@jkloth jkloth deleted the issue45354 branch April 5, 2022 22:28
w-hsiung pushed a commit to w-hsiung/cpython that referenced this pull request Dec 18, 2022
w-hsiung pushed a commit to w-hsiung/cpython that referenced this pull request Jun 8, 2023
w-hsiung pushed a commit to w-hsiung/cpython that referenced this pull request Aug 29, 2023
w-hsiung pushed a commit to w-hsiung/cpython that referenced this pull request Sep 4, 2023
w-hsiung pushed a commit to w-hsiung/cpython that referenced this pull request Sep 5, 2023
w-hsiung pushed a commit to w-hsiung/cpython that referenced this pull request Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants