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.

Sign up for GitHub

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 relocatable shebang with space-in-path robustness #395

Merged
merged 3 commits into from
Dec 3, 2024
+2 −1
Merged

Use relocatable shebang with space-in-path robustness #395

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fdf1d82
Use relocatable shebang with space-in-path robustness
charliermarsh Nov 24, 2024
e2458be
Re-escape
charliermarsh Nov 25, 2024
edbb772
Try again
charliermarsh Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion cpython-unix/build-cpython.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,7 +858,8 @@ def fix_shebang(full):

lines.extend([
b"#!/bin/sh\n",
b'"exec" "\$(dirname \$0)/python${PYTHON_MAJMIN_VERSION}${PYTHON_BINARY_SUFFIX}" "\$0" "\$@"\n',
b"'''exec' \"$(dirname -- \"$(realpath -- \"$0\")\")\"/'python${PYTHON_MAJMIN_VERSION}${PYTHON_BINARY_SUFFIX}' \"$0\" \"$@\"\n",
Copy link

@paveldikov paveldikov Nov 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Are you sure ${PYTHON_MAJMIN_VERSION}${PYTHON_BINARY_SUFFIX} will be interpolated within the single quotes?

(I don't have good knowledge of the control flow in here, so genuinely unsure. If the interpolation is indeed broken, you might want to use implicit concatenation e.g. 'foo'"$BAR". I.e. just flip the pythonXY bit to double-quoted.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh good point. Mentally I assumed this was an f-string because in the written-out shebang, it seems to use the expanded versions (but clearly it's not). I'll review, modify, and test prior to merging.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, so the variables did expand, but the rest of the expressions were also evaluated...

#!/bin/sh
'''exec' "."/'python3.9d' "/var/folders/p1/44pzfl0j2m301zzfb0fv0p380000gn/T/tmppqyxz114/build-cpython.sh" ""
' '''

Copy link
Member Author

Choose a reason for hiding this comment

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

(Still not doing quite the right thing, will revisit soon.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Success!

#!/bin/sh
'''exec' "$(dirname -- "$(realpath -- "$0")")/python3.11d" "$0" "$@"
' '''

import pydoc
if __name__ == '__main__':
    pydoc.cli()

Copy link

Choose a reason for hiding this comment

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

Wouldn't it make more sense to have this complicated launcher only if the Python path contains a space?

Because the Python startup time is already slow enough, I don't think having two extra processes (per startup) (dirname and realpath) would help much. (Also it would break if one intends to use this in a minimal container / environment, where the coreutils or equivalent aren't present.)

Now I don't exactly know if this launcher should be relocatable, but if so, I think there can be a way to detect in pure POSIX sh if the $0 contains a space, and if so do this, else just leaving it alone.

Also, according to the POSIX specification https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02 one could use something like the following (not tested, but could be worked out):

#!/bin/sh
''':' ## this is an alias for `true`
_path="${0}"
_path="${_path%/*}" ## remove the smallest suffix like `/*`
exec -- "${_path}/python3.11d" "${0}" "${@}"
' '''

This, should work, and should remove the need for any other additional processes, except the /bin/sh.

Copy link
Member Author

Choose a reason for hiding this comment

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

I defer to @paveldikov on the shebang itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note however that we can't know if the path will contain a space, since the whole motivation here is that the path is relocatable. The user could put it anywhere!

Copy link

Choose a reason for hiding this comment

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

[...] we can't know if the path will contain a space [...]

One could use the following simple check (that I believe is POSIX compliant):

## replace all spaces and check if the value differs from the original
_path="${0}"
if test "${_path}" == "${_path// }" ; then
    ## the path does not contain a space
else
    ## the path contains a space
fi

However, the other snippet I've given should work, (and after some extra thought about it) perhaps an extra check should be done to see if the path did contain a / or not, as in:

#!/bin/sh
''':'
_path_0="${0}"
_path="${_path_0%/*}"
if test "${_path_0}" == "${_path}" ; then
    ## there was no `/` fould in `_path_0`
    _path='.'
fi
exec -- "${_path}/python3.11d" "${0}" "${@}"
' '''

Copy link
Member

@zanieb zanieb Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Happy to track this as a performance improvement

#397

b"' '''\n",
])

lines.extend(fh)
Expand Down