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

Add a comment to the x.py shebang, using /usr/bin/env #98747

Closed
wants to merge 2 commits into from

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Jul 1, 2022

This is a terrible, horrible, no-good, very-bad idea.

I kind of like it.


Shebang files can't have comments, so instead this passes --unset VAR, where the variable name itself is the comment.
Here's an example run of py on Windows:

PS C:\Users\Joshua Nelson\src\rust> py x.py
Unable to create process using '/usr/bin/env -S -u If_this_file_fails_to_run,read_README_ENTRYPOINT.md bash ./x.py'

This doesn't break anyone using python/python2/python3 directly, and still handles trying all three on distros where they might not exist. Unfortunately it's slightly less portable since not all env commands support -S and -u.


Happy to take improvements on the file name - I'm slightly reluctant to use spaces or punctuation since lots of shell scripts are bad at handling those, but since nothing should be invoking this file directly other than the kernel, it's probably fine.

Helps with #98650.

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

@jyn514 jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself labels Jul 1, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 1, 2022
@Mark-Simulacrum
Copy link
Member

I feel like it might be simpler (and more instructive) to name the file something like "please_read_this_file.sh", and embed actual suggestions into the file.

My sense was that the auto-discovery mechanism planned(?) or already implemented for the Windows py tooling will look in the Windows equivalent of /usr/bin for the file here -- will that work with a current-directory relative path? Will that work when invoking x.py from a different working directory (e.g., ../x.py), including on linux/macos? (In other words, is the relative path relative to the file being executed?)

@dtolnay
Copy link
Member

dtolnay commented Jul 1, 2022

Will that work when invoking x.py from a different working directory (e.g., ../x.py), including on linux/macos? (In other words, is the relative path relative to the file being executed?)

Linux:

$ ../x.py
/bin/sh: 0: cannot open ./src/bootstrap/if_this_file_fails_to_run_try_using_python3_x.py_instead.sh: No such file

@CAD97
Copy link
Contributor

CAD97 commented Jul 1, 2022

The auto-discovery behavior of py.exe will take the shebang, remove a prefix of /usr/bin/env , /usr/bin/, or /usr/local/bin/, and then search in PATH for an executable with that name an an extension in PATHEXT1. Using a /bin/sh means that py will search for a file called /bin/sh with an autorun extension.

The goal in this PR is to use a shebang which is meaningful to POSIXy shells but will consistently fail and show a kinda-useful error message when run under py.exe.

I'm slightly reluctant to use spaces [...], but since nothing should be invoking this file directly other than the kernel, it's probably fine.

It at least looks like for MINGW, the first space (after a non-space) is split to get the program and a singular argument string. To recover split arguments you need to use env -S.

Will that work when invoking x.py from a different working directory (e.g., ../x.py), including on linux/macos?

In MINGW, a relative path in the shebang is relative to cwd, not the file being interpreted.

Footnotes

  1. The unmodified value is .COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC.

@CAD97
Copy link
Contributor

CAD97 commented Jul 1, 2022

Independently: if this technique can be made to work, it's probably better to recommend python x.py (or just a name README.sh) instead of python3 x.py; the filename should only ever show up on Windows which IIUC distinguishes pythons via path and not via executable name.

@jyn514 jyn514 force-pushed the better-python-error-message branch from fa04454 to 8d13dbe Compare July 1, 2022 02:43
@jyn514
Copy link
Member Author

jyn514 commented Jul 1, 2022

I pushed a modified version of this that respects the current directory. Unfortunately it's a little less portable because it requires env to support both -S and -u - not sure that exists on all distros.

@jyn514 jyn514 changed the title Add src/bootstrap/if_this_file_fails_to_run_try_using_python3_x.py_instead.sh Add a commment to the x.py shebang, using /usr/bin/env Jul 1, 2022
@jyn514 jyn514 force-pushed the better-python-error-message branch from 8d13dbe to 23dcc32 Compare July 1, 2022 02:59
This is a terrible, horrible, no-good, very-bad idea.

I kind of like it.

---

Shebang files can't have comments, so instead this passes `--unset VAR`, where the variable name itself is the comment.
Here's an example run of `py` on Windows:

```
PS C:\Users\Joshua Nelson\src\rust> py x.py
Unable to create process using '/usr/bin/env -S -u If_this_file_fails_to_run,read_README_ENTRYPOINT.md bash ./x.py'
```

This doesn't break anyone using python/python2/python3 directly, and still handles trying all three on distros where they might not exist.
@jyn514 jyn514 force-pushed the better-python-error-message branch from 23dcc32 to f234495 Compare July 1, 2022 03:07
README_ENTRYPOINT.md Outdated Show resolved Hide resolved
Co-authored-by: Christopher Durham <[email protected]>
@m-ou-se
Copy link
Member

m-ou-se commented Jul 1, 2022

Unfortunately it's a little less portable because it requires env to support both -S and -u - not sure that exists on all distros.

-S is not Posix. It seems supported by env on macOS, and by GNU coreutils' env. But at least busybox does not support it.

@Ella-0
Copy link

Ella-0 commented Jul 1, 2022

toybox doesn't support env -S either

```
to `%APPDATA%\py.ini`.

5. Wait until October and update to the latest `py` wrapper, which fixes this bug: https://github.com/python/cpython/issues/94399#issuecomment-1170649407
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a year to this in case someone forgets to update this and it remains in place next year?

@jyn514
Copy link
Member Author

jyn514 commented Jul 9, 2022

Unfortunately it's a little less portable because it requires env to support both -S and -u - not sure that exists on all distros.

-S is not Posix. It seems supported by env on macOS, and by GNU coreutils' env. But at least busybox does not support it.

@jyn514 jyn514 closed this Jul 9, 2022
@dtolnay dtolnay changed the title Add a commment to the x.py shebang, using /usr/bin/env Add a comment to the x.py shebang, using /usr/bin/env Oct 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself 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.

8 participants