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.

Sign up for GitHub

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
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
030bf73
Skip tests made obsolete by more strict device name handling in Windows
jkloth Oct 3, 2021
2619eab
Reference issue in comments
jkloth Oct 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions Lib/test/test_winconsoleio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,11 @@ def test_open_name(self):
f.close()
f.close()

f = open('C:/con', 'rb', buffering=0)
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.

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.

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.


@unittest.skipIf(sys.getwindowsversion()[:2] <= (6, 1),
"test does not work on Windows 7 and earlier")
Expand All @@ -114,7 +116,8 @@ def test_conout_path(self):
conout_path = os.path.join(temp_path, 'CONOUT$')

with open(conout_path, 'wb', buffering=0) as f:
if sys.getwindowsversion()[:2] > (6, 1):
# bpo-45354: Windows 11 changed MS-DOS device name handling
if (6, 1) < sys.getwindowsversion()[:3] < (10, 0, 22000):
self.assertIsInstance(f, ConIO)
else:
self.assertNotIsInstance(f, ConIO)
Expand Down