classification
Title: distutils.command.install_lib.get_outputs() wrong with extensions built inplace
Type: behavior Stage: patch review
Components: Distutils Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Elvis Stansvik, dstufft, eric.araujo, estan, inada.naoki, koobs, ned.deily
Priority: normal Keywords: patch

Created on 2017-01-10 15:37 by Elvis Stansvik, last changed 2017-07-19 10:22 by estan.

Files
File name Uploaded Description Edit
fix-install-lib-with-inplace-ext.patch Elvis Stansvik, 2017-01-10 15:42 Patch that fixes the issue review
Pull Requests
URL Status Linked Edit
PR 2759 closed estan, 2017-07-19 08:11
Messages (9)
msg285127 - (view) Author: Elvis Stansvik (Elvis Stansvik) Date: 2017-01-10 15:37
I noticed the following strange behavior:

estan@newton:~/testdist$ find
.
./testext.c
./setup.cfg
./setup.py
estan@newton:~/testdist$ cat setup.py 
from distutils.core import setup, Extension

setup(
    name='testdist',
    version='1.0.0',
    ext_modules = [Extension('testext', ['testext.c'])]
)
estan@newton:~/testdist$ cat setup.cfg
[build_ext]
inplace=1
estan@newton:~/testdist$ ~/cpython/python setup.py install --prefix=~/prefix --record record.txt 
running install
running build
running build_ext
building 'testext' extension
creating build
creating build/temp.linux-x86_64-3.7-pydebug
gcc -pthread -Wno-unused-result -Wsign-compare -g -Og -Wall -Wstrict-prototypes -fPIC -I/home/estan/cpython/Include -I/home/estan/cpython -c testext.c -o build/temp.linux-x86_64-3.7-pydebug/testext.o
gcc -pthread -shared build/temp.linux-x86_64-3.7-pydebug/testext.o -o /home/estan/testdist/testext.cpython-37dm-x86_64-linux-gnu.so
running install_lib
warning: install_lib: 'build/lib.linux-x86_64-3.7-pydebug' does not exist -- no Python modules to install

running install_egg_info
Creating /home/estan/prefix/lib/python3.7/site-packages/
Writing /home/estan/prefix/lib/python3.7/site-packages/testdist-1.0.0-py3.7.egg-info
writing list of installed files to 'record.txt'
estan@newton:~/testdist$ cat record.txt 
/home/estan/prefix/lib/python3.7/site-packages/n-37dm-x86_64-linux-gnu.so
/home/estan/prefix/lib/python3.7/site-packages/testdist-1.0.0-py3.7.egg-info
estan@newton:~/testdist$ find ~/prefix
/home/estan/prefix
/home/estan/prefix/lib
/home/estan/prefix/lib/python3.7
/home/estan/prefix/lib/python3.7/site-packages
/home/estan/prefix/lib/python3.7/site-packages/testdist-1.0.0-py3.7.egg-info
estan@newton:~/testdist$

Notice how in the written record.txt, the path to the extension is wrong ("testext.cpytho" is missing), and the extension is not installed.

I did a little digging and found that the reason is install_lib.get_outputs() returns the wrong result for extensions that are built inplace: It assumes they are built in the build directory.

The attached patch fixes the problem. The failure of the included test case without the change to install_lib is:

======================================================================
FAIL: test_get_outputs_inplace_ext (distutils.tests.test_install_lib.InstallLibTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/estan/cpython/Lib/distutils/tests/test_install_lib.py", line 91, in test_get_outputs_inplace_ext
    self.assertEqual(expected, actual)
AssertionError: Lists differ: ['/tmp/tmptt4nhzgi/foo.cpython-37dm-x86_64-linux-gnu.so'] != ['/tmp/tmptt4nhzgi/dm-x86_64-linux-gnu.so']

First differing element 0:
'/tmp/tmptt4nhzgi/foo.cpython-37dm-x86_64-linux-gnu.so'
'/tmp/tmptt4nhzgi/dm-x86_64-linux-gnu.so'

- ['/tmp/tmptt4nhzgi/foo.cpython-37dm-x86_64-linux-gnu.so']
?                    --------------

+ ['/tmp/tmptt4nhzgi/dm-x86_64-linux-gnu.so']

----------------------------------------------------------------------

Notice the missing "foo.cpython-37". With the change to install_lib, the test passes.
msg285139 - (view) Author: Elvis Stansvik (Elvis Stansvik) Date: 2017-01-10 18:06
Not really related to this fix, but a colleague also prudently pointed out that perhaps an

   assert file[:prefix_len] == build_dir + os.sep

before the

   outputs.append(os.path.join(self.install_dir,
    file[prefix_len:]))

in the helper for pure modules (_pure_outputs after my patch is applied) would be in order, to assert that what is cut away is what was intended. 

That sounds like a good idea to me, but could it give false negatives on Windows (case-insensitive file systems? or file systems where both / and \ are allowed?).
msg287826 - (view) Author: Elvis Stansvik (estan) * Date: 2017-02-15 08:20
It's been over a month. It would be great if someone could at least acknowledge the bug. The current behavior is clearly wrong.

I realize changes to distutils is perhaps not the most fun thing to review, but I'd like some kind of fix for this to go in. I have a hard time seeing anyone is relying on this broken behavior or have put in workarounds for it, so provided the fix is good, it should have minimal impact.
msg298643 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2017-07-19 08:19
Now has PR waiting for review.  Thanks, Elvis.
msg298646 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2017-07-19 09:05
I thought `inplace` option is for debugging/testing without install,
not for installing or packaging.

As far as document [1], `inplace=1` in `setup.cfg` is noticed as:

  "which is probably a bad idea for this option, since always building extensions in-place would break installation of the module distribution."

[1] https://docs.python.org/3.6/distutils/configfile.html#writing-the-setup-configuration-file

So I don't know we should fix this or prohibit this.
Is this works for commands other than `install`?  How about sdist and bdist_wheel?
msg298647 - (view) Author: Elvis Stansvik (estan) * Date: 2017-07-19 09:17
@inada.naoki: I was half afraid that note in the docs would be brought up :) But I should have brought it up myself, sorry about that.

My take is that I see no reason why this limitation should exists. For projects that have extensions, and wish to be runnable both directly from the distribution source tree, but also be installable, it would be quite convenient to be able to set inplace=1 in setup.cfg and have it just work (tm). At least, that's how I ran into this issue. Doing so breaks `setup.py install`.

I haven't tested other targets like sdist and bdist_wheel, but I'll do that and report back.
msg298658 - (view) Author: Elvis Stansvik (estan) * Date: 2017-07-19 10:08
Okay, so it seems my "fix" is not complete at all.

Not even `setup.py install` works with plain distutils, since it just copies the build dir. Back when I created this issue, I must have (accidentally) tested `setup.py install` using setuptools. With setuptools it _does_ work when my fix for get_outputs() is applied, but only because (I think) then the egg creation code from setuptools is used, which is somehow helped by my fix to get_outputs(). Note that it doesn't even work with setuptools if --single-version-externally-managed is used (bypassing egg creation). So my "fix" only really helped for a very specific case.

So, hm. I guess a more involved fix is necessary in order to get this to work for all relevant commands of plain distutils.

Sorry for the noise, I guess I'll close my PR.
msg298659 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2017-07-19 10:13
Sorry, I'm not packaging expert.  Please wait for expert for right direction.
msg298666 - (view) Author: Elvis Stansvik (estan) * Date: 2017-07-19 10:22
So feel free to close this issue. The use case was quite marginal anyway, and we can always instruct developers to run `build_ext --inplace` when they intend to use the distribution out of the source directory (and not have inplace=1 in setup.cfg, to allow for installation as well).

If anyone should want to try to remove the limitation wrt installation with inplace extensions, I think they're in for quite a bit of work.
History
Date User Action Args
2017-07-19 10:22:35estansetmessages: + msg298666
2017-07-19 10:13:39inada.naokisetmessages: + msg298659
2017-07-19 10:08:56estansetmessages: + msg298658
2017-07-19 09:17:49estansetmessages: + msg298647
2017-07-19 09:05:01inada.naokisetnosy: + inada.naoki
messages: + msg298646
2017-07-19 08:19:40ned.deilysetnosy: + ned.deily

messages: + msg298643
stage: patch review
2017-07-19 08:11:22estansetpull_requests: + pull_request2818
2017-07-19 05:20:19koobssetnosy: + koobs
2017-02-15 08:20:47estansetnosy: + estan
messages: + msg287826
2017-01-10 18:06:51Elvis Stansviksetmessages: + msg285139
2017-01-10 15:48:36Elvis Stansviksetversions: + Python 3.7
2017-01-10 15:42:17Elvis Stansviksetfiles: + fix-install-lib-with-inplace-ext.patch
keywords: + patch
2017-01-10 15:37:15Elvis Stansvikcreate