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

Incorrect shebang generated by Pip when run as part of CPython build and installing to a DESTDIR #171

Closed
sam-thursfield-nutanix opened this issue May 27, 2022 · 14 comments

Comments

@sam-thursfield-nutanix
Copy link

sam-thursfield-nutanix commented May 27, 2022

Describe the bug

Since CPython 3.9.11 / pip 22.0.4 / distlib 0.3.3, the pip3 script generated at CPython install time has an incorrect shebang line.

To Reproduce
Steps to reproduce the behavior:

In a CPython git clone:

  1. mkdir /tmp/destdir
  2. ./configure --prefix=/usr/local --with-ensurepip=yes
  3. make -j
  4. DESTDIR=/tmp/destdir make install
  5. head /tmp/destdir/usr/local/bin/pip3

Expected behavior

The shebang I see with CPython 3.9.11 / pip 22.0.4 / distlib 0.3.3 is: #!/usr/local/bin/python. This file does not exist.

The shebang I see with CPython 3.9.8 / pip 21.2.4 / distlib 0.3.2 is: #!/usr/local/bin/python3. This file will exist, but it does not at the time the CPython install runs.

Environment

  • CentOS 7

Additional information

This issue is a little hard to track down, as it only reproduces during CPython build, where distlib is embedded inside the file Lib/ensurepip/_bundled/pip-22.0.4-py3-none-any.whl.

I suspect that the regression was introduced in this commit: ad0ea22

This adds a line to ScriptMaker._get_shebang() which does the following:

        if not os.path.isfile(executable):
            # for Python builds from source on Windows, no Python executables with
            # a version suffix are created, so we use python.exe
            executable = os.path.join(sysconfig.get_config_var('BINDIR'),
                            'python%s' % (sysconfig.get_config_var('EXE')))

In my scenario, executable = /usr/local/bin/python3. This file does not exist, because we ran DESTDIR=/tmp/destdir make install and so the file has been installed to /tmp/destdir/usr/local/bin/python3. However, this is the path that should be used in the script shebang line, and distlib <= 0.3.2 handled the situation correctly. The change introduced in distlib 0.3.3 incorrectly chooses /usr/local/bin/python instead.

The change is subtle, many OS distributors will create a symlink bin/python3 -> bin/python that hides the issue. In my case we do not have such a symlink, thus we get a pip3 executable that fails to start with an error like:

Failed to execute process '/usr/local/bin/pip3'. Reason:
The file '/usr/local/bin/pip3' specified the interpreter '/usr/local/bin/python', which is not an executable command.
@sam-thursfield-nutanix
Copy link
Author

Ideas to solve:

  • the new if statement has a comment about Windows, so adding a guard "os == windows" would fix the situation on Linux

Workarounds:

  • manually fix the shebang of $DESTDIR//usr/local/bin/pip3 and $DESTDIR/usr/local/bin/pip3.9 post-install

@zeha
Copy link

zeha commented Jul 8, 2022

In pypa/pip#11237 I have come to the same conclusion, i.e. suspect ad0ea22 as introducing this problem.

My repro was with Python 3.8.13 (bundles pip 22.0.4) and Python 3.10.5 (same pip) on Debian 10 and macOS 12.4.

@vsajip
Copy link
Collaborator

vsajip commented Jul 9, 2022

the new if statement has a comment about Windows, so adding a guard "os == windows" would fix the situation on Linux

Not quite, because there is already a guard if not os.path.isfile(executable) - so the change only kicks in (on any platform) if the executable names a non-existent file.

@vsajip
Copy link
Collaborator

vsajip commented Jul 9, 2022

It's kind of a corner case. I think the ad0ea22 change exposed something that previously worked by chance - in this case, if none of the files exist at the time the shebang is computed, there's not enough information to make a determination as to whether a particular executable value is correct. For the "normal" (most common) case, it's reasonable to check for the existence of the executable and use python if it isn't found, and I'm not sure skipping the file existence check would be the correct fix because it wouldn't be right to do that for the common case.

@vsajip
Copy link
Collaborator

vsajip commented Jul 9, 2022

What does the shebang look like on your system if you do for example ./configure --prefix=$HOME/.local --with-ensurepip=yes ?

@zeha
Copy link

zeha commented Jul 9, 2022

Well, the files surely exist at the correct place, if ensurepip would tell distlib where to look. I.e. it would need to pass down the --root argument value.

@zeha
Copy link

zeha commented Jul 9, 2022

It's kind of a corner case. I think the ad0ea22 change exposed something that previously worked by chance - in this case, if none of the files exist at the time the shebang is computed, there's not enough information to make a determination as to whether a particular executable value is correct. [..]

Well, I guess in the end this means distlib should not do any guessing, and sysconfig should just know the interpreter binary name. ...

@zeha
Copy link

zeha commented Jul 9, 2022

What does the shebang look like on your system if you do for example ./configure --prefix=$HOME/.local --with-ensurepip=yes ?

Just as one would expect:

~/x/Python-3.10.5 $ ./configure --prefix=$HOME/.local --with-ensurepip=yes
$ make -j
$ make install DESTDIR=$HOME/x/destdir
...
if test "xupgrade" != "xno"  ; then \
                case upgrade in \
                        upgrade) ensurepip="--upgrade" ;; \
                        install|*) ensurepip="" ;; \
                esac; \
                 ./python.exe -E -m ensurepip \
                        $ensurepip --root=/Users/ch/x/destdir/ ; \
        fi
Looking in links: /var/folders/bc/p5x2fgjn1yg0t62dg2sb1fp40000gp/T/tmptm20n_4w
Processing /private/var/folders/bc/p5x2fgjn1yg0t62dg2sb1fp40000gp/T/tmptm20n_4w/setuptools-58.1.0-py3-none-any.whl
Processing /private/var/folders/bc/p5x2fgjn1yg0t62dg2sb1fp40000gp/T/tmptm20n_4w/pip-22.0.4-py3-none-any.whl
Installing collected packages: setuptools, pip
  WARNING: The scripts pip3 and pip3.10 are installed in '/Users/ch/x/destdir/Users/ch/.local/bin' which is not on PATH.
  Consider adding this directory to PATH or, if you prefer to suppress this warning, use --no-warn-script-location.
Successfully installed pip-22.0.4 setuptools-58.1.0
$ head /Users/ch/x/destdir/Users/ch/.local/bin/pip3
#!/Users/ch/.local/bin/python
$ stat /Users/ch/x/destdir/Users/ch/.local/bin/python
stat: /Users/ch/x/destdir/Users/ch/.local/bin/python: stat: No such file or directory

So if I were to put all these files into a package and install them into /, it would still not work.

@vsajip
Copy link
Collaborator

vsajip commented Jul 10, 2022

Well, the files surely exist at the correct place, if ensurepip would tell distlib where to look. I.e. it would need to pass down the --root argument value.

Indeed. The logic for ad0ea22 only kicks in if no executable is specified (the if self.executable: test at the start):

distlib/distlib/scripts.py

Lines 160 to 177 in ad0ea22

if self.executable:
executable = self.executable
enquote = False # assume this will be taken care of
elif not sysconfig.is_python_build():
executable = get_executable()
elif in_venv(): # pragma: no cover
executable = os.path.join(sysconfig.get_path('scripts'),
'python%s' % sysconfig.get_config_var('EXE'))
else: # pragma: no cover
executable = os.path.join(
sysconfig.get_config_var('BINDIR'),
'python%s%s' % (sysconfig.get_config_var('VERSION'),
sysconfig.get_config_var('EXE')))
if not os.path.isfile(executable):
# for Python builds from source on Windows, no Python executables with
# a version suffix are created, so we use python.exe
executable = os.path.join(sysconfig.get_config_var('BINDIR'),
'python%s' % (sysconfig.get_config_var('EXE')))

Thus I'm not sure this is actually an issue for distlib, since in this case pip knows the --root and can compute the executable from that and provide it to distlib.

@zeha
Copy link

zeha commented Jul 10, 2022

Thus I'm not sure this is actually an issue for distlib, since in this case pip knows the --root and can compute the executable from that and provide it to distlib.

I think both parts need changes. distlib needs a way that ensurepip can tell it about --root. And (ensure)pip needs to pass that down.

Smashing together the executable and --root will not work: the final shebang still needs to be written without --root prepended, only the file existence check needs it!

@vsajip
Copy link
Collaborator

vsajip commented Jul 11, 2022

I'm not suggesting that (ensure)pip pass down --root - the way distlib works currently is that if you specify the executable path to use, it'll use that. So I'm suggesting that if the ScriptMaker's executable attribute is set to whatever the correct executable path is, then that's all that's needed to put that in the shebang. Any reason that can't be done by the ensurepip machinery?

@zeha
Copy link

zeha commented Jul 11, 2022

I'm not suggesting that (ensure)pip pass down --root - the way distlib works currently is that if you specify the executable path to use, it'll use that.

Ah, understood.

So I'm suggesting that if the ScriptMaker's executable attribute is set to whatever the correct executable path is, then that's all that's needed to put that in the shebang. Any reason that can't be done by the ensurepip machinery?

Well, how does ensurepip know what to put in there?

@vsajip
Copy link
Collaborator

vsajip commented Jul 11, 2022

Well, whatever the --root is passed to can do the computation and set the executable path in the ScriptMaker instance, right?

@vsajip
Copy link
Collaborator

vsajip commented Aug 24, 2022

It seems like this should be dealt with by ensurepip, as only it has the relevant information available as to the target directory. So I'll close this, but feel free to reopen with more information.

@vsajip vsajip closed this as not planned Won't fix, can't repro, duplicate, stale Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants