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] Fix distutils path #30822

Merged
merged 10 commits into from
Apr 26, 2023
Merged

[python3] Fix distutils path #30822

merged 10 commits into from
Apr 26, 2023

Conversation

jimwang118
Copy link
Contributor

@jimwang118 jimwang118 commented Apr 13, 2023

Fixes #30607
Modified the output of the get_python_inc() function. From the original path: installed\triplet\tools\python3\include to now the output path is \installed\triplet\include\python3.10

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@jimwang118 jimwang118 added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Apr 13, 2023
@Neumann-A
Copy link
Contributor

No that is not the solution for #30607. Don't move python3 internals.

Start a shell open up python3 from vcpkg and do:
'import sys, distutils.sysconfig; print(distutils.sysconfig.get_python_inc())'

disutils is a python module. You dont want to move distutils, you want to fix what the output of distutils is so that you get the right answers in regard to file layout of vcpkg.

@jimwang118 jimwang118 marked this pull request as ready for review April 20, 2023 02:35
Adela0814
Adela0814 previously approved these changes Apr 20, 2023
@Adela0814 Adela0814 added the info:reviewed Pull Request changes follow basic guidelines label Apr 20, 2023
@BillyONeal
Copy link
Member

disutils is a python module. You dont want to move distutils, you want to fix what the output of distutils is so that you get the right answers in regard to file layout of vcpkg.

I think this is doing that now? Can you confirm that you're happy @Neumann-A ?

@@ -18,6 +18,7 @@ set(PATCHES
0010-dont-skip-rpath.patch
0012-force-disable-curses.patch
0013-configure-no-libcrypt.patch # https://github.com/python/cpython/pull/28881
0014-fix_get_python_inc_output.patch
Copy link
Contributor

Choose a reason for hiding this comment

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

You have submitted a diff file, not a patch file.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a problem. It is common practice in vcpkg, and a patch is not really desired.
(I use .diff now when I add the first patch to a port.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please use patches in the python3 port. See the discussion we already had in #27645 (comment) for the rationale. TIA

Copy link
Member

Choose a reason for hiding this comment

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

Please use patches in the python3 port. See the discussion we already had in #27645 (comment) for the rationale. TIA

As a result of your feedback there we didn't implement a standard requiring diffs rather than patches, but we didn't adopt a 'require patch' standard either. There seems to be substantial disagreement among contributors in this area. Given that we failed to arrive at a policy in either direction, I don't think it's reasonable to require contributors to pick one or the other at this time.

If you want to build support for establishing the policy as 'require patch' then I think at a minimum we need doc changes: https://github.com/microsoft/vcpkg-docs/blob/c61c79eeffc5f6c3d975af1243ad2d9c388a042f/vcpkg/examples/patching.md?plain=1#L147

Copy link
Contributor

@Hoikas Hoikas Apr 25, 2023

Choose a reason for hiding this comment

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

I am making this request for the python3 port only; this is so that I can continue to contribute to this port, which has grown rather complex over time. If patch files are not submitted when I ask for them, I will have to re-evaluate spending time on this port. I am not interested in proposing any policy changes. TIA

Copy link
Member

@BillyONeal BillyONeal Apr 25, 2023

Choose a reason for hiding this comment

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

If you are asking for rules that bind other contributors, that the vcpkg maintainers enforce for you, you are asking for policy. I didn't merge this yet because I think there's a 'there there' but if I'm going to say 'no contributor do this' I need a rule to point to. I would write that myself but I still don't have a good enough understanding of the workflow described in #27645 to do so.

I don't mean to say you or your contributions to the python ports thus far are unappreciated. (If I/we didn't care about your thoughts at all, this PR would be merged already.)

Copy link
Contributor

@Neumann-A Neumann-A Apr 25, 2023

Choose a reason for hiding this comment

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

If patch files are not submitted when I ask for them,

Give a workflow which generates patches without requiring a python git checkout.
My workflow is commonly so that I run vcpkg with and without --editable once and then just do a git diff --no-index "src.clean" "src" > my.diff after the edits. Adding the option --patch in there does nothing to the output (as expected since no git checkout).
That way is way easier then to search for the correct git clone someurl command and then do a git checkout <whatever-vcpkg-uses> then run the git format-patch command and actually copy over the patches to vcpkg and apply and test them.
If you want to enforce patches you need to at least show off an easy --editable workflow

Copy link
Member

Choose a reason for hiding this comment

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

OK, I've fixed this up to work with am now. After messing with it myself I'm still not in favor of requiring patches; for all the time am saved in applying the patches, this was lost in needing to revert bogus changes to all existing patches when you go to splat back out the format-patch part. And the workaround of "am stops, git commit -m message && git am --skip" does not seem too onerous.

@Neumann-A
Copy link
Contributor

Can you confirm that you're happy @Neumann-A ?

Output works now as expected. However I don't currently know if more stuff needs to be fixed if environment variables like PYTHONHOME and PYTHONPATH are set (However it currently seems to behave consistent).

Python on windows has the following problems:

  • DLLs are not found from the prefix/bin folder and setting PATH doesn't do anything more since python decided not to search there any longer for security reason. (You can patch in os.add_dll_searchpath() but this makes the py files basically unmovable in relation to the DLLs.)
  • There are no rules for ports where the install location of python wrappers for C/C++ libs should be.

As long as those points are not solved advanced usage of python within vcpkg on windows is basically blocked

@BillyONeal
Copy link
Member

As long as those points are not solved advanced usage of python within vcpkg on windows is basically blocked

Python continues to be the mother of all edge cases, not the least of which being that it wants to control the whole universe and needs a bunch of prerequisites set up by the end user in order to be used safely. I'm not sure if there is an answer for Python that doesn't involve a ton of special cases for it.

It sounds like this is at least better than before, so I'm going to try to patchify the diff and merge. Thanks for the review!

@Hoikas
Copy link
Contributor

Hoikas commented Apr 25, 2023

From what I understand, based on discussion in discord, there are ports that embed Python incorrectly. Embedding Python on unix-like systems tends to "just work" because everything is in a standard location, so the embedder doesn't have to worry about anything. Python on Windows (both the official installer and vcpkg) use a layout that isn't unix-standard, so embedders have to do legwork to make sure that the Python DLLs, standard library, and any required .pyd python modules are appropriately deployed as part of their application's install process. From my own adventure with embedding Python, I learned that the Python library itself has to load standard library Python code as part of the library initialization (a bit of a chicken and egg problem, for sure). From what I can tell, the port(s) being discussed would be broken even with the official Python installer for Windows - it doesn't set PYTHONHOME or PYTHONPATH either. I guess the question is: do we want to expend the effort to fix something in the upstream project(s)? Maintaining the python3 port is hard enough as is because of PSF's policy of dumping Windows 7 (which, despite the official EOL, many users continue to cling to) and CPython's objectively bad build system. Python 3.11 has been out since October, and I've really only had a chance to see that roughly half of the patches fail to apply 😭

@BillyONeal
Copy link
Member

Embedding Python on unix-like systems tends to "just work" because everything is in a standard location, so the embedder doesn't have to worry about anything.

Except not really, because the version that you get from vcpkg and the version on the system do not necessarily match.

From my own adventure with embedding Python, I learned that the Python library itself has to load standard library Python code as part of the library initialization (a bit of a chicken and egg problem, for sure). From what I can tell, the port(s) being discussed would be broken even with the official Python installer for Windows - it doesn't set PYTHONHOME or PYTHONPATH either.

Right, this is kind of what I meant by whatever happens being entirely specific to Python. Nothing else knows or cares about .pyd files, and there doesn't seem to be a standard location for those, so everyone is on the hook for their own mess. If other ports embedding python are doing so 'wrong', its largely fallout from there not really being an obvious 'correct' way of doing so.

Maintaining the python3 port is hard enough as is because of PSF's policy of dumping Windows 7

Do note that we, the vcpkg team, are not terribly interested Win7 support either: it breaks our 'do not implement features in patches' policy. The patch is pretty reasonable now and we have someone who is interested in maintaining it but we are not.

@Hoikas
Copy link
Contributor

Hoikas commented Apr 26, 2023

Except not really, because the version that you get from vcpkg and the version on the system do not necessarily match.

I am speaking about the general case, so vcpkg is not relevant to my statement. Otherwise, we seem to be on largely the same page.

its largely fallout from there not really being an obvious 'correct' way of doing so.

CMake provides a way to get the location of the Python standard library in the FindPython module, and .pyd files are just DLLs with a different extension, FWIW. it's not that it's a particularly hard problem, but it does require some understanding on the part of the build system author to deploy everything correctly. There is an assumption that the embedder has read the documentation and provided a vehicle for loading the aforementioned standard library. Sometimes this can be simply by copying the files into a location in the application install directory or packaging it and providing a loader as specified in PEP 451.

While all of this is not immediately obvious, it is not particularly hard to figure out, especially if you have a modicum of familiarity with the Python ecosystem, which I assume anyone embedding Python in their application should.

@Neumann-A
Copy link
Contributor

Embedding Python

I am not just speaking about embedded python it more general than that.

For example:

  • I have a port for numpy. I can install the scripts into tools/python3/Lib/site-packages and make numpy available for the python installation. However if I try to import numpy I get an error since the dependent DLLs (blas/lapack) cannot be found. I could add code into the init.py script to tell python where to find the dlls but that breaks embedded usage. (since the scripts are no longer relocatable)
  • I have a port for pyside which enables Qt6 usage from python. Same problem.
  • VTK/ITK/ParaView install Python bindings wherever (currently prefix/lib/size-packages according to the file list. There are a few more ports which seem to install python packages without a well defined common layout)
  • FreeCad not only embeds Python it also uses Qt from C++ as well as the Python side. So having consistent qt dlls seems like a must.

Python code as part of the library initialization (a bit of a chicken and egg problem, for sure). From what I can tell, the port(s) being discussed would be broken even with the official Python installer for Windows

The difference there is that I would just pip install whatever instead of building stuff from source.

@BillyONeal BillyONeal merged commit 0a3bc3d into microsoft:master Apr 26, 2023
@jimwang118 jimwang118 deleted the fixpython3_distutils_path branch April 27, 2023 01:34
@BillyONeal
Copy link
Member

@Hoikas , @vicroms @JavierMatosD @dan-shaw @ras0219-msft and I discussed the patching situation today.

@dan-shaw points out that we would be willing to adopt a policy that applies only to Python3 (or other nominal owner of a port) if we can't agree on an overall policy to apply across vcpkg.

@ras0219-msft says that that still means the nominal owner would be on the hook to write such policy even if it applies only to a port.

@JavierMatosD notes that the policy must be consistent with 'global' policies, even in such port 'fiefdoms'.

@BillyONeal notes that policy must include instructions that other contributors can follow, "it must be patches" is insufficient.

======================

Speaking personally, I am weakly against catering to am/format-patch because that workflow triggers updates to unintended to be edited patches, and the workaround in the other direction is not too onerous.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[python3] fix prefix in distutils paths
6 participants