-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[windows] upgrade to Python 3.10.1 embeddable #83488
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
base: main
Are you sure you want to change the base?
[windows] upgrade to Python 3.10.1 embeddable #83488
Conversation
@swift-ci please test |
@@ -349,11 +357,6 @@ $PythonModules = @{ | |||
SHA256 = "353815f59a7f64cdaca1c0307ee13558a0512f6db064e92fe833784f08539c7a"; | |||
Dependencies = @(); | |||
}; | |||
"unittest2" = @{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unittest2
is not compatible with python3.10+ since it's a back port of unittest
. There is no need for it anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets leave that in place until we have switched to 3.10 by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only references to unittest2
in the whole toolchain will be removed in swiftlang/llvm-project#11084. I think it's safe to remove it, as even the 3.9.10
version of Python comes with unittest
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, nice! In that case, we should split this change out and merge it whenever swiftlang/llvm-project#11084 gets merged.
}; | ||
ARM64_Embedded = @{ | ||
URL = "https://www.python.org/ftp/python/3.10.1/python-3.10.1-embed-arm64.zip"; | ||
SHA256 = "1f9e215fe4e8f22a8e8fba1859efb1426437044fb3103ce85794630e3b511bc2"; | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets leave the definitions for 3.9, allowing to optionally upgrade to 3.10 while we iterate to get this working. That will allow you to make progress and get the changes merged earlier rather than trying to keep up to date with the changes in this script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason that we need both the regular and the embedded URLs? Could we not get away with just the embedded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, embeddable python
does not come with pip
. We could download and run get-pip.py
, but I don't think we would gain a lot of CI time in doing so. I have not tried it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reasoning for the version selection here is not CI times but rather to match what llvm.org distributes. I think that we don't need to inject pip
unless we are building the installer for distribution (i.e. nightlies/releases).
Copy-Item -Force ` | ||
-Path "$(Get-EmbeddedPythonPath $HostPlatform)\*" ` | ||
-Destination "$($HostPlatform.ToolchainInstallRoot)\usr\bin" ` | ||
-Recurse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we don't want to embed the Python release. This was what we discussed on the forums - this should be an additional separate MSI that you are building and optionally installing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Embed
was not the right choice of words. I want to put the files in a place where wix can find them for the follow up patch here: swiftlang/swift-installer-scripts#447
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that we should merge this into the toolchain. I think that we should be doing a separate installation: S:\Program Files\Swift\Python-3.10.1
for the image (the WiX authoring should be able to pick it up from there).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the latest revision, the python files are now extracted to S:\Program Files\Swift\Python
. Do we really need the version number? Doing so would mean passing a new parameter to Wix to specify the files path right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer that we keep the version number. Upgrading python is something that would be rare, so I don't think that it is a big deal. We already have the information in build.ps1 where we can pass that down.
3a2b03f
to
bc91500
Compare
bc91500
to
bd4476a
Compare
}; | ||
ARM64_Embedded = @{ | ||
URL = "https://www.python.org/ftp/python/3.10.1/python-3.10.1-embed-arm64.zip"; | ||
SHA256 = "1f9e215fe4e8f22a8e8fba1859efb1426437044fb3103ce85794630e3b511bc2"; | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason that we need both the regular and the embedded URLs? Could we not get away with just the embedded?
@@ -349,11 +357,6 @@ $PythonModules = @{ | |||
SHA256 = "353815f59a7f64cdaca1c0307ee13558a0512f6db064e92fe833784f08539c7a"; | |||
Dependencies = @(); | |||
}; | |||
"unittest2" = @{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, nice! In that case, we should split this change out and merge it whenever swiftlang/llvm-project#11084 gets merged.
@@ -602,6 +615,10 @@ function Get-PythonPath([Hashtable] $Platform) { | |||
return [IO.Path]::Combine("$BinaryCache\", "Python$($Platform.Architecture.CMakeName)-$PythonVersion") | |||
} | |||
|
|||
function Get-EmbeddedPythonPath([Hashtable] $Platform) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, is there a bigger picture around the embedded vs non-embedded python that the extraction needs special logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reason is pip
missing in embeddedable python. We build with the regular Python, but we bundle the embeddable python in the installer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There-in lies the trick! We can inject pip
into the packaging :)
"$($HostPlatform.ToolchainInstallRoot)\usr\lib\swift\windows\_InternalSwiftScan.lib" ` | ||
"$($HostPlatform.ToolchainInstallRoot)\usr\lib" | ||
-Path "$($HostPlatform.ToolchainInstallRoot)\usr\lib\swift\windows\_InternalSwiftScan.lib" ` | ||
-Destination "$($HostPlatform.ToolchainInstallRoot)\usr\lib" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The named parameters are a nice cleanup; we could split those out and merge them as a cleanup before even getting the python 3.10.1 upgrade completed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #83517 to split the changes 👍
This patch upgrades the Python version for LLDB on Windows to 3.10.1. It also embeds Python in the toolchain. This fixes 2 things:
This is a follow-up to this discussion on the forums.
rdar://153842143