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

The python binding doesn't install itself #34

Closed
yurivict opened this issue Jan 30, 2022 · 16 comments
Closed

The python binding doesn't install itself #34

yurivict opened this issue Jan 30, 2022 · 16 comments
Labels
cmake help wanted Extra attention is needed python

Comments

@yurivict
Copy link

===>   Generating temporary packing list
ninja: error: unknown target 'install/strip'
*** Error code 1
@wassimj
Copy link
Owner

wassimj commented Jan 31, 2022

Sorry I need more information. Is this on Linux?

@yurivict
Copy link
Author

This is on FreeBSD.

@wassimj
Copy link
Owner

wassimj commented Feb 1, 2022

@brunopostle Are you able to help with this issue? Many thanks!

@brunopostle
Copy link
Contributor

Yes, basically there is a minimal CMake build configuration that allows you to compile the Topologic C++ library and the pybind11 Python bindings as two separate steps. There is an install target for the library, but no integration with python packaging tools, so you have to install the bindings by copying the .so file into your PYTHONPATH.

This isn't ideal, but the project really needs somebody who understands cmake and python packaging to step up and fix it. The good news is that it isn't a particularly complex problem: the library is a pretty normal C++ shared library, and the python bindings are not doing anything weird either.

@wassimj I suggest adding a couple of 'labels' to this issue, in particular you should add: 'help wanted', 'cmake' and 'python'.

@wassimj wassimj added help wanted Extra attention is needed cmake python labels Feb 1, 2022
@sanzoghenzo
Copy link

Hi there,

I can try to look at this; I'm always scared when I have to build C/C++ code, but I have some experience in packaging python libraries.

Just for reference: the pybind11 documentation points to an example using cmake; alternatively one can go with setuptools without the need to have pybind11 as submodule.

Being a python guy I'd prefer to use setuptools (it seems to be the recommended way), is it ok with you?

@wassimj
Copy link
Owner

wassimj commented May 30, 2022

Hi @sanzoghenzo Sure, thanks for your help.

@sanzoghenzo
Copy link

Hi there,
I'm sorry but my missing C++/CMake knowledge is a big obstacle in helping you with this.

What I got so far:

pyproject.toml

[build-system]
requires = [
    "setuptools>=42",
    "wheel",
    "pybind11>=2.8.0",
]

build-backend = "setuptools.build_meta"

[tool.cibuildwheel]
test-command = "python {project}/Python-Bindings/tests/topologictest01.py"
test-skip = "*universal2:arm64"

setup.py

from glob import glob
from setuptools import setup

from pybind11.setup_helpers import Pybind11Extension, build_ext

__version__ = "0.0.1"

ext_modules = [
    Pybind11Extension(
        "topologic",
        sorted(glob("src/*.cpp")),
        include_dirs=[
            "include", 
            "../TopologicCore/include", 
            "/usr/include/opencascade", 
            "/usr/include/uuid"
        ],
        language='c++',
    ),
]

setup(
    name="topologic",
    version=__version__,
    author="Cardiff University and University College London",
    author_email="",
    url="https://github.com/wassimj/Topologic",
    description="Python bindings of Topologic",
    long_description="",
    ext_modules=ext_modules,
    cmdclass={"build_ext": build_ext},
    zip_safe=False,
    python_requires=">=3.6",
)

WIth this configuration the include/pybind11 directory can be deleted, but I have 2 problems:

1 - the opencascade and uuid include paths are hardcoded, I didn't find any mentions in pybind11's documentation on how to make them auto-discoverable
2 - If I run pip install -e . the build process stops at

src/Attribute.cppwg.cpp: In member function ‘virtual void* Attribute_Overloads::Value()’:
    /tmp/pip-build-env-lag9ihzd/overlay/lib/python3.10/site-packages/pybind11/include/pybind11/pybind11.h:2760:57: error: void value not ignored as it ought to be
     2760 |             return pybind11::detail::cast_safe<ret_type>(std::move(o));                           \
          |                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
    /tmp/pip-build-env-lag9ihzd/overlay/lib/python3.10/site-packages/pybind11/include/pybind11/pybind11.h:2794:9: note: in expansion of macro ‘PYBIND11_OVERRIDE_IMPL’
     2794 |         PYBIND11_OVERRIDE_IMPL(PYBIND11_TYPE(ret_type), PYBIND11_TYPE(cname), name, __VA_ARGS__); \
          |         ^~~~~~~~~~~~~~~~~~~~~~
    /tmp/pip-build-env-lag9ihzd/overlay/lib/python3.10/site-packages/pybind11/include/pybind11/pybind11.h:2832:5: note: in expansion of macro ‘PYBIND11_OVERRIDE_PURE_NAME’
     2832 |     PYBIND11_OVERRIDE_PURE_NAME(                                                                  \
          |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
    /tmp/pip-build-env-lag9ihzd/overlay/lib/python3.10/site-packages/pybind11/include/pybind11/pybind11.h:2858:5: note: in expansion of macro ‘PYBIND11_OVERRIDE_PURE’
     2858 |     PYBIND11_OVERRIDE_PURE(PYBIND11_TYPE(ret_type), PYBIND11_TYPE(cname), fn, __VA_ARGS__);
          |     ^~~~~~~~~~~~~~~~~~~~~~
    src/Attribute.cppwg.cpp:16:9: note: in expansion of macro ‘PYBIND11_OVERLOAD_PURE’
       16 |         PYBIND11_OVERLOAD_PURE(
          |         ^~~~~~~~~~~~~~~~~~~~~~
    error: command '/usr/bin/gcc' failed with exit code 1
    [end of output]

For this I didn't find a solution using my google-fu, and I'm sure that there will be many more problems after this... I already spent too much time and I have to give up on this!

@brunopostle
Copy link
Contributor

I see this last error on the fedora 37 build host, but not on fedora 36.

They both have GCC 12.1.1, but the difference seems to be an upgrade from pybind11 2.9.2 to pybind11 2.10.0

@sanzoghenzo what version of pybind11 do you have?

/src/Attribute.cppwg.cpp:16:9: error: void value not ignored as it ought to be
   16 |         PYBIND11_OVERLOAD_PURE(
      |         ^~~~~~~~~~~~~~~~~~~~~~

@sanzoghenzo
Copy link

@brunopostle great catch, using pybind11==2.9.2 in the pyproject.toml solves this particular issue...

But, as I suspected, another one came up...

    In file included from ../TopologicCore/include/Wire.h:19,
                     from include/wrapper_header_collection.hpp:9:
    ../TopologicCore/include/Topology.h: In instantiation of ‘void TopologicCore::Topology::Navigate(const Ptr&, std::__cxx11::list<std::shared_ptr<_Tp> >&) const [with Subclass = TopologicCore::Topology; Ptr = std::shared_ptr<TopologicCore::Topology>]’:
    src/Topology.cppwg-Modified.cpp:472:29:   required from here
    ../TopologicCore/include/Topology.h:814:35: error: ‘Type’ is not a member of ‘TopologicCore::Topology’
      814 |                 if (Subclass::Type() > GetType())
          |                     ~~~~~~~~~~~~~~^~
    ../TopologicCore/include/Topology.h:825:40: error: ‘Type’ is not a member of ‘TopologicCore::Topology’
      825 |                 else if (Subclass::Type() < GetType())
          |                          ~~~~~~~~~~~~~~^~
    error: command '/usr/bin/gcc' failed with exit code 1

I now realize that going the setuptools way is not a good strategy because it completely ignores the CMake files.
The "best" way should be using the scikit-build example, as suggested by the cmake example I linked above.
I'll try this one before giving up 😉

@brunopostle
Copy link
Contributor

@brunopostle great catch, using pybind11==2.9.2 in the pyproject.toml solves this particular issue...

This is probably still a bug in Topologic, i.e. it needs to be fixed to work with pybind11 2.10.0

@wassimj
Copy link
Owner

wassimj commented Aug 9, 2022

I may be wrong, but this kind of bug ('Type' is not a member of 'TopologicCore::Topology') would have failed with gcc regardless of pybind. The fact that Topologic builds the C++ code successfully tells me there is something else going on here. Now it could be that the cppwg files are buggy. I will have to look deeper into that.

@sanzoghenzo
Copy link

switching to scikit-build honors the CMakeLists.txt and builds the package!

Updated setup.py:

import sys

try:
    from skbuild import setup
except ImportError:
    print(
        "Please update pip, you need pip 10 or greater,\n"
        " or you need to install the PEP 518 requirements in pyproject.toml yourself",
        file=sys.stderr,
    )
    raise

from setuptools import find_packages


__version__ = "0.6.0"

setup(
    name="topologic",
    version=__version__,
    author="Cardiff University and University College London",
    author_email="",
    url="https://github.com/wassimj/Topologic",
    description="Python bindings of Topologic",
    long_description="",
    packages=find_packages(where="src"),
    package_dir={"": "src"},
    cmake_install_dir="src/topologic",
    include_package_data=True,
    extras_require={"test": ["pytest"]},
    python_requires=">=3.6",
)

updated pyproject.toml

[build-system]
requires = [
    "setuptools>=42",
    "pybind11~=2.9.2",
    "cmake>=3.22",
    "scikit-build>=0.15.0",
    "ninja; platform_system!='Windows'"
]
build-backend = "setuptools.build_meta"

[tool.cibuildwheel]
test-command = "pytest {project}/tests"
test-extras = ["test"]
test-skip = ["*universal2:arm64"]

Updated CMakeList.txt:

cmake_minimum_required(VERSION 3.15)
project(topologic)

if(SKBUILD)
  # Scikit-Build does not add your site-packages to the search path
  # automatically, so we need to add it _or_ the pybind11 specific directory
  # here.
  execute_process(
    COMMAND "${PYTHON_EXECUTABLE}" -c
            "import pybind11; print(pybind11.get_cmake_dir())"
    OUTPUT_VARIABLE _tmp_dir
    OUTPUT_STRIP_TRAILING_WHITESPACE COMMAND_ECHO STDOUT)
  list(APPEND CMAKE_PREFIX_PATH "${_tmp_dir}")
endif()

# Now we can find pybind11
find_package(pybind11 CONFIG REQUIRED)

set(CMAKE_CXX_STANDARD 17)
IF(MSVC)  
  find_package(PythonLibs REQUIRED)
ENDIF(MSVC)

include_directories(${PYTHON_INCLUDE_DIRS})
IF(MSVC)
  include_directories(../TopologicCore/include)
include_directories(../TopologicCore/include)
ELSE(MSVC)
  include_directories(/usr/include/TopologicCore)
  include_directories(/usr/include/opencascade)
  include_directories(/usr/local/include/TopologicCore)
  include_directories(/usr/local/include/opencascade)
ENDIF(MSVC)

include_directories(./include)

# This is a hardcoded path as we expect OpenCASCADE to be exactly
# in this directory. Revisit if this changes!
IF(MSVC)
  include_directories(C:/OpenCASCADE-7.4.0-vc14-64/opencascade-7.4.0/inc)
  SET(CUSTOM_LIB_PATH "C:/OpenCASCADE-7.4.0-vc14-64/opencascade-7.4.0/win64/vc14/lib")
ENDIF(MSVC)

add_library(TopologicCore SHARED IMPORTED)
set(ADDITIONAL_LIBRARY_DEPENDENCIES
    "TKernel;TKMath"
)

IF(MSVC)
  SET_PROPERTY(TARGET TopologicCore PROPERTY IMPORTED_LOCATION ${CMAKE_CURRENT_LIST_DIR}/../output/${CMAKE_GENERATOR_PLATFORM}/${CMAKE_BUILD_TYPE}/TopologicCore.dll)
  SET_PROPERTY(TARGET TopologicCore PROPERTY IMPORTED_IMPLIB ${CMAKE_CURRENT_LIST_DIR}/../output/${CMAKE_GENERATOR_PLATFORM}/${CMAKE_BUILD_TYPE}/TopologicCore.lib)
else(MSVC)
  find_library(TOPOLOGIC_CORE TopologicCore)
  SET_PROPERTY(TARGET TopologicCore PROPERTY IMPORTED_LOCATION ${TOPOLOGIC_CORE})
ENDIF(MSVC)

set(SRC_FILES 
  ./src/TopologicalQuery.cppwg.cpp
  ./src/Topology.cppwg.cpp
  ./src/Vertex.cppwg.cpp
  ./src/Edge.cppwg.cpp
  ./src/Wire.cppwg.cpp
  ./src/Face.cppwg.cpp
  ./src/Shell.cppwg.cpp
  ./src/Cell.cppwg.cpp
  ./src/CellComplex.cppwg.cpp
  ./src/Cluster.cppwg.cpp
  ./src/Aperture.cppwg.cpp
  ./src/Graph.cppwg.cpp
  ./src/Attribute.cppwg.cpp
  ./src/Dictionary.cppwg.cpp
  ./src/ContentManager.cppwg.cpp
  ./src/Context.cppwg.cpp
  ./src/IntAttribute.cppwg.cpp
  ./src/StringAttribute.cppwg.cpp
  ./src/DoubleAttribute.cppwg.cpp
  ./src/ListAttribute.cppwg.cpp
  ./src/VertexUtility.Binding.cpp
  ./src/EdgeUtility.Binding.cpp
  ./src/WireUtility.Binding.cpp
  ./src/ShellUtility.Binding.cpp
  ./src/CellUtility.Binding.cpp
  ./src/TopologyUtility.Binding.cpp
  ./src/FaceUtility.Binding.cpp
  ./src/GlobalCluster.Binding.cpp
  ./src/main.cpp
)

set(python_module_name _${PROJECT_NAME})
pybind11_add_module(${python_module_name} MODULE ${SRC_FILES})
target_link_directories(${python_module_name} PUBLIC ${CUSTOM_LIB_PATH})
target_link_libraries(${python_module_name} PRIVATE TopologicCore ${ADDITIONAL_LIBRARY_DEPENDENCIES})
target_compile_definitions(${python_module_name} PRIVATE VERSION_INFO=${PROJECT_VERSION})
install(TARGETS ${python_module_name} DESTINATION .)

then I added a src/topologic/__init__.py with

from ._topologic import main

if I pip install -e . it succesfully build and install the package, but if I try to use it I got this error

(.venv) [sanzo@fedora Python-Bindings]$ python
Python 3.10.6 (main, Aug  2 2022, 00:00:00) [GCC 12.1.1 20220507 (Red Hat 12.1.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import topologic
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/sanzo/dev/Topologic/Python-Bindings/src/topologic/__init__.py", line 1, in <module>
    from ._topologic import main
ImportError: libTopologicCore.so.0: cannot open shared object file: No such file or directory

And here I remembered why I "hate" C/C++ 🤣
This is where I really give up.

@brunopostle
Copy link
Contributor

brunopostle commented Aug 9, 2022 via email

@sanzoghenzo
Copy link

yeh, well, the whole point of having a proper python package, for me, was to avoid fiddling with paths and the like...

As I said, I'm no expert at all at C++/CMake, so I'll leave it to you, if you think this is worth the effort.

@brunopostle
Copy link
Contributor

brunopostle commented Aug 9, 2022 via email

@wassimj
Copy link
Owner

wassimj commented Feb 16, 2023

This has now all been resolved by the new cmake refactoring and the new pybind version.

@wassimj wassimj closed this as completed Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake help wanted Extra attention is needed python
Projects
None yet
Development

No branches or pull requests

4 participants