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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[python3] Improve support for embedding and add tools #13556

Closed
wants to merge 13 commits into from

Conversation

Hoikas
Copy link
Contributor

@Hoikas Hoikas commented Sep 16, 2020

This pull request improves the downstream experience when working with the python3 port by building a functioning interpreter with support for pip and improving support when cmake's FindPython3 finder is used. Previously, when using FindPython3, if both the DEVELOPMENT and INTERPRETER components were requested, you would have to have a local python install matching the vcpkg python. Further, on Windows, this module preferred registry python over any slightly misconfigured python artifacts. Fixing these issues allows us to simplify ports whose cmake build systems need the python3 development libraries.

Anyone who intends to do further work on the python3 port after this might consider fixing vcpkg_find_acquire_program(PYTHON3) to use the interpreter installed by this port.

Note that on Windows a patch is applied in static python interpreters to disable extension modules-due to the way DLLs function, it is not possible to load them without crashing the interpreter. Further, support for the static CRT has been removed both to avoid surprises in that regard and due to the nature of PyObjects.

@Hoikas Hoikas marked this pull request as ready for review September 17, 2020 02:25
@PhoebeHui PhoebeHui added the category:port-feature The issue is with a library, which is requesting new capabilities that didn鈥檛 exist label Sep 17, 2020
@PhoebeHui
Copy link
Contributor

Thanks for the PR!

@ras0219-msft, could you help further review?

@@ -15,7 +15,7 @@ get_filename_component(PYTHON2_DIR ${PYTHON2} DIRECTORY)
vcpkg_configure_cmake(
SOURCE_PATH ${SOURCE_PATH}
PREFER_NINJA
OPTIONS -DCMAKE_PROGRAM_PATH=${PYTHON2_DIR} -DUSE_WINDOWS6_API=ON
OPTIONS -DPYTHON:FILEPATH=${PYTHON2} -DCMAKE_PROGRAM_PATH=${PYTHON2_DIR} -DUSE_WINDOWS6_API=ON
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this writing. it's tiny and beautiful!

@ras0219-msft
Copy link
Contributor

ras0219-msft commented Sep 24, 2020

Thanks for this PR!

Python is a very tricky subject in vcpkg, because there are three different use cases:

  1. Libraries want to run python code during their build -- this means they need python for the host machine. This case is when ports should be using vcpkg_find_acquire_program(PYTHON3).
  2. Libraries want to link against python.dll/.a/.lib because they expect the consumer to be a c++ program that embeds python -- this means they need python for the target machine. This is covered by the python3 port.
  3. Libraries want to link against python.dll/.so because they are authoring extension modules to be used with an external python. This is the stickier situation, because it's possible that the ABI of the external python does not match the ABI of the internal python. However, I've been lead to believe that on Windows at least, python3 should generally be ABI compatible and so in this case it's acceptable to use the python3 port for building. I think same situation may apply for Linux, but I'm less certain of this platform.

Framed by the above three circumstances, could you re-explain what this PR is intended to accomplish? It's important to keep these situations separate in order to enable cross compilation.

@Hoikas
Copy link
Contributor Author

Hoikas commented Sep 24, 2020

Agreed, Python is definitely very tricky. Given the three use case you pointed out, this pull request was designed to address issues I've experienced in projects I contribute to around use case 2 and, to some extent, use case 1.

When using vcpkg's python with any of cmake's FindPython modules on Windows, they first go to the registry. The find modules introduced in CMake 3.12 (FindPython, FindPython2, FindPython3) add components such as Development and Interpreter. When a downstream user of vcpkg python is both embedding and using python as part of their build process eg by find_package(Python3 REQUIRED COMPONENTS Interpreter Development), the find will fail if the user does not have an installed system Python that is ABI compatible with the vcpkg python, assuming that the user correctly manually specifies the vcpkg artifacts.

Additionally, specifying the vcpkg artifacts is tricky with these finders, especially when using a multi-config generator (eg Visual Studio). A common complaint from our users is that the debug library is not properly found, leading to either surprise link errors with python38_d.lib not being found or with FindPython deciding to use some other non-vcpkg python. Overall, this leads to an unacceptable level of surprises and GOTCHAS, IMO. This pull request fixes that by building the python interpreter and ensuring artifacts are properly specified for all five cmake find python variants: FindPythonLibs, FindPythonInterp, FindPython, FindPython2, and FindPython3.

In the case of embedding, having python without its standard library is largely pointless. In fact, in Python 3, python cannot be correctly embedded without the standard library. It imports the encodings module on startup and fails if the standard library is unavailable. CMake 3.12's new find modules return the location of the Python standard library -- which is mostly a set of python scripts. However, to do so, it must run the interpreter itself. This brings us back to the previous point. Further, the standard library requires several internal extension modules to be built. This PR causes those extension modules to be built--they were not previously.

I built a project to test what I expect to work on Windows, macOS, and Linux here. (I disabled Windows CI in that repository in favor of local testing.)

Several ports fall into multiple use cases in that they use python3 in their build process and either embed or extend it. In those cases, we find fragile artifact specifications for the python development libraries in the portfile.cmake and a call to download the python interpreter. With a properly functioning python.exe built by vcpkg, these use cases can be simplified fairly significantly... Unless the upstream library rolls its own python finder (eg pybind11). If, on the other hand, the port only needs python as part of its build process, use case 1 remains unaffected.

I did address one issue related to case 3 while working on the PR. Python is ABI stable at the minor release level (eg, 3.8.3 is compatible with 3.8.5 but not with 3.9.0) unless using the Py_LIMITED_API, which is stable across all of python 3. vcpkg neither supports this option presently nor with this PR. One undocumented sticking point I found with extension modules is related to the library linkage. On Windows, extension modules can only function against a shared python library. Attempting to link an extension module against the static python library will crash the interpreter when the end user imports said module. This limitation is not in present on unix-like platforms. Previously, on Windows, this was a silent GOTCHA. I did not outright remove support for static triplets because the static library is incredibly helpful when embedding python in use case 2.

This allows overriding the name of the install target, allowing
specification of something other than `make install`, eg `make
altinstall`.
- Many deprecated functions were removed.
- A working python interpreter is now installed under tools.
- The import library is now properly frozen into the interpreter.
 This allows us to prevent CMake going to the registry or otherwise
 exploding when using:
 - FindPython
 - FindPython3
 - FindPythonInterp
 - FindPythonLibs
The upstream port supports python3 now, but a new release is not yet
available.
It seems pybind11 has its own python find module. The current upstream
head allows using CMake's new FindPython module with the definition
`-DPYBIND11_FINDPYTHON`, but there has been no upstream release with
this feature. In the meantime, we correctly find the vcpkg python3
interpreter and libraries.
This fixes an observed build failure on x86-windows when the vcpkg expat
port is installed before the python3 port is built.
@Hoikas
Copy link
Contributor Author

Hoikas commented Sep 25, 2020

This has been rebased to merge cleanly with master.

@JackBoosY
Copy link
Contributor

blocked by #13722.

@Hoikas
Copy link
Contributor Author

Hoikas commented Sep 25, 2020

I am unable to reproduce the CI failures with irrilicht and qt5-location locally.

@JackBoosY
Copy link
Contributor

blocked by #13765.

@Hoikas
Copy link
Contributor Author

Hoikas commented Oct 1, 2020

CI issues fixed in #13812 and #13830.

@JackBoosY
Copy link
Contributor

osg regression will be fixed in #13942.

@Hoikas
Copy link
Contributor Author

Hoikas commented Oct 12, 2020

After re-reading the comment by @ras0219-msft, I have reworked some of the commits in this pull request to properly support use case 1 by not assuming that the python interpreter build by vcpkg may be used on the host machine.

scripts/ci.baseline.txt Show resolved Hide resolved
@Hoikas
Copy link
Contributor Author

Hoikas commented Nov 3, 2020

Before I take the time to resolve the merge conflicts, would it help the review process if this were split into multiple pull requests? It's been a little over a month since any substantial input was given.

@JackBoosY
Copy link
Contributor

@Hoikas yes, that's better.

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鈥檛 exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants