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

Python bindings broken when DESTDIR is set #545

Closed
FFY00 opened this issue Jun 5, 2020 · 11 comments
Closed

Python bindings broken when DESTDIR is set #545

FFY00 opened this issue Jun 5, 2020 · 11 comments
Assignees

Comments

@FFY00
Copy link

FFY00 commented Jun 5, 2020

21023a5 broke destdir installations. It does not find the library because the it hasn't been installed to the filesystem.

This use-case is commonly required by packagers.

cmake ...
make
make DESTDIR="local/folder" install
Traceback (most recent call last):
  File "/build/libiio/src/libiio-0.20/build/bindings/python/setup.py", line 60, in _check_libiio_installed
    raise OSError
OSError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/build/libiio/src/libiio-0.20/build/bindings/python/setup.py", line 89, in <module>
    setup(**config)
  File "/usr/lib/python3.8/site-packages/setuptools/__init__.py", line 161, in setup
    return distutils.core.setup(**attrs)
  File "/usr/lib/python3.8/distutils/core.py", line 148, in setup
    dist.run_commands()
  File "/usr/lib/python3.8/distutils/dist.py", line 966, in run_commands
    self.run_command(cmd)
  File "/usr/lib/python3.8/distutils/dist.py", line 985, in run_command
    cmd_obj.run()
  File "/build/libiio/src/libiio-0.20/build/bindings/python/setup.py", line 43, in run
    self._check_libiio_installed()
  File "/build/libiio/src/libiio-0.20/build/bindings/python/setup.py", line 66, in _check_libiio_installed
    raise Exception(msg)
Exception: The libiio library could not be found.
            libiio needs to be installed first before the python bindings.
            The latest release can be found on GitHub:
            https://github.com/analogdevicesinc/libiio/releases
@tfcollins
Copy link
Contributor

Are you trying to package the bindings? Your cmake command should not enable them.

-Travis

@FFY00
Copy link
Author

FFY00 commented Jun 5, 2020

I maintain the libiio package in archlinux. I explicitly want to enable the bindings, that is the whole point. It previously worked fine, but the newly introduced _check_libiio_installed() function is not prepared to handle this use-case.

I already updated the package and just removed the check, but this should be properly fixed here.

@rgetz
Copy link
Contributor

rgetz commented Jun 5, 2020

Shouldn't this be handled by the cmake?

cmake ../ -DCMAKE_INSTALL_PREFIX=./local/folder -DINSTALL_UDEV_RULE=OFF -DPYTHON_BINDINGS=ON

@FFY00
Copy link
Author

FFY00 commented Jun 5, 2020

CMAKE_INSTALL_PREFIX is different from DESTDIR, see https://cmake.org/cmake/help/latest/variable/CMAKE_INSTALL_PREFIX.html. Using CMAKE_INSTALL_PREFIX to install to local directory will result in bad installations in cases where it gets special handling, see https://cmake.org/cmake/help/v3.4/module/GNUInstallDirs.html#special-cases. It is *really* bad practice to use it like that.

@FFY00
Copy link
Author

FFY00 commented Jun 5, 2020

Unrelated comment: It is usually hard to get every behavior right when using CMake. This is why I generally advocate using a build system like meson. FHS paths and standard env vars/options can be complex and have a lot of nuances, meson will handle this for you while CMake makes you do it yourself for the most part.

@rgetz
Copy link
Contributor

rgetz commented Jun 6, 2020

This is why I generally advocate using a build system like meson.

Until a better one comes along. :) the "best" one that works, and is maintainable by the people on the team :)

Using CMAKE_INSTALL_PREFIX to install to local directory will result in bad installations in cases where it gets special handling,

So - don't do those. They seem pretty easy to avoid.

In our opinion, the c library should be installed by the package manager - and the python bindings by the python package manager (pypi). Most other libraries did it this way when we looked at things...

-Robin

@nullr0ute
Copy link

In our opinion, the c library should be installed by the package manager - and the python bindings by the python package manager (pypi). Most other libraries did it this way when we looked at things...

I'm seeing the same issue in Fedora. A lot of distro users like them to come from the same place. How do you keep the pypi bindings and C library in sync between the distro and pypi? How do distro users find the pypi bindings. In a lot of enterprise cases pypi is explicitly blocked so organisations can control/audit the use of source. There is a real use case to have bindings and library available from the same location.

@FFY00
Copy link
Author

FFY00 commented Jun 6, 2020

Until a better one comes along. :) the "best" one that works, and is maintainable by the people on the team :)

I understand. I usually think it's worth investing some initial extra time to minimize problems in the future.

So - don't do those. They seem pretty easy to avoid.

But to avoid them you must know they exist and that you should avoid them :P
If it was as simple as you put it...

In my experience dealing with build systems, environments, etc. the ones which give less problems are the ones that try to do the right thing for you, the ones that do not assume you have a vast knowledge and try to simplify things. This is my main pet peeve with CMake -- it's too easy to screw up.

In our opinion, the c library should be installed by the package manager - and the python bindings by the python package manager (pypi). Most other libraries did it this way when we looked at things...

While I see where you might come from, I don't think it's correct. While it might work fine for pure python and simple native packages, if you go a bit further it's a mess. Don't believe me? Have a look at anaconda and see for yourself.

In this specific case, what if I have an application that depends on the bindings? If it's pure python, it shouldn't be much of an issue. But let's say it is a Python/native application, and as such can't be packaged by pip. How would you handle those cases? This is a valid and very possible use-case.

On top of this, I can't see any drawbacks of packaging the python bindings in distribution repositories apart from you having to support this use-case.

I think pip is very useful, but for development. You can setup virtual environments and use it for your development, but you shouldn't use it for system package management. End users should not use pip, they should use the distribution package manager.

When you deal with native code you need to worry about binary compatibility, native dependencies, etc. pip is just not designed to do that.

If something breaks when mixing pip and system packages/manual installations, you can fix it. The end user probably won't be able to, and that shouldn't be expected from him.

@rgetz
Copy link
Contributor

rgetz commented Jun 6, 2020

That’s what we were trying to do - help support end users. (Not developers).

We were getting lots of questions from people about installing things on pip, but not installing the actual c library. That’s why we put the check in there....

We can remove the check - but unfixed the problem we were trying to solve.

@FFY00
Copy link
Author

FFY00 commented Jun 6, 2020

You don't need to remove the check, DESTDIR is passed as an environment variable. In setup.py you just need to check if it set, and also search there for the lib (honestly you could just skip the check). I could send a patch but I have some other stuff to deal with at the moment.

Everything would be easier for you if you were able to say to your users to just install the system package. For Arch Linux you can 😄.

@rgetz
Copy link
Contributor

rgetz commented Jun 6, 2020

Yeah, more and more distributions are including libiio in upstream packages (and we do point to those), see the new doc - it's just that everyone is doing it a little differently. :)

Thanks for the feedback - we will have a look, and try to make sure this gets resolved.

-Robin

@FFY00 FFY00 changed the title Python bindings broken when desdir is set Python bindings broken when DESTDIR is set Jun 7, 2020
@rgetz rgetz closed this as completed in 5c86f76 Jun 16, 2020
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

4 participants