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

Use #!/usr/bin/env python3 instead of a bash script in the x.py shebang #98716

Closed
wants to merge 2 commits into from

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Jun 30, 2022

Reverts #98474. This uses Unix-isms that don't work on Windows, and break at least one python entrypoint (py x.py). Instead this uses #!/usr/bin/env python3, which at least breaks fewer systems than python alone.

r? @Mark-Simulacrum cc @dtolnay @CAD97 @yoshuawuyts

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 30, 2022
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Reverting is not the right tradeoff, I think. This breaks things for (most) Linux and macOS users going forwards, since those platforms are deleting python. The only motivation I can identify from your comment in #98474 (comment) is that the error message from Window's py is not good while the error message on Linux/macOS are somewhat better, so we would prefer for the error messages to appear on Linux/macOS rather than Windows, not taking into account the size of each cohort.

We are already exploring other options in #98650, for example documenting that py users will want to stick a py.ini in a particular directory which will make py x.py and .\x.py work with the python.org setup.

@jyn514
Copy link
Member Author

jyn514 commented Jun 30, 2022

I don't think documentation is a good replacement for better error messages. If we can check in a file that does this automatically for all contributors, that seems good.

Another reason for the revert is that your PR breaks existing workflows, while the state of affairs only breaks people starting out for the first time.

From the discussion on the cpython issue it sounds like this is already fixed in the upcoming Python 3.11's py launcher. The release is scheduled for October 2022.

That sounds great. That's quite soon, I think it makes sense to revert until October and reevaluate then.

@dtolnay
Copy link
Member

dtolnay commented Jun 30, 2022

Another reason for the revert is that your PR breaks existing workflows, while the state of affairs only breaks people starting out for the first time.

I don't think this captures the dynamic situation around python. Your point would be compelling if python worked as well today as it did when the x.py shebang was first written in 2016. That is not the case. Whether we break someone or python breaks someone is a technicality that leaves users broken either way.

Everyone upgrading macOS or Linux today is getting broken. My PR makes them not get broken.

I think leaving things in the previous state until October would not be net better.

@jyn514
Copy link
Member Author

jyn514 commented Jun 30, 2022

Everyone upgrading macOS or Linux today is getting broken. My PR makes them not get broken.

If people upgrade their distro, that's a choice they've opted into. If we change x.py, that's a change they just have to live with because they can't modify x.py without git constantly reporting modified files.

I am not sure why there is an urgency to have this done now, instead of a few months from now.

@yoshuawuyts
Copy link
Member

As someone who doesn't know Python well, and just spent the better part of an hour trying to debug why the my builds on windows once again had stopped working, running into this issue was particularly frustrating.

Casting a value-judgement of different types of breaks is hard though, since no platform should feel second-class. But it appears we're faced with somewhat of a zero-sum game, and something's got to give. In general I think the right decision in the short term is to do whatever breaks the fewest number of people's workflows 1. But in the medium/long-term #94829 seems like the right way to categorically eliminate this problem all together.

Footnotes

  1. I think this means keeping some of the windows workflows broken, and not rolling back the changes? But I'm not sure.

@dtolnay
Copy link
Member

dtolnay commented Jun 30, 2022

If we change x.py, that's a change they just have to live with because they can't modify x.py without git constantly reporting modified files.

This is absolutely not the case. As discussed in #98650 and I mentioned above, It appears it'll be a matter of sticking an py.ini somewhere and then both py x.py and .\x.py will work. We are not asking users to edit the x.py shebang each time they want to run the script. While we're figuring out the exact recommendation, in the short term py -3 x.py works.

@Mark-Simulacrum
Copy link
Member

I tend to agree that py vs. py -3 is hopefully not a huge negative for folks, and that the situation now is a net positive over the previous state (on macos and Linux things now work out of the box). For windows, I'd personally rather see us add a x.ps1 or similar as the interim or permanent solution which presumably works for ~everyone; it's pretty non-obvious that the previous state was actually better on Windows in my eyes.

If it helps, we can post a blog or some other vehicle for getting information to users - pinning an issue to rust-lang/rust, for example, seems eminently reasonable.

@jyn514
Copy link
Member Author

jyn514 commented Jun 30, 2022

it's pretty non-obvious that the previous state was actually better on Windows in my eyes.

This bit I don't understand, #98474 (comment) doesn't show anything moving from no to yes on Windows, and py is definitely broken now when it wasn't before.

The rest seems reasonable though - I'm going to see today if I can add some hack that makes the error from py less awful.

@CAD97
Copy link
Contributor

CAD97 commented Jun 30, 2022

That's not quite a correct summary of the Windows behavior. I'm not sure what went wrong with @dtolnay's tests (one thing which catches me all the time: installing software does not update %PATH% in place, and you have to launch a completely new terminal emulator instance to see updates to the path.)

See also further information I've collected in #98650.

Before (#!/usr/bin/env python)

  • ./x.py (pwsh; x.py in cmd) works iff an ftype handler has been registered for .py; this is the case if the “full installer” was used, or if the msstore python was used and a default “Open With” has been manually set to a python interpreter.
  • With a “full installer” python, py ./x.py and python ./x.py work; python2 ./x.py and python3 ./x.py do not.
  • With msstore python, python2 or python3 (as applicable) may work; I tested installing msstore python and did not get a python3 exe in path, but this behavior may be different if the default python3 shim which launches msstore has not been uninstalled.

After (#!/usr/bin/env bash)

  • py ./x.py (and ./x.py if py is the ftype registration) attempt to emulate the behavior of env bash, failing with an unhelpful error if bash is not in path, trying to run /usr/bin/env. (If a bash is available, it is run.)
  • python ./x.py, python3, and python2 work as before.

Which shebangs work on Windows?

Through py.exe, any /usr/bin/env python{version} is documented as launching via py -{version}; this applies with any vcommand shebang. Through an MSYS like shell, the shebang is always interpreted and the env program is always run like on any other POSIX shell.

As such, #! /usr/bin/env python is the most Windows-portable shebang line. python3 is probably also completely acceptable; imho it is not unreasonable to expect a POSIXy shell environment on Windows to provide python3 as a (soft) alias to (the logic of) py -3.

#! /usr/bin/env x where env x fails lookup may start to call the default python when launched via py.exe with Python 3.11+, and may even skip path lookup entirely. It's impossible to say for sure until it releases whether the current implemented behavior diverging from the existing launcher is desired or will be changed to match it. However, trying to envoke /usr/bin/env does seem to be considered a bug.

@CAD97
Copy link
Contributor

CAD97 commented Jun 30, 2022

a x.ps1 or similar as the interim or permanent solution which presumably works for ~everyone

From my side of the OS experience, it seems more reasonable to have
x.py with #!/usr/bin/env python3 and x.sh with #!/usr/bin/env bash than
x.py with #!/usr/bin/env bash and a pwsh-specific x.ps1 for Windows.
If we're going to have shell-specific x entrypoints anyway, then x.py should just be a normal python script rather than a polyglot handling a bash entrypoint as well.

It's also important to note that on Windows, .ps1 is not always an executable script! If run from cmd, the most likely behavior is an "open with" or opening in the default code editor. Even from pwsh (which does interpret x.ps1 files), unless windows developer settings have been changed1, the default pwsh execution policy is to prevent the running of scripts (this is a security feature for end-user machines). The default developer setting is to require a trusted signature for scripts with a Mark of the Web, which git should be setting.

pinning an issue to rust-lang/rust, for example, seems eminently reasonable.

#98650 is a good candidate if we want to do this, and currently has the workaround (py.ini or py -3) as the first thing in the issue.

Footnotes

  1. After installing a clean Win11, I only remembered that this is a thing I needed to do when looking up why git checkouts were not creating real symlinks. This isn't a thing you remember to do until something reminds you that it's necessary (unless you're a sysadmin for developers), because you do it approximately once per computer.

@jyn514
Copy link
Member Author

jyn514 commented Jul 1, 2022

I am leaning towards closing this in favor of #98747.

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2022

it seems more reasonable to have x.py with #!/usr/bin/env python3

I tend to agree. We should at least try this and see how well it works. Basically, revert the polyglott PR but also change the shebang to python3, which should resolve @dtolnay's concern about support for modern Unixy systems.

x.py Outdated Show resolved Hide resolved
Co-authored-by: Ralf Jung <post@ralfj.de>
@jyn514
Copy link
Member Author

jyn514 commented Jul 10, 2022

I am happy to use python3 here instead.

x.py Show resolved Hide resolved
@jyn514 jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jul 10, 2022
@jyn514 jyn514 changed the title Revert "x.py: Support systems with only python3 not python" Use #!/usr/bin/env python3 instead of a bash script in the x.py shebang Jul 10, 2022
@jyn514
Copy link
Member Author

jyn514 commented Jul 31, 2022

I've opened #99992, which is this PR plus shell scripts for people who don't have python3 installed.

@Mark-Simulacrum
Copy link
Member

Going to go ahead and close in favor of #99992 which I think will likely land soon (just needs some CI checks added).

jyn514 added a commit to jyn514/rust that referenced this pull request Aug 8, 2022
This is a more ambitious version of rust-lang#98716.
It still changes the shebang back to python3, for compatibility with non-Unix systems,
but also adds alternative entrypoints for systems without `python3` installed.

These scripts will be necessary for the rust entrypoint (rust-lang#94829), so I see
little downside in adding them early.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 11, 2022
…crum

Add `x.sh` and `x.ps1` shell scripts

This is a more ambitious version of rust-lang#98716.
It still changes the x.py shebang back to python3, for compatibility with non-Unix systems,
but also adds alternative entrypoints for systems without `python3` installed.

These scripts will be necessary for the rust entrypoint (rust-lang#94829), so I see
little downside in adding them early.

I'll update the dev-guide to suggest using these instead of x.py once this is merged.

Fixes rust-lang#98650

r? `@Mark-Simulacrum` cc `@dtolnay` `@CAD97` `@yoshuawuyts`
@pietroalbini pietroalbini deleted the revert-98474-python3 branch November 13, 2022 21:18
@dtolnay dtolnay self-assigned this Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants