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] Update to 3.11.4 #31727

Merged
merged 41 commits into from
Aug 7, 2023
Merged

[python3] Update to 3.11.4 #31727

merged 41 commits into from
Aug 7, 2023

Conversation

Osyotr
Copy link
Contributor

@Osyotr Osyotr commented May 31, 2023

testing...

@LilyWangLL LilyWangLL added the category:port-update The issue is with a library, which is requesting update new revision label May 31, 2023
Comment on lines 152 to 154
vcpkg_find_acquire_program(PYTHON3)
get_filename_component(PYTHON3_DIR "${PYTHON3}" DIRECTORY)
set(ENV{PythonForBuild} "${PYTHON3_DIR}/python.exe")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we do this unconditionally? vcpkg_find_acquire_program(PYTHON3) should give us a Python that will run on the host 😕.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is python3 now a bootstrapping requirement for native builds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Hoikas we can. I'm just little worried whether vcpkg_find_acquire_program will always give us python of required version (right now it's 3.10 which is ok for bootstrapping)
@dg0yt I think it started with this commit python/cpython@09b4ad1
Theoretically this change can be omitted if we allow find_python.bat to use system python or download it from nuget (failed in CI for some reason).

Copy link
Contributor

Choose a reason for hiding this comment

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

If python is needed now, there is only vcpgk_find_acquire_program (if version requirements are relaxed) or vcpkg_download_distfile for a specific download.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is python3 now a bootstrapping requirement for native builds?

Python has needed a working python interpreter for its own build since before Python 3.7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

potential build issues in ports that depend on Python 3 when the interpreter from vcpkg_find_acquire_program(PYTHON3) does not match the libraries from the python3 port.

Hmm could you give an example? Or confirm that my understanding is correct:

  1. Port A depends on python3 and produces python modules
  2. Port B uses vcpkg_find_acquire_program(PYTHON3) and uses this interpreter to import modules from port A.
    Shouldn't it use python from tools instead (probably with patches from [python3] Some patches up for discussion  #31279)?

Copy link
Contributor

Choose a reason for hiding this comment

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

This case would be that port foo depends on python3 to build Python modules. It also depends on python.exe as part of its build process to perform operation bar at build time. Currently, vcpkg uses the python.exe provided by vcpkg_find_acquire_program(PYTHON3) for this. If the port's upstream CMake requests the development modules and Python interpreter simultaneously and vcpkg_find_acquire_program(PYTHON3) and python3 do not match, CMake will reject this. The results of this rejection are difficult to determine but could include failing the port build or finding the system Python.

Option 2 is probably closest to this situation, but we don't have a Python tool port yet. That should be addressed by #31748, if I read it correctly.

Copy link
Member

Choose a reason for hiding this comment

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

I'm just little worried whether vcpkg_find_acquire_program will always give us python of required version (right now it's 3.10 which is ok for bootstrapping)

This is not correct. vcpkg_find_acquire_program will give you a version of python that you can use as an interpreter that satisfies constraints. On most POSIX, for example, it's going to give you /usr/bin/python3 which will basically never have a version that matches what the port wants to build from source. Note that making vcpkg-tool-python3 won't change anything about this situation: we have both scenarios that want to

  • load in to python, where the versions of everything need to match and thus we must build from source, and
  • just want to use python to run a .py, which usually do not want to build python from source, and thus are going to get whatever the system says
  • (There's another scenario about embedding python with an application that looks closer to the first one, but I don't have a good understanding of what's going on there)

Ports need to make the judgement call about which of these cases they are in, and depend on the python port in the first case and call vcpkg_find_acquire_program in the second.

The guarantee that you appear to be attempting to make here, that the 'tool' copy of python will match the 'built from source' copy of python, is not a guarantee that we are capable of making. The problem is that we don't have a good enough understanding of what you are trying to do with that guarantee to suggest what the right approach going forward is.

Copy link
Member

Choose a reason for hiding this comment

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

  1. Port B uses vcpkg_find_acquire_program(PYTHON3) and uses this interpreter to import modules from port A.

Port B cannot do this. vcpkg_find_acquire_program(PYTHON3) does not give a version of python that has any relationship with the modules built by Port A. Port B must also depend on the python3 port and use the interpreter built by it, not what vcpkg_find_acquire_program(PYTHON3) says.

Copy link
Contributor

@ras0219-msft ras0219-msft Aug 2, 2023

Choose a reason for hiding this comment

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

Add'l note onto what @BillyONeal said above: Port B may need to depend on both python3 for the host as well as python3 for the target. This will give you a python.exe executable on the current system with the same source version as a python.lib for the target (to use Windows nomenclature).

[
    { "name": "python3", "host": true },
    "python3"
]

Comment on lines 152 to 154
vcpkg_find_acquire_program(PYTHON3)
get_filename_component(PYTHON3_DIR "${PYTHON3}" DIRECTORY)
set(ENV{PythonForBuild} "${PYTHON3_DIR}/python.exe")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is python3 now a bootstrapping requirement for native builds?

ports/python3/portfile.cmake Outdated Show resolved Hide resolved
ports/python3/portfile.cmake Show resolved Hide resolved
ports/python3/portfile.cmake Show resolved Hide resolved
@@ -1,19 +1,24 @@
if(CMAKE_HOST_WIN32)
set(program_name python)
set(program_version 3.10.7)
set(program_version 3.11.3)
if(VCPKG_TARGET_ARCHITECTURE STREQUAL "x86")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't this check for host architecture instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should, but ...

  • it is not a regression ;-),
  • there is no VCPKG_HOST_ARCHITECTURE, because host triplets are not loaded,
  • parsing of triplet names is not desired,
  • CMAKE_HOST_SYSTEM_PROCESSOR could be uninitialized in script mode.

This leaves ENV{PROCESSOR_ARCHITECTURE} etc. for WIN32 hosts. Cf. CLANG and GN scripts.

@JavierMatosD
Copy link
Contributor

@Osyotr
Okay, the only thing that is still in contention is the changes to gobject-introspection that seem unrelated to the python update. In the spirit of getting the Python update through, would you mind splitting out the gobject-introspection changes along with the changes to vcpkg_find_acquire_program() into a separate PR?

Additionally, I think we should drop the Windows 7 backcompat patch. It's been two years since support for Windows 7 ended, and these changes appear to be related to security.

@JavierMatosD JavierMatosD marked this pull request as draft August 3, 2023 21:34
@Hoikas
Copy link
Contributor

Hoikas commented Aug 3, 2023

Please do not drop the Windows 7 patch. A significant plurality of our end users still use this OS, even though it is EOL 😞. My understanding from @BillyONeal and @ras0219-msft is that this patch is acceptable in vcpkg as long as someone is willing to maintain it.

@BillyONeal
Copy link
Member

Please do not drop the Windows 7 patch. A significant plurality of our end users still use this OS, even though it is EOL 😞. My understanding from @BillyONeal and @ras0219-msft is that this patch is acceptable in vcpkg as long as someone is willing to maintain it.

Is someone signing up to fix the patch?

@Osyotr
Copy link
Contributor Author

Osyotr commented Aug 3, 2023

Additionally, I think we should drop the Windows 7 backcompat patch. It's been two years since support for Windows 7 ended, and these changes appear to be related to security.

This patch does not change observable behavior for users with Windows 10 or later.
Can we please keep the patch for at least until the next update?

Is someone signing up to fix the patch?

I already fixed the patch for 3.11.

@JavierMatosD
Copy link
Contributor

JavierMatosD commented Aug 4, 2023

Additionally, I think we should drop the Windows 7 backcompat patch. It's been two years since support for Windows 7 ended, and these changes appear to be related to security.

This patch does not change observable behavior for users with Windows 10 or later. Can we please keep the patch for at least until the next update?

Since you've already fixed the patch for this update, we can keep it until the next one. However, please add a big honking comment in the portfile saying, "THIS WILL BE REMOVED ON THE NEXT SOURCE UPDATE." I'm sorry, but I actually think we need to remove the patch. This seems to be exposing a known CVE

@Hoikas
Copy link
Contributor

Hoikas commented Aug 4, 2023

However, please add a big honking comment in the portfile saying, "THIS WILL BE REMOVED ON THE NEXT SOURCE UPDATE."

@JavierMatosD I'm afraid I don't understand your comment. As I understand it, the position of the maintainers as expressed by @BillyONeal is that as long as contributors are willing to maintain the Windows 7 patch for this port, then it is acceptable. Has the official stance of the vcpkg maintainers changed? From what I can tell, @Osyotr has been very willing to do the work here, and I have previously been involved with maintaining the patch and port.

@LilyWangLL LilyWangLL removed the info:reviewed Pull Request changes follow basic guidelines label Aug 4, 2023
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

Is someone signing up to fix the patch?

I already fixed the patch for 3.11.

Sorry for my underinformed question about this I posted before; I didn't read all the history before, I just was replying to the specific question and was unaware of the security relevant discussion. I just spoke with @JavierMatosD about this in person and he told me about some of the context.

As I understand it, the position of the maintainers as expressed by @BillyONeal is that as long as contributors are willing to maintain the Windows 7 patch for this port, then it is acceptable. Has the official stance of the vcpkg maintainers changed? From what I can tell, @Osyotr has been very willing to do the work here, and I have previously been involved with maintaining the patch and port.

The policy has never been "we are OK with any patch as long as someone is willing to do the work to maintain the patch". At one time, we looked at the patch that was required to keep XP support, and it looked like it was just adding polyfills around some relatively minor Windows APIs. It was a bigger patch than we would normally have accepted. However, a motivated contributor was present who signed up to do that work, and we were clear that in future version updates, the patch might go away.

The current rules for patches we accept have not changed much over time. Currently they are located in the maintainer guide: https://learn.microsoft.com/en-us/vcpkg/contributing/maintainer-guide#patching . Quoting that:

We want to avoid patches that:

  • upstream would disagree with
  • cause vulnerabilities or crashes
  • we are incapable of maintaining across upstream version updates
  • are large enough to cause license entanglement with the vcpkg repository itself

The difference between before and today is that the patch now contains security relevant content. This appears to be explicitly undoing a fix for https://nvd.nist.gov/vuln/detail/CVE-2020-8315 (thanks to @JavierMatosD who found python/cpython#18231 ). That means resolving the third bullet by having someone signed up to do the work of maintaining the patch across versions is no longer sufficient, the first and second bullet are affected. Clearly, if Python is telling their customers 'Python 3.8.2 and later have a fix for CVE-2020-8315', and we are giving them something we claim to be Python 3.11 which does not have a fix for CVE-2020-8315, upstream is going to not like that. It also is explicitly creating a vulnerability, which entangles the second bullet.

Similarly, if a vcpkg customer gets exploited by an attacker using CVE-2020-8315 because we explicitly undid the fix for that, that's a Serious problem. We aren't qualified to review some other fix that claims to fix CVE-2020-8315 while retaining Win7 support. (I also made another comment on why I think the patch is questionable)

As a result I am personally strongly against keeping the patch.

Additionally, I think we should drop the Windows 7 backcompat patch. It's been two years since support for Windows 7 ended, and these changes appear to be related to security.

This patch does not change observable behavior for users with Windows 10 or later. Can we please keep the patch for at least until the next update?

If the other maintainers are willing to keep it one more version as a gesture of goodwill to contributors who have done most of the python port maintenance, I don't like it but at least understand it.

@Osyotr
Copy link
Contributor Author

Osyotr commented Aug 4, 2023

@Hoikas I've checked whether it's feasible to keep the patch for 3.12 and this commit python/cpython@0f17576 uses new Windows API that I don't know how to replace correctly.
In particular, FILE_ID_INFO is available only in Windows 8 and later.

Otherwise 3.12 builds fine with just minor adjustments to patches. :)

@Hoikas
Copy link
Contributor

Hoikas commented Aug 4, 2023

@Osyotr I'm unfamiliar with that API... I'll try to look into it before Python 3.12 comes out. I'm a little worried that, some day, we may not be able to easily polyfill our way out of the problems :(

The difference between before and today is that the patch now contains security relevant content. This appears to be explicitly undoing a fix for https://nvd.nist.gov/vuln/detail/CVE-2020-8315 (thanks to @JavierMatosD who found python/cpython#18231 ).

@BillyONeal @JavierMatosD This is incorrect. The patch is reverting, only on Windows 7, to the method previously used in Python 3.8.2 and higher. When I initially proposed this patch, I asked @ras0219-msft (in discord) to review it due to the "security content", and he approved of it because it matched the previous behavior of the CVE fix from Python 3.8. The patch is still just polyfilling Windows APIs. The only difference is that, as Python continues to evolve without Windows 7 support, we have to continue replicating the polyfills from Python 3.8.2, which means the patch grows in size. Eventually, it will probably become too much effort to be worthwhile. Until then, I'd appreciate it if we retained the patch and all parties fully informed themselves of the history of the port's development and previous reviews thereof.

At one time, we looked at the patch that was required to keep XP support,

The patch adds Windows 7 support, not Windows XP support.

@Osyotr Osyotr marked this pull request as ready for review August 7, 2023 15:05
@JavierMatosD
Copy link
Contributor

JavierMatosD commented Aug 7, 2023

@Osyotr I'm unfamiliar with that API... I'll try to look into it before Python 3.12 comes out. I'm a little worried that, some day, we may not be able to easily polyfill our way out of the problems :(

The difference between before and today is that the patch now contains security relevant content. This appears to be explicitly undoing a fix for https://nvd.nist.gov/vuln/detail/CVE-2020-8315 (thanks to @JavierMatosD who found python/cpython#18231 ).

@BillyONeal @JavierMatosD This is incorrect. The patch is reverting, only on Windows 7, to the method previously used in Python 3.8.2 and higher. When I initially proposed this patch, I asked @ras0219-msft (in discord) to review it due to the "security content", and he approved of it because it matched the previous behavior of the CVE fix from Python 3.8. The patch is still just polyfilling Windows APIs. The only difference is that, as Python continues to evolve without Windows 7 support, we have to continue replicating the polyfills from Python 3.8.2, which means the patch grows in size. Eventually, it will probably become too much effort to be worthwhile. Until then, I'd appreciate it if we retained the patch and all parties fully informed themselves of the history of the port's development and previous reviews thereof.

At one time, we looked at the patch that was required to keep XP support,

The patch adds Windows 7 support, not Windows XP support.

Okay, it looks like the patch addresses the specific, known CVE. However, the problem is that today, two years after support has been dropped, we have no idea what other CVEs lurk in the codebase and we aren't qualified to review python security fixes. Additionally, we know that this patch will continue to grow over time. After speaking with @ras0219-msft, he agreed to allow the patch to stay until the next update with the condition that we put a big comment in the portfile saying it will be removed on the next update.

@JavierMatosD JavierMatosD marked this pull request as draft August 7, 2023 18:05
@Osyotr
Copy link
Contributor Author

Osyotr commented Aug 7, 2023

put a big comment in the portfile saying it will be removed on the next update

Could someone from vcpkg team do that? Touching python port at this point will result in another 24h half-world rebuild.

@JavierMatosD JavierMatosD marked this pull request as ready for review August 7, 2023 20:19
@JavierMatosD JavierMatosD merged commit c6928df into microsoft:master Aug 7, 2023
15 checks passed
@Osyotr Osyotr deleted the py3_update branch August 7, 2023 20:22
@Neumann-A
Copy link
Contributor

There is definitively a bug here:

         Das Verzeichnis "E:\vcpkg_folders\master_clean\buildtrees\python3\x86-windows-dbg\v3.11.4-072e9c7c83.clean\PCbuild\obj\311amd64_Debug\_freeze_module\" wird erstellt.
    28>Die Erstellung von Projekt "E:\vcpkg_folders\master_clean\buildtrees\python3\x86-windows-dbg\v3.11.4-072e9c7c83.clean\PCbuild\_freeze_module.vcxproj" ist abgeschlossen (Clean Ziel(e)).
     1>Das Projekt "E:\vcpkg_folders\master_clean\buildtrees\python3\x86-windows-dbg\v3.11.4-072e9c7c83.clean\PCbuild\pcbuild.proj" (1) erstellt "E:\vcpkg_folders\master_clean\buildtrees\python3\x86-windows-dbg\v3.11.4-072e9c7c83.clean\PCbuild\_freeze_module.vcxproj" (28:2) auf Knoten "6", Build Ziel(e).
    28>PrepareForBuild:
         Das Verzeichnis "E:\vcpkg_folders\master_clean\buildtrees\python3\x86-windows-dbg\v3.11.4-072e9c7c83.clean\PCbuild\amd64\" wird erstellt.
         Das Verzeichnis "E:\vcpkg_folders\master_clean\buildtrees\python3\x86-windows-dbg\v3.11.4-072e9c7c83.clean\PCbuild\obj\311amd64_Debug\_freeze_module\_freeze_module.tlog\" wird erstellt.
       InitializeBuildStatus:
         "E:\vcpkg_folders\master_clean\buildtrees\python3\x86-windows-dbg\v3.11.4-072e9c7c83.clean\PCbuild\obj\311amd64_Debug\_freeze_module\_freeze_module.tlog\unsuccessfulbuild" wird erstellt, da "AlwaysCreate" angegeben wurde.
       ClCompile:
         C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.36.32532\bin\HostX64\x64\CL.exe /c /I"E:\vcpkg_folders\master_clean\buildtrees\python3\x86-windows-dbg\v3.11.4-072e9c7c83.clean\Include" /I"E:\vcpkg_folders\master_clean\buildtrees\python3\x86-windows-dbg\v3.11.4-072e9c7c83.clean\Include\internal" /I"E:\vcpkg_folders\master_clean\buildtrees\python3\x86-windows-dbg\v3.11.4-072e9c7c83.clean\PC" /I"E:\vcpkg_folders\master_clean\buildtrees\python3\x86-windows-dbg\v3.11.4-072e9c7c83.clean\PCbuild\obj\311amd64_Debug\_freeze_module\\" /I"E:/vcpkg_folders/master_clean/installed/x86-windows/include" /Zi /nologo /W3 /WX- /diagnostics:column /FS /Od /Ob1 /Oi /D _Py_HAVE_ZLIB /D Py_NO_ENABLE_SHARED /D Py_BUILD_CORE /D _CONSOLE /D WIN32 /D "PY3_DLLNAME=L\"python3_d\"" /D _WIN64 /D _M_X64 /D _DEBUG /D _UNICODE /D UNICODE /GF /Gm- /MDd /GS /Gy /Zc:wchar_t /Zc:forScope /Zc:inline /Fo"E:\vcpkg_folders\master_clean\buildtrees\python3\x86-windows-dbg\v3.11.4-072e9c7c83.clean\PCbuild\obj\311amd64_Debug\_freeze_module\\" /Fd"E:\vcpkg_folders\master_clean\buildtrees\python3\x86-windows-dbg\v3.11.4-072e9c7c83.clean\PCbuild\obj\311amd64_Debug\_freeze_module\vc143.pdb" /external:W3 /Gd /TC /FC /errorReport:queue /utf-8  "..\Modules\atexitmodule.c"

x86-windows is building a x64 application that shouldn't happen!
And the reason is:

    <FreezeProjects>
      <Platform>$(PreferredToolArchitecture)</Platform>
      <Configuration>$(Configuration)</Configuration>
      <Configuration Condition="$(Configuration) == 'PGInstrument'">Release</Configuration>
      <Properties></Properties>
      <BuildTarget>Build</BuildTarget>
      <CleanTarget>Clean</CleanTarget>
      <CleanAllTarget>CleanAll</CleanAllTarget>
      <BuildInParallel>false</BuildInParallel>
    </FreezeProjects>

so python3 is currently building stuff for the host and not the target.

@Hoikas
Copy link
Contributor

Hoikas commented Aug 9, 2023

@JavierMatosD

Okay, it looks like the patch addresses the specific, known CVE. However, the problem is that today, two years after support has been dropped, we have no idea what other CVEs lurk in the codebase and we aren't qualified to review python security fixes.

I agree. Thankfully, Python is maintaining its code upstream (3.8 is not EOL until October 2024), and the security patching has already been done by Python upstream.

Additionally, we know that this patch will continue to grow over time. After speaking with @ras0219-msft, he agreed to allow the patch to stay until the next update with the condition that we put a big comment in the portfile saying it will be removed on the next update.

Would it be helpful if all of the polyfills were pulled out into a separate file and set of clearly defined functions instead of replicated verbatim each time? There is nothing different here (except for the length of the patch) than when this patch was initially proposed and the understanding was that the patch was acceptable as long as contributors maintained it. As it stands, the maintainers are making a seemingly invalid/incorrect argument to justify removing a patch that many of our end users are depending on. If vcpkg drops this patch, we will be in a bad position with our users and in a sticky spot with certain stakeholders. I find this seeming reversal of policy quite frustrating, but I would like to find a way forward where we can continue to maintain this functionality for as long as we, as contributors, can, as per the initial understanding.

@Neumann-A
Copy link
Contributor

many of our end users are depending on

Which industry/country is this?

I would like to find a way forward where we can continue to maintain this functionality for as long as we, as contributors, can, as per the initial understanding.

I mean there is always the possibility to provide an port overlay or an version override.

Python is maintaining its code upstream

maybe that should have an overlay than?

@JavierMatosD
Copy link
Contributor

There is nothing different here (except for the length of the patch) than when this patch was initially proposed and the understanding was that the patch was acceptable as long as contributors maintained it. As it stands, the maintainers are making a seemingly invalid/incorrect argument to justify removing a patch that many of our end users are depending on. If vcpkg drops this patch, we will be in a bad position with our users and in a sticky spot with certain stakeholders. I find this seeming reversal of policy quite frustrating, but I would like to find a way forward where we can continue to maintain this functionality for as long as we, as contributors, can, as per the initial understanding.

@Hoikas, I empathize with your frustration and the importance of this patch. However, we will be dropping the patch in the upcoming update. This decision is based on the evolution of the patch and not on a change of policy.

When the patch was first introduced, its primary purpose was to reintroduce the Win7 fix for Python, which was straightforward and concise. Over the past two years, the patch has grown considerably in size and complexity. Its current state is significantly different from its initial version, making it challenging to maintain and at odds with upstream.

We genuinely want to ensure that you aren't being left to dry. Before the next update, we recommend two potential solutions:

  1. Overlay the port, allowing you to maintain a version of the port with the patch applied.
  2. Create a custom registry, giving you more control over the versions and patches applied to the ports you use.

Please feel free to reach out if neither solution works for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants