Skip to content

CMake improvement: support windows python with msys & mingw #2312

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

Merged
merged 1 commit into from
Jul 21, 2020

Conversation

lkeegan
Copy link
Contributor

@lkeegan lkeegan commented Jul 21, 2020

Change to FindPythonLibsNew.cmake behaviour:

  • on windows, look for windows python lib (including when using mingw and msys)
  • if python lib is not found and we are using msys, then look for system python lib

Previous behaviour was to skip the first step when using msys & mingw, so windows python libs are not found (see example travis job):

-- Found PythonInterp: C:/Python38/python.exe (found version "3.8.2") 
-- Found PythonLibs: python38

i.e. doesn't find the python library

New behaviour with this PR (see example travis job):

-- Found PythonInterp: C:/Python38/python.exe (found version "3.8.2") 
-- Found PythonLibs: C:/Python38/libs/python38.lib

@bstaletic
Copy link
Collaborator

So this looks reasonable, but someone with more experience with msys/mingw needs to give his blessing.

@YannickJadoul
Copy link
Collaborator

So this looks reasonable, but someone with more experience with msys/mingw needs to give his blessing.

That's not me, but I don't know if there is a regular contributor that has more experience?

There's talks to ditch all this FindPythonLibsNew.cmake in favour of a newer FindPythonLibs in CMake >= 3.12 (#2201), but meanwhile, I guess this could go in?

Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

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

If this one question gets resolved, I don't see how it could hurt non-mingw/msys builds, so as far as I'm concerned, it could be merged, if it solves an issue.

@henryiii
Copy link
Collaborator

ditch all this

I don't think we will be able to ditch it, at least for a while, but there will (hopefully) be a much better way to do it soon. But agree that we should still get fixes like this in!

@YannickJadoul
Copy link
Collaborator

I don't think we will be able to ditch it,

Ow, then I got slightly too excited. Even better to get this in then.

  - look for windows python lib when using mingw & msys
  - if not found, then look for system python lib as before
@lkeegan lkeegan force-pushed the find_python_libs_msys branch from 0915198 to c5fb605 Compare July 21, 2020 14:35
@YannickJadoul YannickJadoul requested a review from henryiii July 21, 2020 15:52
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.

Me too. :)

@YannickJadoul
Copy link
Collaborator

Me too. :)

OK, and it's green, so let's merge it then? :-)

Thanks, @lkeegan!

@YannickJadoul YannickJadoul merged commit c4fd1fd into pybind:master Jul 21, 2020
@lkeegan
Copy link
Contributor Author

lkeegan commented Jul 21, 2020

thanks!

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.

4 participants