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

[package] CPython: Misc recipe improvements #23151

Open
1 of 13 tasks
Ahajha opened this issue Mar 18, 2024 · 6 comments
Open
1 of 13 tasks

[package] CPython: Misc recipe improvements #23151

Ahajha opened this issue Mar 18, 2024 · 6 comments

Comments

@Ahajha
Copy link
Contributor

Ahajha commented Mar 18, 2024

I'm making this issue to serve as a TODO list of things that I believe need to be done in the CPython recipe:

  • Emulate CMake's built-in find_package(Python): Currently this is just find_package(cpython), but we need to manually pass in a bunch of extra variables to make sure we find the Conan one. We should be able to use find_package(Python) without this extra setup. We would need to include Python_add_library, which is implemented in CMake here: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/FindPython/Support.cmake?ref_type=heads#L4016. Potentially we could have this as an option. Details in comments.

  • Check which options are actually usable: Many of the options (like with_lzma) imply that there is a way to not include certain dependencies/packages, but I suspect with recent changes that many of these no longer work (or maybe never did). For each of these we should determine if it is actually possible to implement it properly, and remove it if it is either impossible or highly unfeasible.

  • Add 3.13 (once released): I expect this to not have too many breaking changes regarding the build system, but I at least know that some standard library modules have been removed: https://peps.python.org/pep-0594/. The main ones of note are nis, msilib, and crypt, which I believe are the only ones that would affect the recipe (mainly just disabling things in 3.13+)

  • Enabling the optimizations and lto options, and ideally turn it on by default, at least in Release mode: This would ensure the Conan Python packages are on par speed-wise with the official binaries.

  • Setting shared by default: Windows static mode has not worked since about 3.10, and seems to only be supported on a best-effort basis (Are static library builds on Windows still supported? python/cpython#110234). Linux/Mac static builds work and are fully functional, but on all OSs shared seems to be the preferred distribution method. For Windows specifically, it's a bit annoying for the default configuration to be invalid.

  • Simplifying/fixing defined conf info and environment variables (detail in comments).

  • Moving Windows' install up one directory: There is a lot of conditional logic for Windows installs/package info, since it puts everything in self.package_folder/bin, rather than self.package_folder directly. I believe this was a workaround in the original PR due to limitations in Conan at the time, I'm fairly sure it could be moved now. My only possible concern is regarding linters, it might not like that layout.

  • Model Python C Module dependencies(expat, bz2, xz_utils, etc) in package: Right now, they are propagated through the _hidden component, but they can still get linked. It seems the only reason the _hidden component exists is to stop the "Required package not in component requires" error, but perhaps there is a better way. transitive_libs=False? visible=False? Not sure.

  • Fix Windows all-shared build: This is currently skipped in CI due to mpdecimal not supporting shared mode by default (with additional c++ things). At minimum, it should work with all other dependencies in shared mode, ideally we should get CI to also run this config, either by tweaking the mpdecimal or cpython recipes.

  • Add options to enable GIL-less Python and for the experimental JIT.

  • Fix mpdecimal 2.5.1 on Mac. I believe this is caused by CPython using a fork of mpdecimal with some extra configuration settings, on Mac it builds universal binaries (Or so I think, it defines a UNIVERSAL macro which in turn defines other config macros). Will need to analyze a diff.

  • Fix using the Python Limited API on Windows. Adding this triggers a #pragma(comment lib ...), which fails because the link directories aren't available.

  • Test the latest CMake: PR https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9482 introduces some changes to Windows/Debug mode, perhaps it could simplify some handling in the test package.

Other notes:

  • There's a fork of CPython that supports MingGW here: https://github.com/msys2-contrib/cpython-mingw/, these changes are likely going to continue to stay separate. If we wanted the recipe to support MinGW, we would almost certainly need to pull from that remote, and it would likely lag behind in versions. IMO the high effort, plus MinGW being a relatively small share of the 'market', plus having to pull from a different remote, makes it not worth the effort.
@Ahajha
Copy link
Contributor Author

Ahajha commented Mar 20, 2024

Regarding conf info and environment variables:

Currently, we have the following defined:
Env: PATH, PYTHON, PYTHONHOME, PYTHON_ROOT
Conf: user.cpython:python, user.cpython:pythonhome, user.cpython:module_requires_pythonhome, user.cpython:python_root

Env:

  • PATH: fine
  • PYTHON: I can't find any docs on this, inclined to remove it if we don't know its purpose. My guess is that it was just used to mirror the python conf variable.
  • PYTHONHOME: This is fine to keep, but it needs to be fixed. First of all, it's only defined on Windows, which if anything is the only platform that it might not work. On Windows at least, defining it(with the current value) breaks everything involving running Python. Not defining it (current behavior) breaks embedded interpreters only, but then defining it manually allows embedded interpreters to work. The docs at https://python.readthedocs.io/en/latest/using/cmdline.html#envvar-PYTHONHOME seem to not be accurate to Windows (https://discuss.python.org/t/the-document-on-pythonhome-might-be-wrong/19614/5), so we might need to play around with this one a bit. So TL;DR: Fix(probably remove) the condition, fix the value.
  • PYTHON_ROOT: As far as I can tell, this is only used for ancient versions of Boost.Python. I can't find any official documentation on it, and the latest thing I've found that even mentions it is from nearly a decade ago. Probably best to remove it.

Conf:

  • python: Good to keep, probably generally useful to have an absolute path to the interpreter.
  • pythonhome: Maybe useful, as long as we can solve the above issue this seems harmless to keep, thought it might not get used much if at all.
  • module_requires_pythonhome: Currently true if on Windows or an Apple OS, but it should also apply to relocated packages built with autotools. So essentially just remove it, since it will always be true.
  • python_root: Same note as with the PYTHON_ROOT: Delete.

I'm unsure of the use of the env_vars option for Conan 2.0, that kind of option seemed more of a hack around there being no way to opt out of the variables. Now, Conan 2.0 has an opt-in method of using env vars, so I don't think this option should be used in Conan 2.0.

@Ahajha
Copy link
Contributor Author

Ahajha commented Mar 27, 2024

Regarding transparent FindPython integration, I've played with a few ideas:

  1. If we make the CMake find modules Python and Python3 (one has to be CONFIG, I don't think there's a way to make aliases as of yet), then we can set some hint variables and then include("${CMAKE_ROOT}/Modules/FindPython3"). This gets us decently far, but doesn't seem to work on Windows in Debug mode(problem, was known before and unsure how to fix), and would likely require linking the created targets with cpython::XXX (likely not a problem, assuming I'm allowed to change those targets within CMake).
  2. Include a definition of Python3_add_library. This gets us further, but it becomes difficult to manage the various targets that FindPython exposes, for example, Python3::SABIModule vs Python3::Module. This would be a longer implementation, and would lock us into latest the version of FindPython, instead of relying on CMake's version.

I think (1) is the ideal route, but I need to figure out why Debug mode on Windows isn't working. Maybe it's a bug in CMake, unsure.

@valgur
Copy link
Contributor

valgur commented Mar 27, 2024

For FindPython it's sufficient to add cpython as a tool_requires and a VirtualBuildEnv. I think that's not too much to ask from the package consumers.

@Ahajha
Copy link
Contributor Author

Ahajha commented Mar 27, 2024

That seems to work with Release mode on Windows, but not Debug (seems to really want to pick any other version on the system). I suppose it's less of an issue if it's in the build context, since that will usually be release mode, but I would like it to work without issue.

@Ahajha
Copy link
Contributor Author

Ahajha commented Apr 5, 2024

Update on the find_package issue regarding Windows Debug mode: I don't think it will be possible to just do nothing on our side and have it work, but I can get it to work in the test package (and consequently in other recipes) by defining just Python3_EXECUTABLE and Python3_LIBRARY in CMake to the .exe and .lib file, respectively. Going back to idea 1 from above, this should now be possible.

@Ahajha
Copy link
Contributor Author

Ahajha commented Apr 9, 2024

Regarding options that might not work:

All of these options default to on, so I tried disabling all of them, then reverted the ones that caused test failures.

For Windows:

3 options are removed for Windows: with_curses, with_gdbm, with_nis. The remaining 4 work, except that a small patch needs to be made: In _msvc_excluded_projects, the discarded bz2 project should be changed to _bz2.

No notably changes were made to the build system at any point, so I only tested 3.10.14.

For non-Windows platforms:

(Tested on Ubuntu 22.04)

The build system for modules changed significantly between 3.10 and 3.12, with 3.11 being in a transition state, so I tested all 3. Pre-3.10 not much changed. (Also to note, so far I don't see much change between 3.12 and 3.13.)

All options work in all versions. Some options, however, will not correctly apply if the respective system package is installed, for example with_curses.

Honorable mention to with_nis, which currently defaults to off since the requisite nsl package does not exist in CCI. However, this module is being removed in 3.13, so I don't think there's a point trying to enable it at this point, as it ends up picking up a system package for now.

Edit: After removing libncurses-dev and other dev packages (unsure why they were installed, but besides the point), all options now work in all versions. So some of them are affected by the system packages, ideally they shouldn't be.

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

No branches or pull requests

2 participants