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

fix(setup): sync with cmake_example #954

Merged
merged 1 commit into from
Jun 24, 2021

Conversation

henryiii
Copy link
Member

The current version of the setup.py wasn't loading the correct MSVC reliably. I've synced this up with the work done on the cmake_example, which fixes this issue and is arguably a bit nicer with more docs.

@henryiii
Copy link
Member Author

FYI, the current pybind/cmake_example was heavily influenced by awkward's setup.py, so it's coming full circle. :)

@henryiii henryiii requested a review from jpivarski June 24, 2021 16:31
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

This looks good to me: adding more CMake environment variables and a mapping of platform names, presumably to build the more fine-grained matrix of wheels. The only thing that sticks out to me is a mixture of += and append for accumulating arguments, but that's a minor stylistic thing. I'm relying heavily on the fact that the tests pass and that you definitely know what you're doing.

So I'd merge this as-is, and since your last commit was 19 hours ago (and you just asked me to review), I assume you're done and I'll merge it right away.

@jpivarski jpivarski merged commit 1e332b8 into scikit-hep:main Jun 24, 2021
@henryiii henryiii deleted the henryiii/fix/msvc branch June 24, 2021 16:45
This was referenced Jun 24, 2021
@henryiii
Copy link
Member Author

The only thing that sticks out to me is a mixture of += and append

I had intended to make all the one-item additions an append, but got missed. Might try to clean that up later, then.

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.

2 participants