gh-102255: Improve build support on xbox

[maxbachmann]

• Issue: Improve build support on xbox #102255

[zooba]

This looks like it's ready, or very close. @maxbachmann, @eryksun is there anything left to do here? I don't mind it being done in future PRs if they're necessary.

[maxbachmann]

I am done with everything I planned for this PR. As suggested by @eryksun I plan to extend the sys module to retrieve the api partition. I will add this in a separate PR.

[zooba]

I plan to extend the sys module to retrieve the api partition. I will add this in a separate PR.

Give that a new issue as well. We really ought to add the platform to sys.implementation as well, since currently it's a bit weird to check for 32-bit vs 64-bit vs ARM64 as well (sys.winver is the only way). I assume that would be where the API partition is indicated, too.

[eryksun]

The value of bufsize isn't limited. The result length may be limited. If PATHCCH_ALLOW_LONG_PATHS isn't set in flags, and new_len >= MAX_PATH (not MAXPATHLEN), the returned error should be HRESULT_FROM_WIN32(ERROR_FILENAME_EXCED_RANGE).

Else if new_len is greater than bufsize, the returned error should be HRESULT_FROM_WIN32(ERROR_INSUFFICIENT_BUFFER).

PATHCCH_ALLOW_LONG_PATHS also determines whether to convert to or from an extended path, depending on whether the current process supports long paths (Python 3.6+ does). But it's okay to omit that behavior for GAMES.

[zooba]

Oh yeah, we shouldn't have references to MAXPATHLEN in here.

I'm not so concerned about error codes. They're only going to be reported to users, and really we ought to be getting the buffer right ourselves before calling.

I think it's fine for this implementation to assume PATHCCH_ALLOW_LONG_PATHS. Don't worry about mimicking every possible behaviour of the original.

[eryksun]

Suggested change

	if (bufsize >= MAXPATHLEN || new_len > bufsize) {
	if (new_len > bufsize) {

---

Don't worry about mimicking every possible behaviour of the original.

I agree, if getting the correct behavior is excessively complicated with no tangible benefit. What about my other comment on this function? The way PathCchStripToRoot() is used, when relfile is a rooted path, leads to incorrect and inconsistent behavior. If I know I'm combining a rooted path, I may use a smaller buffer because I know that most of dirname won't be copied -- just the drive part. The current implementation requires using a buffer that's big enough to hold dirname, relname and a joining backslash. OTOH, if relname has a drive or UNC share and thus replaces dirname, then the length of the passed-in dirname is ignored.

[zooba]

Firstly, the suggested change is good.

I'm not particularly worried about the case where relname would shorten the overall drive. It seems something you could take advantage of, but generally we don't control either half of the paths that will come in here, which means the caller is going to have to assume worst case of plain-old concatenation.

Maybe just add a comment like /* assuming worst case result length, so our caller should as well */.

The only reasonable alternative (without complicating everything) would involve modifying the result buffer before potentially returning an error, which I think is worse. I'm pretty sure the original API uses temporary buffers internally, but there's no real need to go there.

[maxbachmann]

Yes I would assume the implementation to use something like PathAllocCombine internally

[maxbachmann]

If we really wanted to I could come up with an implementation that behaves closer to PathCchCombineEx. I was simply not sure it was worth the complexity (I kind of hope that at some point microsoft makes these APIs available on the xbox, since there does not appear to be any inherent reason to provide them on every platform except the xbox)

[eryksun]

The only reasonable alternative (without complicating everything) would involve modifying the result buffer before potentially returning an error, which I think is worse. I'm pretty sure the original API uses temporary buffers internally, but there's no real need to go there.

It would calculate dir_len based on PathCchSkipRoot() if relfile begins with a backslash. Then it's the same initial test based on new_len = dir_len + file_len + 2, requiring new_len < bufsize. Copying dirname would now use wcsncpy(buffer, dirname, dir_len), and PathCchStripToRoot() would not have to be implemented. So you get better behavior AND it's simpler overall.

[zooba]

Hold off on any more changes on this function for a day or two - I'm having some interesting conversations with colleagues right now. Might have a more suitable approach (or at least more information about it).

[zooba]

Okay, thanks for holding. What I'm hearing from my colleagues at Microsoft is that the PathCch* APIs are implemented in the Xbox OS, just not exposed through the partition or their import lib. It sounds like both of these will be fixed for an update later this year, at which point our original code will be fine. The only restriction seems to be that the APIs are not implemented on Win7, so even though games are meant to be compatible all the way back, if we were to use the API then it would not work.

Until the GDK update arrives, it seems like we can probably use code like what we used to have to load the API dynamically (from getpathp.c in the 3.7 branch):

static int _PathCchCombineEx_Initialized = 0;
typedef HRESULT(__stdcall *PPathCchCombineEx) (PWSTR pszPathOut, size_t cchPathOut,
                                               PCWSTR pszPathIn, PCWSTR pszMore,
                                               unsigned long dwFlags);
static PPathCchCombineEx _PathCchCombineEx;


static void
join(wchar_t *buffer, const wchar_t *stuff)
{
    if (_PathCchCombineEx_Initialized == 0) {
        HMODULE pathapi = LoadLibraryExW(L"api-ms-win-core-path-l1-1-0.dll", NULL,
                                         LOAD_LIBRARY_SEARCH_SYSTEM32);
        if (pathapi) {
            _PathCchCombineEx = (PPathCchCombineEx)GetProcAddress(pathapi, "PathCchCombineEx");
        }
        else {
            _PathCchCombineEx = NULL;
        }
        _PathCchCombineEx_Initialized = 1;
    }


    if (_PathCchCombineEx) {
        if (FAILED(_PathCchCombineEx(buffer, MAXPATHLEN+1, buffer, stuff, 0))) {
            Py_FatalError("buffer overflow in getpathp.c's join()");
        }
    } else {
        if (!PathCombineW(buffer, buffer, stuff)) {
            Py_FatalError("buffer overflow in getpathp.c's join()");
        }
    }
}

@maxbachmann Would you be able to give this approach a try and see if it works? Unfortunately, I don't think there's any way to detect the version of the games SDK involved, so we'd just have to keep this until we assume everyone's on the fixed update.

If it helps (and I suspect it won't), it also ought to be okay to temporarily define the PARTITION constants needed when including pathcch.h. It's more likely going to be better to exclude the include statement in GAMES when we're defining a GetProcAddress wrapper, since that way we can define our own PathCchCombineEx implementation and it shouldn't collide with the real one until we choose to let it. You can probably even keep the existing implementation as a fallback for the (hopefully rare) cases where games may be running on Win7, but I'm happy to defer to you on that one, as you've likely got a better view of the gamedev landscape than I do.

[maxbachmann]

Okay, thanks for holding. What I'm hearing from my colleagues at Microsoft is that the PathCch* APIs are implemented in the Xbox OS, just not exposed through the partition or their import lib. It sounds like both of these will be fixed for an update later this year, at which point our original code will be fine.

Yes I did read in the developer forums, that there is already an implementation as well. Great to hear that they are likely going to get exposed at some point.

Would you be able to give this approach a try and see if it works? Unfortunately, I don't think there's any way to detect the version of the games SDK involved, so we'd just have to keep this until we assume everyone's on the fixed update.

just gave this a quick test and it works both on the Xbox One and Xbox Scarlett.

You can probably even keep the existing implementation as a fallback for the (hopefully rare) cases where games may be running on Win7, but I'm happy to defer to you on that one, as you've likely got a better view of the gamedev landscape than I do.

I do not think we need to take Windows 7 into consideration here:

1. Cpython overall does not support Windows 7 anymore
2. when building games for the PC you can always build against the desktop partition. Right now you only really need to use the games partition on the xbox, which does not use Windows 7 anyways.
3. even on PC the share of Windows 7 users is really slim. According to the current steam hardware survey around 1.5% of Windows users is on Windows 7: https://store.steampowered.com/hwsurvey/directx/

[zooba]

!buildbot .Windows.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @zooba for commit 331b1f4 🤖

The command will test the builders whose names match following regular expression: .*Windows.*

The builders matched are:

• AMD64 Windows10 PR
• ARM64 Windows Non-Debug PR
• AMD64 Windows10 Pro PR
• ARM64 Windows PR

[zooba]

There we go. I expect there'll be no buildbot issues, but worth double checking (there are some unrelated stack issues, so may still fail, but we can ignore those).

Other than that, I'm happy to merge this whenever. @eryksun I'll wait for a positive signal from you.

[eryksun]

@zooba, since the check for building winreg and mmap occurs in several places (here, in "PC/config.c", and the respective modules), maybe HAVE_WINDOWS_REG_API and HAVE_WINDOWS_MMAP_API should be defined in "PC/pyconfig.h". And to match that, maybe rename HAVE_WINDOWS_CONSOLE_IO to HAVE_WINDOWS_CONSOLE_API. That's a better name anyway, since GenerateConsoleCtrlEvent() is a console API, but not one that's particularly related to I/O.

[zooba]

The mmap API should lose the definitions fairly soon, we just spun that work out to another issue.

For winreg I wouldn't worry about pyconfig.h, but the definition could be calculated at the top of the file. Those aren't C API functions, so they can be detected by AttributeError.

None of these feel important enough to block on, but I wouldn't oppose doing them later. Let's call this PR done and let Max focus on the other changes that have been lined up

[zooba]

Thanks @maxbachmann! This was a huge effort, but I think it's worth it. Don't shy away from adjusting things we decided here if it seems important - nothing we changed should count as a public API change, so we can revert or modify if needed.