-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix infinite recursion by sniffing the shebang #6711
Fix infinite recursion by sniffing the shebang #6711
Conversation
When we see `#!/usr/bin/env -S uv run"`, treat the input as a python file to avoid infinite recursion. Tested manually. Fixes #6360
@@ -882,6 +883,20 @@ impl TryFrom<&ExternalCommand> for RunCommand { | |||
{ | |||
Ok(Self::PythonGuiScript(target_path, args.to_vec())) | |||
} else { | |||
// Sniff the shebang to avoid infinite recursion | |||
let known_shebang = br"#!/usr/bin/env -S uv run"; |
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 wouldn't take into account for cases where e.g. env -S
isn't supported (older version of coreutils), where some black magic or custom script (e.g. named uv-run
in my case) is used in the shebang.
Also it wouldn't be completely alien to want to hardcode a path to uv
instead of relying to env
.
In both of these cases the shebang wouldn't match, and this heuristic would not completely fix the infinite recursion 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.
I also worry this heuristic is too brittle. I'm not sure what the solution is without peeking. Some sort of environment variable?
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.
My two cents as I'm still discovering and playing with uv
, I'd be happy with an alternative entry point (e.g. uv exec
) which would only accept files as input.
It seems to me that this whole issue is made complex by uv run
also being expected to run 'scripts' declared in pyproject.toml
(I think?), and being bent to also work like a standalone script runner.
EDIT: I feel like a CLI flag to force a file to be executed would also be acceptable.
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 also have something like... #3097 — maybe we just need a --script
flag here to force us to treat something as a script? We should still have a way to detect recursion and bail instead of looping though.
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.e. a UV_INTERNAL__RECURSION
variable that we bump on every recursive subprocess and bail after some limit is hit)
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.
A --script
flag plus a UV_INTERNAL_RECURSION
for better error reporting would work.
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.
A more resilient solution would be not running binaries, but only running python scripts and python
from uv run
, this seems to be done by e.g. bash
to avoid infinite recursion.
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'm pretty unwilling to change that part of the user experience — running binaries from uv run
is important. I think a separate command or opt-in to that experience via --script
makes the most sense to me.
When we see
#!/usr/bin/env -S uv run"
, treat the input as a python file to avoid infinite recursion.Tested manually.
Fixes #6360