-
-
Notifications
You must be signed in to change notification settings - Fork 139
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 simple shebang launcher when the path is not complex #397
Comments
Just for reference, my last proposal that uses only POSIX compliant
|
I'm very sympathetic to the startup time and sh only dependency arguments. I've seen that ''' trick before and it is very effective. |
@zanieb is the state of the main branch with the initial shebang change safe to release? Or should we revert to get back to a known good state? |
I think it's worth releasing the initial change regardless, it's a bug fix which I think takes precedence over the performance optimization here especially without benchmarks. |
Here are some figures:
Where:
Conclusions:
For completeness, here are the files:
|
I was more concerned with the portability concerns that were raised. But I guess we relied on dirname before and introduced realpath. So the dependency on non shell builtins was already there. This feels safe to ship. |
The Thanks for doing some benches @cipriancraciun. I feel like 2ms is very small when we're talking about invoking a Python application, but I agree it's worth changing regardless — it doesn't feel too costly to maintain. I'm not sure of other possible downsides? |
There is a potential issue: if the This could be patched by checking if However, I hope others think at other conrer-cases before deploying it. Another possible corner-case, although less-likely, is when the Python executable is installed directly under The new prologue should thus be (not tested):
It remains to be debated if Python is invoked with |
Python's startup time is horrible and it inflicts real pain on the ecosystem. However in this case we're talking about supplementary tools like I want to say the startup time issue is not a consideration here because all the tools we're talking about are rarely executed or startup time is a trivial part of their total time. |
Posted by @cipriancraciun in #395 (comment)
The text was updated successfully, but these errors were encountered: