raisimTech / raisimLib

Visit www.raisim.com
http://www.raisim.com
Other
341 stars 91 forks source link

updating pybind11, fixing raisimpy installation #467

Closed jhwangbo closed 1 year ago

jhwangbo commented 1 year ago

@wxmerkt can you test this one then? This one will install raisimpy in the current python's site_package or dist-packages directory

wxmerkt commented 1 year ago

I am still getting the same error since PYTHON_SITE is the global path, not relative the CMAKE_INSTALL_PREFIX or another relocatable variable

jhwangbo commented 1 year ago

did you delete the build folder and run the cmake again? This line python -c "import sysconfig; print(sysconfig.get_paths()['purelib'])" should output the current python's site-packages folder.

wxmerkt commented 1 year ago

I did - it returns /usr/lib/python3.8/site-packages which is also a path that isn't used (all system-installed packages on Ubuntu are directly under /usr/lib/python3.8) - in any case, it shouldn't default to the global installation path

jhwangbo commented 1 year ago

Is /usr/lib/python3.8/site-packages not used? I thought python searches there by default. We need better path than CMAKE_PREFIX_PATH because that path is for C++ packages. C++ and Python packages should be installed separately. Global installation path is not bad because that is the default path for CMake. It will warn the user first if he didn't intend to do so

wxmerkt commented 1 year ago

A package shouldnt automatically install to /usr/lib. The sysconfig.get_paths() are all global paths but not used for installation. The distutils as before can provide paths for package installation.

What the previous solution did is to install Python to the platform-specific relative path, e.g. lib/python3.10/site-packages or lib/python3/dist-packages (Debian etc.) - added on top and relative to the install prefix the CMake uses (which can be overridden with CMAKE_INSTALL_PREFIX).

If you have some cases where the old solution didn't work or returned bad paths, I might be able to take a look

jhwangbo commented 1 year ago

I only have one point. CMAKE_INSTALL_PREFIX is for C++ packages and shouldn't be used for python packages. They should be installed in a separate directories.

wxmerkt commented 1 year ago

If installed via CMake, it's common practice to configure relative to it using the platform-specific Python paths. The current solution tries to install to an absolute path ignoring a specified prefix/installation location. I commonly follow this advice from Matthew who is with Kitware/CMake - https://stackoverflow.com/a/40006251. Starting CMake 3.12, Python also provides these paths directly as variables: https://cmake.org/cmake/help/v3.15/module/FindPython.html#result-variables

I haven't used Windows for Python development recently (where the previous solution broke) - if you can describe how it broke / how things are usually configured, I'd be happy to think about an approach

jhwangbo commented 1 year ago

So do you mean that we have to install C++ and python packages in the same directory? or do you mean that we have to run cmake install separately?

wxmerkt commented 1 year ago

The install would run at the same time (if they are configured and built with CMake). If the Python bit is e.g. built by setuptools/setup.py this would be different. They would be installed to the correct repositories for each type of code, e.g.

jhwangbo commented 1 year ago

I am not asking the convention. I am talking about the usage. I tried to rephrase my question multiple times but probably it is still not clear to you. Following the convention you mentioned, we will install both the C++ and python packages relative to $CMAKE_PREFIX_PATH. But when using them, we want them to be in different directories. For python packages, we want them to be in the python's site-package folder, not in $CMAKE_PREFIX_PATH/${PYTHON-SITE}. So how would one use this convention when using raisimpy? Do we have to add $CMAKE_PREFIX_PATH to the $PYTHON_PATH?

jhwangbo commented 1 year ago

To be more clear, I need an install instruction.