pip installs headers to incorrect location when using '--root'

[lkollar]

Environment

• pip version: 20.1.1
• Python version: Python 3.8.3
• OS: Debian 10.4

Description
Installing an extension from source which ships headers results in installing the headers in the wrong destination. It appears that the value of --install-headers is appended to --root.

Expected behavior

The headers should be installed into the includes directory under the prefix location (--root).

How to Reproduce

python3.8 -m pip install --no-deps -vvv  --no-binary :all: --root /tmp/greenlet greenlet==0.4.16

   Running command /usr/local/bin/python3.8 -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/tmp/pip-install-hpyyby3f/greenlet/setup.py'"'"'; __file__='"'"'/tmp/pip-install-hpyyby3f/greenlet/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' install --record /tmp/pip-record-1x66znm7/install-record.txt --single-version-externally-managed --root /tmp/greenlet --compile --install-headers /tmp/greenlet/usr/local/include/python3.8/greenlet
    running install
    running build
    running build_ext
    building 'greenlet' extension
    creating build
    creating build/temp.linux-x86_64-3.8
    gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -I/usr/local/include/python3.8 -c greenlet.c -o build/temp.linux-x86_64-3.8/greenlet.o
    creating build/lib.linux-x86_64-3.8
    gcc -pthread -shared build/temp.linux-x86_64-3.8/greenlet.o -L/usr/local/lib -o build/lib.linux-x86_64-3.8/greenlet.cpython-38-x86_64-linux-gnu.so
    running install_lib
    creating /tmp/greenlet
    creating /tmp/greenlet/usr
    creating /tmp/greenlet/usr/local
    creating /tmp/greenlet/usr/local/lib
    creating /tmp/greenlet/usr/local/lib/python3.8
    creating /tmp/greenlet/usr/local/lib/python3.8/site-packages
    copying build/lib.linux-x86_64-3.8/greenlet.cpython-38-x86_64-linux-gnu.so -> /tmp/greenlet/usr/local/lib/python3.8/site-packages
    running install_headers
    creating /tmp/greenlet/tmp
    creating /tmp/greenlet/tmp/greenlet
    creating /tmp/greenlet/tmp/greenlet/usr
    creating /tmp/greenlet/tmp/greenlet/usr/local
    creating /tmp/greenlet/tmp/greenlet/usr/local/include
    creating /tmp/greenlet/tmp/greenlet/usr/local/include/python3.8
    creating /tmp/greenlet/tmp/greenlet/usr/local/include/python3.8/greenlet
    copying greenlet.h -> /tmp/greenlet/tmp/greenlet/usr/local/include/python3.8/greenlet

As seen above the greenlet.h file is installed into /tmp/greenlet/tmp/greenlet/usr/local/include/python3.8/greenlet. It seems that the value of --install-headers is appended to the value of --root.

To get the correct behaviour one needs to modify the setuptools arguments and remove the prefix from --install-headers:

python3.8 ./setup.py install --record /tmp/pip-record-7__qmu5v/install-record.txt  --root /tmp/greenlet --compile --install-headers /usr/local/include/python3.8/greenlet

I wonder if this was introduced by the Scheme class. Judging from 6fa64a6 this used to be only set under virtualenv (and then it was set without the prefix directory). I checked and removing the --install-headers in the setuptools call works correctly in conjunction with --root and the header is installed into the prefix directory.

[lkollar]

I managed to git bisect this to the 6fa64a6 commit.

The test I used for bisecting simply checks that the tmp directory does not exist in the specified --root location. The incorrect behaviour is that the headers get installed under <root>/tmp. I ran this in a Docker container as the behaviour is different when running inside a virtualenv.

#!/bin/bash

rm -rf /tmp/greenlet
python3.8 -m pip install --no-deps -vvv  --no-binary :all: --root /tmp/greenlet greenlet==0.4.16 &>/dev/null

[ -e /tmp/greenlet/tmp ] && exit 1

exit 0

The change in the above mentioned commit results in header_dir to be always set, even though setuptools will append this value to the specified --root location. By setting header_dir to None the headers get installed correctly.

I was trying to check if there are any existing tests for this and found test_install_package_with_root, but because this runs in a venv, the behaviour is different as scheme.headers is handled differently under a virtual environment.

I don't know what the correct fix is. Resurrecting the old logic would probably work, but the I suspect that distutils_scheme should handle the logic to correctly figure out scheme.headers.

[uranusjr]

Probably related to #8521?

[chrahunt]

An alternative approach is that we can explicitly pass all of the --install-* options to setup.py, and omit --root. This is possible now because #7309 has reached the end of the deprecation period and we can assume that --install-option and --global-option do not contain any install-location-related arguments.

[chrahunt]

I just filed #8556. Once that's in we can unravel the logic around passing location-related arguments to setuptools and fix this bug. I think a proper automated test will be harder, since we don't currently have properly isolated tests. The comments in #7813 describes one possible approach for that.

[lkollar]

Now that #8556 has been landed, I took another look to see how this could be fixed. The issue is that the root parameter is considered together with the header_dir value from the Schema object, which has already prefixed the header path with the value of root, so pip ends up passing --root <prefix> --install-headers <prefix>/<header path> which results in the duplicate creation of the prefix.

pip/src/pip/_internal/utils/setuptools_build.py

Lines 162 to 177 in a13f201

	if root is not None:
	args += ["--root", root]
	if prefix is not None:
	args += ["--prefix", prefix]
	if home is not None:
	args += ["--home", home]
	if use_user_site:
	args += ["--user", "--prefix="]
	if pycompile:
	args += ["--compile"]
	else:
	args += ["--no-compile"]
	if header_dir:
	args += ["--install-headers", header_dir]

Either the Schema class should not take prefix directories into account, or the call into setuptools should check if the root is already a prefix of the header directory, but this seems hacky.

There might be much better alternatives, but I'm not that familiar with the pip codebase.

[lkollar]

I made a draft PR following @chrahunt's suggestion to pass the -install-* arguments separately. I mainly want to see if the existing tests will pass and get some feedback if this is the right direction. If this looks good, I can work on adding tests for it.

[lkollar]

The PR is ready for review. I've added a regression test as well.

[lkollar]

We have been using #8881 for a while and ran into an issue with it: the generated installed-files.txt ends up containing paths including the prefix. This makes sense as with the patch we are only passing --install-<scheme> arguments, so from setuptool's point of view there is no prefix it should ignore when generating the record file. Adding --root as well does not work as it takes precedence over the --install-<scheme> arguments.

Example installed-files.txt snippet from the siphash package with the patch:

../../../../../../../../tmp/siphash/Users/lkisskollar/.virtualenvs/pip/lib/python3.8/site-packages/siphash-0.0.1-py3.8.egg-info/PKG-INFO
../../../../../../../../tmp/siphash/Users/lkisskollar/.virtualenvs/pip/lib/python3.8/site-packages/siphash-0.0.1-py3.8.egg-info/SOURCES.txt
../../../../../../../../tmp/siphash/Users/lkisskollar/.virtualenvs/pip/lib/python3.8/site-packages/siphash-0.0.1-py3.8.egg-info/dependency_links.txt

The correct contents would be (pip 19.2.3 generates this):

../siphash/__init__.py
../siphash/__pycache__/__init__.cpython-38.pyc
PKG-INFO
SOURCES.txt
dependency_links.txt

@pradyunsg suggested using --prefix instead of --root and this works as expected and generates the correct installed-files.txt. Given that --root does not install headers correctly, it might be best to deprecate it in favour of --prefix, if it can do everything --root can.