-
Notifications
You must be signed in to change notification settings - Fork 47
[windows] add an option to install Embeddable Python 3.10.1 in the toolchain installer #447
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?
Conversation
Version="$(NonSemVerProductVersion)" | ||
Scope="$(PackageScope)"> | ||
|
||
<Media Id="1" Cabinet="$(VariantCabinetName)" EmbedCab="$(ArePackageCabsEmbedded)" /> |
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.
this is not going to build without these variables defined. i would recommend moving this file to a wxi, and defining these variables in a wxs file that imports it. see https://github.com/swiftlang/swift-installer-scripts/blob/main/platforms/Windows/bld/asserts/bld.asserts.wxs
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! However, I think the file structure in swift-installer-scripts\platforms\Windows\python
is not correct: the subdirectory should not be named asserts
. Do you have a suggestion of what this should be named? Or should I just adopt a flat directory structure?
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 think that cpy.wxs
(as this is CPython) is fine as a name.
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.
Right now, the file structure is:
python/
├─ asserts/
│ ├─ python.wxs
│ ├─ python.wixproject
python.wxi
I will rename the python
files to cpy
but what should the asserts
folder be named?
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.
@mhegazy regarding the variant name, I would really like to remove asserts
for Python as it's not an asserts build, just a regular Python distribution. Would the following file structure be OK?
python/
├─ python.wxs
├─ python.wixproject
├─ python.wxi
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 have tried to install the embeddable Python alongside the Asserts
and NoAsserts
directories, with LLDB_PYTHON_HOME=../../Python
and tried other CMake defines like LLDB_PYTHON_EXE_RELATIVE_PATH
.
This does not work because lldb
still tries to look for python310.dll
in the Path.
I see 2 possible solutions going forward:
1
Use Windows APIs to manually add specify the location of python310.dll
before starting lldb
. Since we optionally install Embeddable Python, this adds some hidden logic, if the user uses regular Python instead, in which case the DLL will actually be in the PATH. For this reason, I'm not in favor of this solution.
2
Put the contents of the embeddable Python folder in usr/bin
. We would have something like this:
Asserts/
├─ usr/
│ ├─ bin/
│ │ ├─ lldb.exe
│ │ ├─ python.exe
│ │ ├─ python310.dll
NoAsserts/
├─ usr/
│ ├─ bin/
│ │ ├─ lldb.exe
│ │ ├─ python.exe
│ │ ├─ python310.dll
I have tested this scenario and it works fine.
Now your concern here was regarding this scenario:
Asserts/
├─ usr/
│ ├─ bin/
│ │ ├─ lldb.exe
│ │ ├─ python.exe
│ │ ├─ python310.dll
NoAsserts/
├─ usr/
│ ├─ bin/
│ │ ├─ lldb.exe
In this scenario, the Asserts
variant works fine. The NoAsserts
variant should work fine as well because Asserts/usr/bin
will be in the PATH. If it's not, the user will have to install regular Python manually. I think this is OK because they would have had to install it either way. To I don't think that the following is true.
so if we only install to one, the other variant is broken since lldb expects it to be in the path, and it will not be.
I think we should go with the 2nd solution, because it works with minimal changes to lldb's code.
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 do not think option 2 is a good one. Installing the component per variant is not the best path as we discussed earlier.
Here is the thing, we install usr/bin
in the PATH
. so if we put python.exe
in usr/bin
users will find it if they just run python
. this is not diffrent from putting it in ../../Python
and then adding this to the PATH
so that lldb find it.
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 think I have another option. lldb needs 2 things, python310.dll
since it was linked against it, and cannot load without it, and the rest of the embeded python installation to execute script, and find imports etc..
For the first, we would add a new component under LLDB
ComponentGroup for python310.dll
- this will be in the PATH
since the whole folder is, but it is not an issue since this is the dll and not the python.exe
.
For the second, we would install the new python.msi to the ..\..\Python
as we discussed before, and set LLDB_PYTHON_HOME=../../Python
as part of the installation.
@compnerd what do you think?
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.
That would be problematic as the python.exe
(interpreter) is then going to fail to find the DLL. I think that the best option here is to do a LoadLibraryW
for python3.10.dll
and in the failing case use SetDllDirectory
to inject the additional library search path. I just want to verify that it does not change the DLL lookup order if we do that (as in would block the previous search order from being honoured).
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.
We will be using SetDllDirectory
once the following PR is merged:
</Component> | ||
|
||
<Component Directory="toolchain_$(VariantName)_usr_bin"> | ||
<File Source="$(PythonRoot)\python.exe" /> |
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.
do we really need the executable? i thought that all lldb needed is the dll and the built in modules?
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, we should include thee executable. Ultimately, the issue with all this is that LLDB is meant to be usable as a scriptable debugger, including for post-mortem scenarios. In such a case, you want to be able to execute a script, which is going to require the interpreter.
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.
do we really need the executable? i thought that all lldb needed is the dll and the built in modules?
On Windows, following this discussion, we want to bundle an executable Python with the toolchain installer.
Scope="$(PackageScope)"> | ||
|
||
<?define PlatformRoot = "$(ImageRoot)\Platforms\Windows.platform"?> | ||
<?define PythonRoot = "$(ImageRoot)\python"?> |
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.
this should be dedined in python msi project and not here.
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.
+1
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!
<ProjectReference Include="..\bld\asserts\bld.asserts.wixproj" BindName="bld.asserts" /> | ||
<ProjectReference Include="..\cli\asserts\cli.asserts.wixproj" BindName="cli.asserts" /> | ||
<ProjectReference Include="..\dbg\asserts\dbg.asserts.wixproj" BindName="dbg.asserts" /> | ||
<ProjectReference Include="..\python\python.wixproj" BindName="python" /> |
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 shuffle this down after all ide
MSI. I wonder if it makes sense to rename this to match the 3-letter naming.
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 moved it down below ide
. Should it be at the very bottom?
Regarding the 3-letter renaming, I'm not opposed to it, however I think it would be good to differentiate between embedded Python and regular Python if we ever decide to bundle/chain the full Python msi with the toolchain installer.
If we are sure we are never going to chain the full Python msi, then we could name this py3
?
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 am certainly curious about why the differentiation matters.
Ultimately, the difference between the embedded and full python is something that I think that we should somewhat blur. The actual difference is the lack of an installer (not a problem), a slightly smaller standard library (is that truly a concern?), and the missing pip
which we can inject. So, difference-wise it is pretty small.
They also do not author MSIs, which means that the chaining would be more complicated than if they provided a MSM/MSI.
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.
and the missing pip which we can inject. So, difference-wise it is pretty small.
Embedded Python does not support pip
.
I think the scope of this patch is to fix the issues with users being confused by the missing python310.dll
, where lldb crashes immediately (without an error message when executed from powershell):
- [Windows] lldb fails to start (missing python36.dll) llvm/llvm-project#43432
- lldb binary does not launch on Windows llvm/llvm-project#47381
- on windows, missing file for lldb llvm/llvm-project#53646
- Official packaging for Windows do not include Python dependencies for lldb llvm/llvm-project#58095
- [lldb, Windows 11] LLDB executable crashes on every invocation llvm/llvm-project#59524
- Missing Python DLL in Windows binary llvm/llvm-project#74073
- windows: lldb-dap doesnt start, is missing python310.dll llvm/llvm-project#85764
There is a PR in llvm/llvm-project
to address those crashes:
I might be misunderstanding, but I don't see the benefits of having a constrained version of Python with an unsupported pip
installation that is not in the PATH
. It seems reasonable, to me, to expect that a user who wants to do lldb
Python scripting has a fully configured Python on their system.
platforms/Windows/bundle/theme.xml
Outdated
<Checkbox Name="OptionsInstallAndroidSDKAMD64" X="210" Y="363" Width="-11" Height="17" TabStop="yes" FontId="3" EnableCondition="OptionsInstallAndroidPlatform">#(loc.Sdk_ProductName_Android_amd64)</Checkbox> | ||
<Checkbox Name="OptionsInstallAndroidSDKARM" X="210" Y="381" Width="-11" Height="17" TabStop="yes" FontId="3" EnableCondition="OptionsInstallAndroidPlatform">#(loc.Sdk_ProductName_Android_armv7)</Checkbox> | ||
<Checkbox Name="OptionsInstallAndroidSDKX86" X="210" Y="399" Width="-11" Height="17" TabStop="yes" FontId="3" EnableCondition="OptionsInstallAndroidPlatform">#(loc.Sdk_ProductName_Android_x86)</Checkbox> | ||
<Checkbox Name="OptionsInstallEmbeddedPython" X="192" Y="165" Width="-11" Height="17" TabStop="yes" FontId="3">#(loc.EmbeddedPython_ProductName)</Checkbox> |
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 think that this should come at the very end as it is not really part of the toolchain, it is a third party dependency we are 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.
Fixed, thanks 👍
Scope="$(PackageScope)"> | ||
|
||
<?define PlatformRoot = "$(ImageRoot)\Platforms\Windows.platform"?> | ||
<?define PythonRoot = "$(ImageRoot)\python"?> |
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.
+1
platforms/Windows/python/python.wxs
Outdated
Name="$(VariantProductName)" | ||
UpgradeCode="$(VariantUpgradeCode)" | ||
Version="$(NonSemVerProductVersion)" | ||
Scope="$(PackageScope)"> |
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.
How do we version this component? Should we version it as per the Python version or the Swift toolchain version?
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 think we should version it per the Python version, if we don't change the Python version between Swift versions, there is no need to reinstall Python.
</Component> | ||
|
||
<Component Directory="toolchain_$(VariantName)_usr_bin"> | ||
<File Source="$(PythonRoot)\python.exe" /> |
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, we should include thee executable. Ultimately, the issue with all this is that LLDB is meant to be usable as a scriptable debugger, including for post-mortem scenarios. In such a case, you want to be able to execute a script, which is going to require the interpreter.
d806397
to
91159fc
Compare
<?define VariantCabinetName = python.asserts.cab?> | ||
<?define ToolchainVersionedVariantDirectory = ToolchainVersionedAsserts ?> | ||
<?define VariantEnvironmentComponentGUID = 30629e0c-b376-47bc-bedf-fefb7d4ca61d?> | ||
<?if $(ProductArchitecture) = "arm64" ?> |
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.
this is not variant specific. i would recommend moving it to the wxi to avoid it being duplicated for diffrent variants.
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.
Yeah, this seems like we should sink it into the wxi.
<?define VariantCabinetName = python.asserts.cab?> | ||
<?define ToolchainVersionedVariantDirectory = ToolchainVersionedAsserts ?> | ||
<?define VariantEnvironmentComponentGUID = 30629e0c-b376-47bc-bedf-fefb7d4ca61d?> | ||
<?if $(ProductArchitecture) = "arm64" ?> |
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.
Yeah, this seems like we should sink it into the wxi.
796d69c
to
a0e6271
Compare
a0e6271
to
66022f0
Compare
<Checkbox Name="OptionsInstallAssertsToolchain" X="210" Y="435" Width="-11" Height="17" TabStop="yes" FontId="3" EnableCondition="OptionsIncludeNoAsserts and OptionsInstallNoAssertsToolchain">#(loc.Asserts_Toolchain_ProductName)</Checkbox> | ||
<Checkbox Name="OptionsInstallNoAssertsToolchain" X="210" Y="453" Width="-11" Height="17" TabStop="yes" FontId="3" EnableCondition="OptionsIncludeNoAsserts and OptionsInstallAssertsToolchain">#(loc.NoAsserts_Toolchain_ProductName)</Checkbox> | ||
|
||
<Checkbox Name="OptionsInstallEmbeddedPython" X="192" Y="471" Width="-11" Height="17" TabStop="yes" FontId="3" EnableCondition="OptionsInstallDBG">#(loc.EmbeddedPython_ProductName)</Checkbox> |
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.
can we nest this under OptionsInstallDBG
?
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.
@mhegazy, just to be clear - you mean nesting as in indented?
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.
correct. a few changes are included in that:
- move it up so that it is immediately under
OptionsInstallDBG
- indent it so it looks like a sub feature
- set the enableCondition to track
OptionsInstallDBG
(EnableCondition="OptionsInstallDBG"
), so if the user did not chooseOptionsInstallDBG
in the first place, we do not allow them to choose to install python.
And if we do that we should also change the install condition in installer.wxs
from InstallCondition="OptionsInstallEmbeddedPython = 1"
to be InstallCondition="OptionsInstallEmbeddedPython = 1 and OptionsInstallDBG = 1"
this way we only install it if both options are set
@@ -0,0 +1,15 @@ | |||
<Wix xmlns="http://wixtoolset.org/schemas/v4/wxs"> |
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 need for spliting the file into python.wxs
and python.wxi
. just have them all in python.wxs
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.
That's what I originally did but I ended up splitting it into 2 files on your advice here: #447 (comment)
I'm happy to revert it, I think having only one file makes more sense given the small size of the 2 files.
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.
That was my bad, originally i assumed we wanted to ship them as part of the toolchain
folder which is keyed on the variant. but since this is sitting in its own folder now there should not be any need to depend on the variant.
appologies for giving incorrect suggessions earlier.
66022f0
to
497fa7e
Compare
<Component> | ||
<File Source="$(PythonRoot)\libffi-7.dll" /> | ||
</Component> | ||
|
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.
Why the extra whitespace?
<ComponentGroupRef Id="EmbeddedPythonLicense" /> | ||
</Feature> | ||
</Package> | ||
</Include> No newline at end of file |
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.
Missing newline
<?define VariantUpgradeCode = $(PythonUpgradeCode)?> | ||
<?define VariantProductName = !(loc.EmbeddedPython_ProductName)?> | ||
<?define VariantCabinetName = python.cab?> | ||
<?define VariantEnvironmentComponentGUID = 30629e0c-b376-47bc-bedf-fefb7d4ca61d?> |
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 think that we can inline these values and I don't think that we need the Variant
prefix as we do not have variants.
This patch adds an option to install the embeddable version of Python directly from the toolchain installer on Windows.
The installer will pick up the python files from
T:\Program Files\Swift\Python
and install it intoolchain-install-dir/Python-3.x.x
.This PR depends on the following being merged and cherry-picked:
rdar://157773554