-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
x.py: Support systems with only python3
not python
#98474
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
I've never done much development on Windows, but is bash typically available there? FWIW, I believe the current trajectory is to add an x.ps1 PowerShell script (not sure on extension, I think that's right?) which would be ~equivalent to the x.py wrapper, so even if python isn't available adding that might let us move forward regardless. |
cc also python/cpython#90803 |
msys2 and git bash are development platforms that emulates a nix environment on Windows, including common gnu tools and bash, In those environments this file will be run in bash. "Native" Windows shells will simply treat this as a python file. So it will run whatever python version is associated with |
Does this need to use bash rather than sh? |
I tried this in #80585 and it broke because powershell didn't know how to run it. Can you confirm this works on windows with powershell? I had other issues there, but I think they might have been because I intentionally omitted the shebang. IIRC the reason for omitting the shebang is because there's something called |
Oh hm. Right the If python is installed via the Microsoft store, winget, manually, etc then you don't get this "helpful" utility and everything works fine because Windows shells don't read shebangs. If python is installed via the official installer from python.org then that will install the |
@ChrisDenton to be clear, the issue isn't 2 vs 3 - x.py already handles trying to relaunch itself with 3 if necessary. The problem I hit was that |
@jyn514 Yes. Sorry I wasn't clear there. What I'm saying is that the installer sets Or they could set the association to run |
Not a proficient Windows user, but toying around in a VM, I was not able to get
|
I think my powershell result agrees with #71818 (comment): the shebang currently on master already doesn't work: so before this PR we manage to be broken on all modern environments across Linux, macOS, and Windows. 😅 |
@wesleywiser @rylev @ChrisDenton you use Windows and PowerShell regularly, right? Do you normally use |
I think @ChrisDenton answered that in #98474 (comment), and I found the same:
In the "Administrator Command Prompt": |
The |
Btw, here's a quick powershell log after installing from python.org in a fresh sandbox:
On the machine I use for development (using the same x.py as above):
It's possible I did some additional setup though. I'm using the python from the Windows store. |
I think it'd be great to get this line noted somewhere in our documentation -- maybe the rustc-dev-guide at least? We could consider emitting a one-line command if it's not the case (presumably detectable?) from x.py or similar. In the meantime I'm happy with the table (#98474 (comment)) and other comments here sufficiently to let us move forward; we can revisit if this causes regressions in practice. Thanks @dtolnay for pushing this forward! @bors r+ rollup |
📌 Commit 9169905 has been approved by |
x.py: Support systems with only `python3` not `python` Fixes rust-lang#71818 without the pitfalls so far described in previous attempts.
I can follow up with a docs PR. I know almost nothing about Windows beyond what's discussed above and in the issue, so maybe it would be even better if someone regularly doing development on Windows could handle the docs. Here is a draft: PowerShell users: if
Maybe we could put this not just in the rustc-dev-guide but also directly into x.py, since that's probably where I would first look after a failure. |
@jyn514 I use |
Rollup of 8 pull requests Successful merges: - rust-lang#98371 (Fix printing `impl trait` under binders) - rust-lang#98385 (Work around llvm 12's memory ordering restrictions.) - rust-lang#98474 (x.py: Support systems with only `python3` not `python`) - rust-lang#98488 (Bump RLS to latest master on rust-lang/rls) - rust-lang#98491 (Fix backtrace UI test when panic=abort is used) - rust-lang#98502 (Fix source sidebar hover in ayu theme) - rust-lang#98509 (diagnostics: consider parameter count when suggesting smart pointers) - rust-lang#98513 (Fix LLVM rebuild with download-ci-llvm.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
This broke Specifically, running (Windows 11) I'm also fairly certain I did install python via |
In general, it seems suspicious to have a |
I'd prefer to avoid a revert without more information - at the very least, to determine what we can do to test in the future, given that comments on this thread suggest to me that the file should continue to work with py x.py... |
@Mark-Simulacrum I think @dtolnay tested with ./x.py, but not |
Ah, I see. #98474 (comment) has the bash error that's presumably not working - py x.py doesn't work, but apparently py -3 x.py does, and presumably |
I can confirm that the status on my install is (from cmd)
I did install python through winget, but I installed Note that on Windows, the story about when you have to use Because of this, even if I change my The proper fix is for |
I opened an issue: #98650 |
I think we should revert this change. Before this PRUnixYou need Windows
After this PRUnixYou need bash in path, and any of python3, python, or python2 installed. Windows
|
That's not quite a correct summary of the Windows behavior. Before (
|
For completeness I'd add that installing the |
Fixes #71818 without the pitfalls so far described in previous attempts.