set VSCMD_ARG_TGT_ARCH based on targetted architecture

MusicalNinjaDad commented 3 months ago

This fixes #1861 and any other build setups which make use of setuptools._distutils.util.get_platform() on windows

Approach

Uses dict lookup pattern analog to many other places within the code
Only sets the variable if it has not already been set manually. Raises a FatalError if the variable is preset to a different value than expected.
Will overwrite a pre-set VSCMD_ARG_TGT_ARCH=""
Tests monkeypatch the environment before calling setuptools._distutils.util.get_platform() and validating returned platform

Test availability

Tests are set to only run on windows
monkeypatching os.name in a tight context worked locally (Win11 + WSL2 + Ubunutu 22.04), but fails in pipeline on github actions
all tests pass on github actions windows runner but tests targeting arm64 fail on azure pipeline. I'm assuming this is a weird, azure-specific thing in their environment and have set the arm64 test to skip on azure

Potential refactoring idea for a future PR

I'd like to refactor the various mapping lookups into a WindowsArchitecture dataclass - there are many places with the pattern somevar = architecture_mapping_dict[arch] which would be more readable as somevar = targetarchitecure.somevar

MusicalNinjaDad commented 3 months ago

Failing check on azure pipelines - linux caused by a timeout waiting to download docker image: docker: Error response from daemon: Get "[https://quay.io/v2/"](https://quay.io/v2/%22): net/http: request canceled (Client.Timeout exceeded while awaiting headers). logs

MusicalNinjaDad commented 3 months ago

@henryiii - I'd welcome your feedback on this suggestion please.

henryiii commented 3 months ago

I've always been in favor of this, it's just been others that are worried that setting this variable will make some build backend confused about not having the others.

henryiii commented 3 months ago

@joerick or @mayeut, thoughts?

mayeut commented 3 months ago

I don't know enough to really have an informed opinion here.

zooba commented 2 months ago

This seems harmless, provided we aren't replacing an existing value for the variable. If it's been set, we have to assume that cross-compilation is configured across a range of settings (particularly LIB and PATH environment variables) and bail out (or use it to automatically select the set of builds to run...?)

Other than that, seems fine. I don't think anything but setuptools takes this into account, but it's as "standard" as things get. Anything else will be backend-specific.

MusicalNinjaDad commented 2 months ago

I think this is done now (?) - I've updated the tests to expect the FatalError and can't think of anything else to do in the main code

pypa / cibuildwheel