gh-152433: Windows: modernize and refactor fileutils to allow build for Windows UWP#153700
gh-152433: Windows: modernize and refactor fileutils to allow build for Windows UWP#153700thexai wants to merge 12 commits into
Conversation
|
Updated with more simplified and less invasive changes. |
57f8125 to
f0f837b
Compare
|
This breaks CI, e.g.
Can you reproduce this on a local Windows installation and fix? Regarding tests I have another question: are you able to run the tests on XBOX in general? I'd be curious in how many of them fail ... |
Reproduced and now should be fixed in latest PR update. 🙂 It's important to note that some tests fail because they expect specific responses to unusual situations that may not have a single logical outcome. For example: If a file is inaccessible (due to permissions), the parent directory is read, and if it's no longer available, the original error is not restored (instead of return access violation error, return file no exist error).
Also on UWP, due security restrictions (every App runs isolated) is not possible to access to some information in the file system like |
chris-eibl
left a comment
There was a problem hiding this comment.
LGTM, but I'd appreciate a second pair of eyes @python/windows-team.
zooba
left a comment
There was a problem hiding this comment.
Only left two comments, but the guts of it is, I think we can switch to FileStandardInfo for everyone, and that should simplify this change down to basically not needing the separate UWP path at all.
|
Updated PR with one simplification: UWP not support Lines 2027 to 2043 in 202c4c8 This allows remove part of UWP only code and some ifdef. |
|
Simplification 2: Merges |
Use same code for UWP and Desktop.
|
I think I've finished this! Minimal changes and the same common code for UWP and Desktop. Tests should pass... |
zooba
left a comment
There was a problem hiding this comment.
Apologies for lots of suggestions, but I'm excited about this cleanup! We're so close to really streamlining this code, so it'd be great to go the whole way.
My main target is to fully remove the BY_HANDLE_FILE_INFORMATION usage entirely, and also to minimise the logic in the to_stat function (more overwriting, less checking). The comments I've left should lead you there, but feel free to ignore them if you see another way.
|
Full remove |
zooba
left a comment
There was a problem hiding this comment.
Looks great overall, glad we kept going, this is really neat!
Still have a question about guessing at ino values when we don't know them.
| // use these fallback values for systems where it's not possible | ||
| // to obtain VolumeSerialNumber / FileId (e.g. Windows UWP) | ||
| result->st_dev = 1; | ||
| result->st_ino = basic_info->CreationTime.QuadPart; |
There was a problem hiding this comment.
Do you have a specific scenario for needing these to be non-zero but inaccurate?
They are documented as allowed to be zero if not obtainable (even though it turns out that samestat doesn't check that), but I think the risk of declaring two files to be the same when they aren't is worse than declaring the ino as unknown when it's unknown. So I'd prefer to leave them both at zero if there's no ID info.
There was a problem hiding this comment.
I don't have a concrete answer right now, I only added it because it was in the old UWP patches... I can remove it for now and add it back later if a specific reason comes up.
Windows: modernize and refactor fileutils to allow build for Windows UWP
This is probably the last relevant commit missing to allow compile all Python core for UWP.
GetFileInformationByHandleis not supported in UWP, then is replaced with more modernGetFileInformationByHandleExcalls.Also code refactored to avoid some duplicated code.Note that Xbox (all models) runs only on Windows 11 based OS. Then is guaranteed that "fast stat" code (based on
GetFileInformationByName) is supported:cpython/Modules/posixmodule.c
Line 2299 in 050a84b
"slow stat" code path is not much relevant for UWP but also works if replaced
CreateFileWwithCreateFile2(in other PR).