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

set VSCMD_ARG_TGT_ARCH based on targetted architecture #1876

Merged
merged 10 commits into from
Jul 12, 2024

Conversation

MusicalNinjaDad
Copy link
Contributor

@MusicalNinjaDad MusicalNinjaDad commented Jun 12, 2024

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 MusicalNinjaDad force-pushed the MusicalNinjaDad/issue1861 branch 4 times, most recently from 4ee16c8 to c8a6e53 Compare June 14, 2024 06:27
@MusicalNinjaDad

This comment has been minimized.

@MusicalNinjaDad MusicalNinjaDad marked this pull request as ready for review June 14, 2024 17:46
@MusicalNinjaDad
Copy link
Contributor Author

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

@henryiii
Copy link
Contributor

henryiii commented Jul 1, 2024

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
Copy link
Contributor

henryiii commented Jul 1, 2024

@joerick or @mayeut, thoughts?

@mayeut
Copy link
Member

mayeut commented Jul 1, 2024

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

Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with this, it seems to me to be very similar to _PYTHON_HOST_PLATFORM/ARCHFLAGS that we set in macos.py.

I don't have any specific knowledge of the MS compiler to predict and second-order effects though. @zooba, since you created the cross-compilation mechanism, do you have an opinion on the use of VSCMD_ARG_TGT_ARCH for cross-compilation?

cibuildwheel/windows.py Outdated Show resolved Hide resolved
@zooba
Copy link
Contributor

zooba commented Jul 2, 2024

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.

cibuildwheel/windows.py Outdated Show resolved Hide resolved
cibuildwheel/windows.py Outdated Show resolved Hide resolved
@MusicalNinjaDad
Copy link
Contributor Author

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

Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@henryiii henryiii merged commit de84624 into pypa:main Jul 12, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cibuildwheel GitHub action fails due to duplicate skbuild build directory on Windows ARM64 build
5 participants