-
Notifications
You must be signed in to change notification settings - Fork 54
Fix bug when writing out scripts if the python interpreter has a space #276
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
base: main
Are you sure you want to change the base?
Conversation
In SchemeDirectoryDestination, we write: - files, which may be part of the wheel "scripts" scheme and if so must have #!python replaced by the interpreter shebang - entrypoints, which are built from scratch with the interpreter shebang Only in the latter case did we ensure that we build a portable shebang. Partially reverts commit 0edcaa7, which refactored build_shebang into a class.
|
This is not strictly a followup on #200 but it is something I happened to notice as an aside while I was working on that. Apologies for the lateness. |
| if forlauncher: # The launcher can just use the command as-is. | ||
| return b"#!" + executable_bytes | ||
| if _is_executable_simple(executable_bytes): | ||
| return b"#!" + executable_bytes |
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 have just noticed, that for both those cases we return exactly the same value. Can you make it into a single if statement with an or?
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 have a followup commit that does this. It's not really appropriate when moving a block of code from one location to another, to also start changing the interior design of that block.
| cast, | ||
| ) | ||
|
|
||
| from installer.scripts import _build_shebang |
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.
Not sure that I like importing and using a "private" function. I think a better solution would be to extract the part of the logic that is required for this use case into another function and having the _build_shebang use that. That should also allow _build_shebang to remain a part of the class, as it was before.
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.
The part of the logic that is required for this use case is "all of it".
Would it make you feel more comfortable if we remove the underscore and declare it a public function?
In SchemeDirectoryDestination, we write:
files, which may be part of the wheel "scripts" scheme and if so must have #!python replaced by the interpreter shebang
entrypoints, which are built from scratch with the interpreter shebang
Only in the latter case did we ensure that we build a portable shebang.
Partially reverts commit 0edcaa7, which refactored build_shebang into a class.