Skip to content
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

Update Windows CI tests to run on Python 3.12 #12562

Merged
merged 3 commits into from
Mar 10, 2024

Conversation

notatallshaw
Copy link
Member

My understanding is that Windows tests are slow, so the compromise is to run the tests on the newest and oldest versions that pip supports.

It seems the Windows tests were never updated to Python 3.12, this update that and adds a comment justifying this so when new versions of Python are added and removed from the CI hopefully the updater will notice this and update Windows as well.

@notatallshaw
Copy link
Member Author

Not sure if there is a "tests" news entry categiry, and if there is if it should be that or "trivial".

@notatallshaw
Copy link
Member Author

Exciting, there appears to be a Windows Python 3.12 specific failure, I will try and take a look this weekend.

@notatallshaw
Copy link
Member Author

notatallshaw commented Mar 9, 2024

Okay, I've found the issue, although getting tests to run locally on Windows is painful, and I still get more failures than in CI, but I always have.

The following test fails on Python 3.12 (I have tested 3.12.2 and 3.12.1) but not earlier versions of Python:
tests/functional/test_uninstall.py::test_uninstall_entry_point_colon_in_name[test_:test_ = distutils_install:test_test]

The exception comes from this call when there is a colon in the name:

shutil.move(old, new)

The end of the exception looks like this:

line 358, in renames
    shutil.move(old, new)
  File "...\Python\Python312\Lib\shutil.py", line 906, in move
    copy_function(src, real_dst)
  File "...\Python\Python312\Lib\shutil.py", line 460, in copy2
    _winapi.CopyFile2(src_, dst_, flags)
OSError: [WinError 87] The parameter is incorrect

I created a minimal reproducer:

Python 3.11:

>>> import shutil
>>> open('test_311:_test.txt', 'a').close()
>>> shutil.move('test_311:_test.txt', 'test_311_2:_test.txt')
'test_311_2:_test.txt'

Python 3.12:

>>> import shutil
>>> open('test_312:_test.txt', 'a').close()
>>> shutil.move('test_312:_test.txt', 'test_312_2:_test.txt')
Traceback (most recent call last):
  File "...\Python\Python312\Lib\shutil.py", line 874, in move
    os.rename(src, real_dst)
OSError: [WinError 87] The parameter is incorrect: 'test_312:_test.txt' -> 'test_312_2:_test.txt'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "...\Python\Python312\Lib\shutil.py", line 894, in move
    copy_function(src, real_dst)
  File "...\Python\Python312\Lib\shutil.py", line 448, in copy2
    _winapi.CopyFile2(src_, dst_, flags)
OSError: [WinError 87] The parameter is incorrect

So, I'm not sure what to do with this? Do I attempt to write a workaround in pip to handle renaming files on Windows with colons in them? Do I mark the test xfail in pip? Do I write a bug to CPython:?

@pfmoore
Copy link
Member

pfmoore commented Mar 9, 2024

I'd raise a bug with CPython. It's not a pip issue, as is demonstrated by the fact that your reproducer doesn't need pip. It's possibly a Windows filename limitation (colons usually delimit drive letters or NTFS stream components) but if that's the case, why did it work in 3.11. The error "The parameter is incorrect" is a Windows error, which also suggests a Windows-level problem.

Maybe shutil.move is implemented differently in 3.12, triggering the Windows issue? (Yes, I just checked the diff of shutil.py between 3.11 and 3.12 and there's definitely changes in there that might be relevant).

Regardless, we probably need to work around this in pip. If my suspicion is right, and it's the colons in the filename that are the issue, then as the test is explicitly looking at behaviour when there are colons in the name, I don't think we should simply ignore it. We either support the scenario being tested (in which case, we find a fix on Windows) or we don't (in which case the test shouldn't even be there).

@notatallshaw
Copy link
Member Author

This appears to be the causing CPython issue python/cpython#88745 and commit python/cpython@cda1bd3

It appears prior to Python 3.12 shutil.rename would ignore everything after a colon, which matches the behavior of the builtin open.

It seems a simple workaround could be in the util.misc.renames function to detect if on Windows and ignore everything after the :. I was going to ping Steve Dower, but I guess I should do that on the CPython issue.

@pfmoore
Copy link
Member

pfmoore commented Mar 9, 2024

The Windows documentation here points out that colon is a reserved character. And this document covers the use of colons when naming a stream within a file (streams are an very rarely used feature of NTFS where a file can have multiple named contents, similar to the old MacOS data and resource forks).

The entry point specification stops just short of saying that console_scripts entry points must be valid filenames, although it certainly implies it, and it does note that the entry point name will be used to construct a filename.

All of which is to say that I don't think the test is valid (specifically the part that creates a console script called test_:test_). We should probably just remove that case (on all platforms).

The part of the test that creates normal entry points ep:name1 and ep:name2 is fine, though - these aren't used as filenames.

@notatallshaw
Copy link
Member Author

notatallshaw commented Mar 9, 2024

Okay, updated.

Also I checked, and this used to be an xfail until ~two months ago: https://github.com/pypa/pip/pull/12493/files#diff-1a2d44e11ea700f3acffd1fc567470ee975b4ba7dcf421925087ebe652cdf7ba

cc @sbidoul if you have any opinions on this.

@notatallshaw notatallshaw force-pushed the update-windows-ci-test-to-Python-3.12 branch from 2dfdc47 to 876cfe3 Compare March 9, 2024 23:50
@sbidoul
Copy link
Member

sbidoul commented Mar 10, 2024

I'm fine with removing that test.

For completeness here is where it was marked xfail: #11871 (comment)

@sbidoul sbidoul merged commit a33caa2 into pypa:main Mar 10, 2024
24 checks passed
@notatallshaw notatallshaw deleted the update-windows-ci-test-to-Python-3.12 branch March 10, 2024 16:01
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants