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

[python3] Add vcpkg-cmake-wrapper. #15221

Merged
merged 18 commits into from
Jan 5, 2021

Conversation

Hoikas
Copy link
Contributor

@Hoikas Hoikas commented Dec 19, 2020

This adds a vcpkg-cmake-wrapper.cmake to the python3 port. CMake offers three disparate find modules that are compatible with Python 3.x. They are FindPythonInterp, FindPythonLibs, and FindPython. The first two are deprecated since CMake 3.12 but still widely used, and the last one implements FindPython2 and FindPython3. Working with these find modules is particularly challenging due to their propensity to find the Python installed by the system package manager or an installed Windows Python. FindPython particularly has issues resolving the Python libraries due to the separate debug/lib and lib directories in the vcpkg installed tree. This ensures that any calls to find_package() for PythonLibs, PythonInterp, and Python3 correctly finds the vcpkg provided python3 port. To see the improvement, look at the simplification to the python feature of itk.

I'd appreciate review, especially around the macOS portion of the wrapper. I don't have access to a mac machine, and the workarounds for static linkage are very empirical from my custom tests on GHA.

@Hoikas Hoikas marked this pull request as ready for review December 20, 2020 00:24
@JackBoosY JackBoosY added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist requires:discussion labels Dec 21, 2020
@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Dec 22, 2020
@BillyONeal
Copy link
Member

FindPython is a builtin cmake module so this meets the usual "vcpkg-cmake-wrapper" nuclear option criteria. I do wonder though what our actual story is; we don't intend users to use the python port to get a python.exe or pythonw.exe or similar; that port is really about writing things that expose bindings to python rather than trying to be a complete python distribution channel. (i.e. we do not want to compete with Python's own MSIs or similar)

That said, the existing FindPython bits don't distinguish these use cases so we might be in a situation where we provide that accidentially; what effect does this have on people who use vcpkg but are expecting to find a system python where there might be pip packages or similar?

@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Dec 24, 2020
@Hoikas
Copy link
Contributor Author

Hoikas commented Dec 24, 2020

My focus in this PR is embedding Python into another application (in this case a 17 year old game). I agree that we don't want to replace the official Python distribution, but the separation of concerns as of Python 3 is very muddy. Python 3 is interesting in that the core actually imports Python code from its own standard library during initialization. If that code is unavailable, that is a fatal error.

In order to properly make the Python standard library available, FindPython can report the location of the standard library on the build machine for bundling but only if the executable is available. In the project I help maintain, we have to use find_package(Python3 Interpreter Development REQUIRED), causing an awkward situation where a system Python with a minor version level matching the vcpkg port needed to be available for the interpreter component. #14891 helps with this slightly in that a matching interpreter is now available. But all of these finders by default go plundering in the registry and are incapable of finding the combination of the vcpkg interpreter and libraries without manual intervention**. FindPython tends to be very nondeterministic in error cases, returning either arbitrary system Python or failing entirely.

In #14891, I strove to make the vcpkg python3 "tool" look and behave similar to a system install of Python. When the port is built, we ensure that pip is installed, and that pip functionality is available. In the aforementioned project, we also have build scripts that need extensions installed by pip. This works by effectively ${Python3_EXECUTABLE} -m pip install -r requirements.txt

** The manual intervention required is to pass something like this to cmake:
-DPython3_FIND_REGISTRY=NEVER -DPython3_EXECUTABLE=<path to exe> -D_Python3_LIBRARY_RELEASE=<path to release lib> -D_Python3_LIBRARY_DEBUG=<path to debug lib>
IMO this is burdensome to users of vcpkg and is the crux of the complaint in #9026.

@JackBoosY JackBoosY requested a review from BillyONeal December 29, 2020 06:35
@marekr
Copy link
Contributor

marekr commented Dec 30, 2020

FindPython is a builtin cmake module so this meets the usual "vcpkg-cmake-wrapper" nuclear option criteria. I do wonder though what our actual story is; we don't intend users to use the python port to get a python.exe or pythonw.exe or similar; that port is really about writing things that expose bindings to python rather than trying to be a complete python distribution channel. (i.e. we do not want to compete with Python's own MSIs or similar)

@BillyONeal I'll give you my bit on this. I work on KiCad. We embed python as part of our application and expose application functionality via api bindings. This means we have a fully functioning python interpreter DLL involved but not the python exes.
However to provide our user the full python experience, we need to compile and include the python modules which currently are missing from vcpkg. Without these packages, even the python library being in vcpkg is rather completely worthless as a chunk of the expected core functionality is in them.

The python upstream distributions cannot be used for this as compiler settings have to match entirely across the whole build. In many cases there are libraries used by these core modules that vcpkg builds as well. This would mean we would need to somehow handle two separate copies of openssl at different versions for example which I don't even want to think of the LoadLibrary hell.

Hoikas has been pushing to include support for being able to fully have true embeddable python with these modules

@JackBoosY
Copy link
Contributor

Ping @BillyONeal for response.

@BillyONeal
Copy link
Member

I tagged with "requires: discussion" because I want it to be discussed; normally we discuss PRs with that tag on Thursdays but that didn't happen last week due to holidays :)

@BillyONeal
Copy link
Member

OK, after some discussion it seems like the behavior should be:

  • find_package(python COMPONENTS interpreter) should do nothing and just pass through to the built in find python
  • find_package(python COMPONENTS development) should be overridden like you're trying to do here
  • find_package(python COMPONENTS interpreter development) should be overridden even though the resulting interpreter might not be what the user wants

Was that your intent?

@Hoikas Hoikas force-pushed the py3_vcpkg_cmake_wrapper branch from 3593500 to 5db060a Compare January 1, 2021 00:43
@Hoikas Hoikas force-pushed the py3_vcpkg_cmake_wrapper branch from 5db060a to 17aa0b2 Compare January 1, 2021 00:56
@Hoikas Hoikas force-pushed the py3_vcpkg_cmake_wrapper branch from 17aa0b2 to e9647f7 Compare January 1, 2021 01:09
@Hoikas
Copy link
Contributor Author

Hoikas commented Jan 1, 2021

Ok, I've written up the suggestions by @BillyONeal, and, as a bonus, added a usage file to point specifically to the CMake 3.12+ python finders. To recap, this is the expected behavior...

Per the documentation:

If no COMPONENTS are specified, Interpreter is assumed.

If component Development is specified, it implies sub-components Development.Module and Development.Embed.

So:

find_package(Python) - implies COMPONENTS Interpreter, see below
find_package(Python COMPONENTS Interpreter) - does nothing
find_package(Python COMPONENTS Development) - implies COMPONENTS Development.Embed Development.Module, see below
find_package(Python COMPONENTS Development.Embed) - overrides both the interpreter and libraries
find_package(Python COMPONENTS Development.Module) - overrides only libraries
find_package(Python COMPONENTS Interpreter Development.Module) - overrides both the interpreter and libraries
find_package(Python COMPONENTS Interpreter Development.Embed) - overrides both the interpreter and libraries
find_package(PythonInterp) - does nothing
find_package(PythonLibs) - does nothing

Note that Python3 is an alias for the Python package.

Development.Embed must override the interpreter so that the Python_STDLIB can be fetched and packaged as part of the embedder's build process.

@Hoikas Hoikas marked this pull request as ready for review January 1, 2021 01:13
@JackBoosY JackBoosY requested a review from vicroms January 4, 2021 02:13
@Hoikas
Copy link
Contributor Author

Hoikas commented Jan 5, 2021

I've pushed an important change that should prevent the python3 port from hiding other python interpreters on the path. This change should address CI failures seen in #15200 and #15298.

@Hoikas Hoikas force-pushed the py3_vcpkg_cmake_wrapper branch from 6d370e1 to 6f9a24a Compare January 5, 2021 15:48
@Hoikas Hoikas marked this pull request as draft January 5, 2021 16:32
@Hoikas Hoikas force-pushed the py3_vcpkg_cmake_wrapper branch from 6f9a24a to cfd451b Compare January 5, 2021 16:36
@Hoikas Hoikas marked this pull request as ready for review January 5, 2021 16:36
@Hoikas Hoikas marked this pull request as draft January 5, 2021 16:37
@Hoikas Hoikas marked this pull request as ready for review January 5, 2021 16:49
@vicroms vicroms merged commit 378ffbb into microsoft:master Jan 5, 2021
@JackBoosY
Copy link
Contributor

JackBoosY commented Jan 6, 2021

Note that this PR does not solve the issue that some ports rely on python2 instead of python3:
when python2 and python3 are downloaded to downloads/tools/python at the same time, cppcms will always find python3 first, however, its script is version 2.x.

PS F:\vcpkg> .\vcpkg.exe install cppcms
Computing installation plan...
The following packages will be built and installed:
    cppcms[core]:x86-windows -> 1.2.1-1
...
Finding PYTHON2 name python in F:/vcpkg/downloads/tools/python/python-2.7.16-x86;F:/vcpkg/downloads/tools/python/python-2.7.16-x86
...

but log shows:

[306/431] cmd.exe /C "cd /D F:\vcpkg\buildtrees\cppcms\x86-windows-dbg && F:\vcpkg\installed\x86-windows\tools\python3\python.exe F:/vcpkg/buildtrees/cppcms/src/20a4ad93d4-26c9d29282.clean/bin/cppcms_tmpl_cc -s skin1 -o F:/vcpkg/buildtrees/cppcms/x86-windows-dbg/skin1.cpp F:/vcpkg/buildtrees/cppcms/src/20a4ad93d4-26c9d29282.clean/src/hello_world_skin1.tmpl F:/vcpkg/buildtrees/cppcms/src/20a4ad93d4-26c9d29282.clean/src/hello_world_view1.tmpl"
FAILED: skin1.cpp 
cmd.exe /C "cd /D F:\vcpkg\buildtrees\cppcms\x86-windows-dbg && F:\vcpkg\installed\x86-windows\tools\python3\python.exe F:/vcpkg/buildtrees/cppcms/src/20a4ad93d4-26c9d29282.clean/bin/cppcms_tmpl_cc -s skin1 -o F:/vcpkg/buildtrees/cppcms/x86-windows-dbg/skin1.cpp F:/vcpkg/buildtrees/cppcms/src/20a4ad93d4-26c9d29282.clean/src/hello_world_skin1.tmpl F:/vcpkg/buildtrees/cppcms/src/20a4ad93d4-26c9d29282.clean/src/hello_world_view1.tmpl"
Traceback (most recent call last):

edit: I think we can only fix cppcms to solve that.

@Hoikas
Copy link
Contributor Author

Hoikas commented Jan 6, 2021

IIRC, cppcms does something strange with the CMAKE_PROGRAM_PATH instead of adding Python2 to the path. The next version of cppcms will support Python3 though 😄

@JackBoosY
Copy link
Contributor

@Hoikas Can you please fix it for the current version?

@Hoikas Hoikas deleted the py3_vcpkg_cmake_wrapper branch March 17, 2021 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[python3] cmake FindPython3 Development component
5 participants