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 Pybind11Extension on Mingw64 #2921

Merged
merged 1 commit into from
Apr 2, 2021
Merged

Conversation

jeromerobert
Copy link
Contributor

@jeromerobert jeromerobert commented Mar 27, 2021

Description

Suggested changelog entry:

Fix the default ``Pybind11Extension`` compilation flags with a Mingw64 python

* Pybind11Extension add the "/EHsc /bigobj /std:c++14" flags on Windows.
  This is good for Visual C++ but not for Mingw.
* According
  https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-python2/0410-MINGW-build-extensions-with-GCC.patch
  sysconfig.get_platform() is the way to check for a Mingw64
@henryiii
Copy link
Collaborator

henryiii commented Apr 2, 2021

Great to see fixes for MinGW! Would you have suggestions for how to run minGW in GHA? I'd like to test for it, too, but don't know anything about using MinGW.

@henryiii henryiii added the bug label Apr 2, 2021
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

I feel it's fine to merge this change even if we don't have a MINGW GitHub Action at the moment.

Copy link
Collaborator

@henryiii henryiii 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 perfectly fine to merge this change as is, but I don't find someone with MinGW knowledge that often, so I'm hoping I can get some advice and/or maybe actually finally add a MinGW run to the tests. :) (And I thought I already approved, but I guess not)

@rwgk
Copy link
Collaborator

rwgk commented Apr 2, 2021

Thanks @jeromerobert and @henryiii ! I'll merge this now. If you can help us with GHA that'll be great, but optional.

@rwgk rwgk merged commit 1259db6 into pybind:master Apr 2, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Apr 2, 2021
@rwgk
Copy link
Collaborator

rwgk commented Apr 2, 2021

Just logging my thinking here about GHA:

  • GHA MinGW would be a great to have.
  • But it is an effort to get going, and will have a cost maintaining, too.
  • How many people care/are affected? Maybe just a few for MinGW?
  • Assuming breakages are rare, and we merge such tiny fixes like this one quickly, that's actually an efficient use of everybody's time.

@henryiii
Copy link
Collaborator

henryiii commented Apr 2, 2021

These things pop up every so often, especially the the example repos. It's very much worth having if we can do it easily. I'm guessing that it's possible and pretty easy if you know how to set it up. We promise MinGW support in the readme, so it should be tested (I think now it is the only one promised that is not tested, actually). I'm hoping someone who cares about MinGW will help set it up one day.

@jeromerobert
Copy link
Contributor Author

@henryiii Using https://github.com/msys2/setup-msys2 it should be easy enough to add a test for Mingw64. What existing PyBind11 Github Actions job could we take inspiration from?

@henryiii
Copy link
Collaborator

henryiii commented Apr 2, 2021

Well, any of the checks, maybe this one is close:

win32-msvc2015:
name: "🐍 ${{ matrix.python }} • MSVC 2015 • x64"
runs-on: windows-latest
strategy:
fail-fast: false
matrix:
python:
- 2.7
- 3.6
- 3.7
# todo: check/cpptest does not support 3.8+ yet
steps:
- uses: actions/checkout@v2
- name: Setup 🐍 ${{ matrix.python }}
uses: actions/setup-python@v2
with:
python-version: ${{ matrix.python }}
- name: Update CMake
uses: jwlawson/[email protected]
- name: Prepare MSVC
uses: ilammy/msvc-dev-cmd@v1
with:
toolset: 14.0
- name: Prepare env
run: python -m pip install -r tests/requirements.txt --prefer-binary
# First build - C++11 mode and inplace
- name: Configure
run: >
cmake -S . -B build
-G "Visual Studio 14 2015" -A x64
-DPYBIND11_WERROR=ON
-DDOWNLOAD_CATCH=ON
-DDOWNLOAD_EIGEN=ON
- name: Build C++14
run: cmake --build build -j 2
- name: Run all checks
run: cmake --build build -t check

We don't really test the one thing that you fixed here, but just that pybind11 compiles correctly; IIRC the python extensions are tested only in pybind/python_example, https://github.com/pybind/python_example/blob/master/.github/workflows. If we had a test in pybind11, it could be duplicated there (it uses pip instead of CMake). I'd only add 1-2 checks, say 64 bit and 32 bit for some version of Python.

@henryiii
Copy link
Collaborator

FYI, this doesn't seem to work (like all untested code 🤣 ). I believe sysconfig only tells you what Python was built with, not what you are targeting. That requires a custom build_ext (which I believe I literally wrote in a comment somewhere in the docs).

A fix will need to patch the build_ext command and change these arguments if mingw64 is being used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants