-
Notifications
You must be signed in to change notification settings - Fork 207
[windows] add a check for a matching python architecture #1942
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] add a check for a matching python architecture #1942
Conversation
@swift-ci please test |
a1ef87c
to
def835d
Compare
@swift-ci please test |
@@ -1434,6 +1438,42 @@ extension Driver { | |||
|
|||
return (.subcommand(subcommand), updatedArgs) | |||
} | |||
|
|||
private static func checkIfMatchingPythonArch() { |
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 you add a comment here explaining that this exists because users who install the x86 toolchain on arm64 windows will get an extremely cryptic error message otherwise?
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.
Completely forgot to add it after our discussion. Fixed 👍
This is going to be huge usability improvement. I'd like @artemcm to sign off though due the unusual nature of the patch. |
def835d
to
f163a58
Compare
@swift-ci please test |
@swift-ci please test Windows 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.
Is it possible to find a way to do this without executing Python code? e.g. can we locate the python.exe
executable and do something equivalent to what file
does on UNIX?
/// of the Python installation. | ||
/// | ||
/// When installing the x86 toolchain on ARM64 Windows, if the user does not | ||
/// install an x86 version of Python, they will get acryptic error message |
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.
/// install an x86 version of Python, they will get acryptic error message | |
/// install an x86 version of Python, they will get a cryptic error message |
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.
It is quite non-cryptic IMO. 0xC000007B is a NTSTATUS
; 0xC => Kernel; 0x7B = 123 = ERROR_INVALID_NAME. This is the same behaviour as Linux when the loader is not found.
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.
return; | ||
} | ||
if !output.lowercased().contains(arch) { | ||
stderrStream.send("There is an architecture mismatch between the installed toolchain and the resolved Python's architecture:\n") |
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.
Could you please use the driver's diagnosticEngine
and define a new .warning
for this message instead of using the stderr stream directly?
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.
Switched to using the diagnosticsEngine.
#else | ||
return; | ||
#endif | ||
let process = Process() |
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.
By the time we are in invocationRunMode
we have an instance of the DriverExecutor
, so we should plumb that here and use it to run this subprocess instead of using Process
APIs directly. You can see an example of this being done in xcrunFind
.
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.
Switched to using DriverExecutor
.
@@ -1425,6 +1426,9 @@ extension Driver { | |||
if firstArg == "repl" { | |||
updatedArgs.remove(at: 1) | |||
updatedArgs.append("-repl") | |||
#if os(Windows) | |||
checkIfMatchingPythonArch() | |||
#endif |
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.
Does this problem not exist on Darwin as well? If you run arch -m x86_64 swift repl
that should fail similarly.
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 was not able to get the problem to happen on Darwin. arch -x86_64 swift repl
starts the REPL correctly.
Maybe we should bring this check to other platforms as well. I focused on Windows in this patch because all the reports I received were from Windows users on ARM64 machines.
/// of the Python installation. | ||
/// | ||
/// When installing the x86 toolchain on ARM64 Windows, if the user does not | ||
/// install an x86 version of Python, they will get acryptic error message |
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.
It is quite non-cryptic IMO. 0xC000007B is a NTSTATUS
; 0xC => Kernel; 0x7B = 123 = ERROR_INVALID_NAME. This is the same behaviour as Linux when the loader is not found.
#endif | ||
let process = Process() | ||
process.executableURL = URL(fileURLWithPath: "C:\\Windows\\System32\\cmd.exe") | ||
process.arguments = ["/c", "python.exe", "-c", "import platform; print(platform.machine())"] |
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 seems like a pretty bad way to check if the DLL matches. Are we checking if python matches the architecture or if the current runtime as indicated by PYTHONHOME
matchess?
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 are checking that the Python that is resolved from the Path
matches the architecture. I have tried some combinations of configurations:
Everything below was run on an ARM64 Windows VM with the x86 toolchain installed.
Python Path pointing to | PYTHONHOME pointing to | Output |
---|---|---|
ARM64 | Nothing | Crash with warning |
ARM64 | ARM64 | Crash with warning |
ARM64 | AMD64 | Crash with warning |
AMD64 | Nothing | Launches fine |
AMD64 | ARM64 | Fatal Python error: init_fs_encoding |
AMD64 | AMD64 | Launches fine |
I think that checking if the Python that is in the Path
is the correct one is enough. If there is a mismatch between the Python that is in the Path and the one that is in PYTHONHOME
, that is another problem and there are enough information printed to the user to help them figure it out.
There is no non third party alternative to |
If we can reduce the set of dependencies the driver needs in order to perform all the tasks it needs to then we should. Even though Furthermore, as-is, when executing this check in the driver on a system where Python is missing, the user will still get this warning with some surprising I know there is |
To add to the above comment, the driver today on most code paths plans the build without launching any subprocesses at all, which is crucial in some environments where the driver is used as a library in an environment where it is not allowed to launch processes itself. While I'm not aware of this being a hard requirement on Windows today, it's still worthwhile to avoid adding new process launch sites when we can. |
@swift-ci please test |
The COFF header approach seems to work fine 👍 I am unsure what's the best way to resolve the path to |
Looking up in Path is the right thing to do - if it is not in Path, it won't be found anyway. |
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 a couple of small nits, but otherwise this looks good, thank you!
47bc64b
to
ede165c
Compare
Thanks! Fixed the issues and added documentation to the new methods. |
@swift-ci test |
@swift-ci test Windows platform |
@swift-ci test Windows platform |
Odd, the build error happens only on Windows:
Do I need to manually import the method? EDIT: Added the file to |
@swift-ci please test windows platform |
ede165c
to
4c094d5
Compare
@swift-ci please test |
@swift-ci please test windows platform |
static func readWindowsExecutableArchitecture( | ||
cwd: AbsolutePath?, env: [String: String], filename: String | ||
) -> Self { | ||
let searchPaths = getEnvSearchPaths(pathString: env["Path"], currentWorkingDirectory: cwd) |
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.
Environment on Windows is case-insensitive and this WILL lead to very subtle bugs in cases where Path
happens to be cased as PATH
.
Do we have an Environment
abstraction available to SwiftDriver like we have in SwiftPM that we can use instead of a raw [String: String]
dictionary?
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 have TSCBasic.ProcessEnvironmentBlock
which I just transitioned most uses in the driver to: fe4a66c.
Though this is missing a bunch of good stuff that SPM's Environment
has, it does use ProcessEnvironmentKey
which appears to differentiate case sensitivity for Windows in particular. @charles-zablit we should switch to that here.
/// When installing the x86 toolchain on ARM64 Windows, if the user does not | ||
/// install an x86 version of Python, they will get acryptic error message | ||
/// when running lldb (`0xC000007B`). Calling this function before invoking | ||
/// lldb gives them a warning to help troublshoot the issue. |
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.
nit: "troublshoot" mis-spelled
/// of the Python installation. | ||
/// | ||
/// When installing the x86 toolchain on ARM64 Windows, if the user does not | ||
/// install an x86 version of Python, they will get acryptic error message |
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.
nit: "acryptic" => "a cryptic"
Fixes swiftlang/swift#79981.
On Windows, if there is a mismatch between the architecture of the installed Python and the architecture of the toolchain,
swift repl
will crash immediately with0xC000007B
. This patch adds a check to warn the user of the architecture mismatch right before invoking the REPL, which will crash.We should also look into adding a warning at the end of the Swift GUI installer.