dgasmith / gau2grid

Fast computation of a gaussian and its derivative on a grid.
https://gau2grid.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
29 stars 15 forks source link

build failure: pybuild installs during build_extension #38

Closed mbanck closed 5 years ago

mbanck commented 5 years ago

Just updating the Debian packaging to 1.3.1 results in a build/install failure:

[100%] Built target gg
make[3]: Leaving directory '/<<PKGBUILDDIR>>/build/temp.linux-amd64-3.7'
Install the project...
-- Install configuration: "Release"
-- Installing: /usr/lib/python3/dist-packages/gau2grid
CMake Error at cmake_install.cmake:41 (file):
  file INSTALL cannot make directory
  "/usr/lib/python3/dist-packages/gau2grid": Permission denied

make[2]: *** [Makefile:118: install] Error 1
make[2]: Leaving directory '/<<PKGBUILDDIR>>/build/temp.linux-amd64-3.7'
Traceback (most recent call last):
  File "setup.py", line 147, in <module>
    zip_safe=False,
  File "/usr/lib/python3/dist-packages/setuptools/__init__.py", line 143, in setup
    return distutils.core.setup(**attrs)
  File "/usr/lib/python3.7/distutils/core.py", line 148, in setup
    dist.run_commands()
  File "/usr/lib/python3.7/distutils/dist.py", line 966, in run_commands
    self.run_command(cmd)
  File "/usr/lib/python3.7/distutils/dist.py", line 985, in run_command
    cmd_obj.run()
  File "/usr/lib/python3.7/distutils/command/build.py", line 135, in run
    self.run_command(cmd_name)
  File "/usr/lib/python3.7/distutils/cmd.py", line 313, in run_command
    self.distribution.run_command(command)
  File "/usr/lib/python3.7/distutils/dist.py", line 985, in run_command
    cmd_obj.run()
  File "setup.py", line 33, in run
    self.build_extension(ext)
  File "setup.py", line 58, in build_extension
    subprocess.check_call(['cmake', '--build', '.', '--target', 'install'], cwd=self.build_temp)
  File "/usr/lib/python3.7/subprocess.py", line 347, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['cmake', '--build', '.', '--target', 'install']' returned non-zero exit status 2.

I think the root of the problem is that setup.py now runs install in the build_extension stanza, via 6f720c56da:

        subprocess.check_call(['cmake', '--build', '.', '--target', 'install'], cwd=self.build_temp)

During build, $DESTDIR is not setup by packaging infrastructure so it tries to install into the root filesystem, not the staging one.

Not sure how install should work otherwise (I've never seen in handled specifically in setup.py), why was that line added in 1.3.1 and what's the purpose? If I remove it, gau2grid builds/installs just fine for me; this is what gets run (and which is mostly in line with README.md):

/usr/bin/python3 setup.py install --root `pwd`/debian/tmp

/cc @loriab

loriab commented 5 years ago

Sorry about the trouble. The install line in setup.py came about to solve multiple issues. Particularly with adding so.1 to Linux lib it couldn’t be loaded by python. And the setup.py was otherwise fragile as it mixed built lib and source python. The pure cmake build of pygau2grid didn’t work and setup.py just calls cmake. I fixed the cmake, then adjusted setup.py to do the most common setup.py usage. It doesn’t do fancy setup.py stuff. I just use cmake directly. Would that work for you?

mbanck commented 5 years ago

I'm not sure what exactly you are proposing? The project isn't very big so I don't mind having to basically build it twice; if one of them would suffice for everything, that'd be just fine as well.

Right now, I've patched out that install line and I've uploaded 1.3.1, I am happy to update to newer versions that don't need the patch, but it is not bothering a lot for now.

loriab commented 5 years ago

I meant to finish this comment a couple days ago. Basically, the intersection of Linux install conventions and Python install conventions is difficult to navigate and is only flimsily tied together in setup.py. Since setup.py is just trying to provide a nice wrapper for the most common user installation case and calling cmake under the hood, I just skip the wrapper when building gau2grid+pygau2grid. My setup is https://github.com/psi4/psi4meta/blob/master/conda-recipes/gau2grid-multiout/build.sh#L39-L55 and then the packages get split in two by glob https://github.com/psi4/psi4meta/blob/master/conda-recipes/gau2grid-multiout/meta.yaml#L78-L79 . (Also need https://github.com/psi4/psi4meta/blob/master/conda-recipes/conda_build_config.yaml#L77-L80.)

If you've got 1.3.1 working, great. Just suggesting cmake-only is easier to control and reason about.

dgasmith commented 5 years ago

Is this still a valid issue? Any action items here?

mbanck commented 5 years ago

Well the call to cmake --build --target install still needs to be patched out of setup.py for the current gau2grid release to be buildable for distribution packaging. It's not clear to me whether just removing that would break other things (psi4), but in any case running the install target in the build step seems wrong to me.

loriab commented 5 years ago

I recommend ignoring setup.py and interacting directly with cmake. With the GNUInstallDirs and INSTALL_PYMOD=ON and PYMOD_INSTALL_LIBDIR, can get a fully customizable build and install of gau2grid and pygau2grid.

mbanck commented 5 years ago

OK, I'll try that, thanks.

mbanck commented 5 years ago

PYMOD_INSTALL_LIBDIR

That seems to be relative to CMAKE_INSTALL_LIBDIR, but at least on Debian, the proper python path is /usr/lib/python3/dist-packages/ whereas CMAKE_INSTALL_LIBDIR is /usr/lib/x86_64-linux-gnu, which is not part of any of the system python search paths.

NATIVE_PYTHON_INSTALL does the right thing (-- Installing: /<<PKGBUILDDIR>>/debian/tmp/usr/lib/python3/dist-packages/gau2grid/__init__.py ) but then skips the library so I can't use that either.

loriab commented 5 years ago

I see. I hadn't encountered a site-packages that wasn't within CMAKE_INSTALL_LIBDIR so hadn't planned on that. Sol'n pending ...

mbanck commented 5 years ago

If I set both NATIVE_PYTHON_INSTALL and NATIVE_PYTHON_INSTALL_WITH_LIB then everything ends up where I expect it. However, the egg-info stuff isn't being shipped contrary to a setup.py install, which means Debian's python helper cannot figure out the dependencies and numpy is undeclared, leading to runtime errors.

loriab commented 5 years ago

Ok, setup.py should now understand -DNATIVE_PYTHON_INSTALL_WITH_LIB=ON so the egg-info stuff comes along.

mbanck commented 5 years ago

Sorry I am confused, I thought I don't need to run setup.py anymore? That additional patch doesn't help for a cmake-only build.

loriab commented 5 years ago

Sorry, I was unclear. The egg business is something cmake can't provide, so I think you'll have to go back to a setup.py. The existing PR changes should allow building both python package and library in one run (let me know if you need any other cmake variables whitelisted for setup.py). But then there's the install issue that started this thread. The install target does need to run in the build step because that's what creates an un-SO-versioned copy of the library for the python module to load. I'm rather confused that patching out the install gives what you need, but I'll attribute that to debian build infrastructure details. I've added a route -DBYPASS=INSTALL=True that should skip the troublesome step in setup.py. Alternatively, if all you need from the egg is dependency info, the sole dependency is numpy and that's not expected to change, so if there's a way to inject that manually, could stick with the pure cmake.