CadQuery / OCP

Apache License 2.0
90 stars 27 forks source link

Dependency management #97

Closed vespakoen closed 1 year ago

vespakoen commented 1 year ago

Hi there!

I was trying to update the conda-forge OCP feedstock (https://github.com/conda-forge/ocp-feedstock)

And noticed that there are a lot of hardcoded paths, especially in the pywrap / bindgen module (https://github.com/CadQuery/pywrap)

This project also depends on the CONDA_PREFIX variable a lot.

I understand that this was probably done to get something working quickly, but in the long term I think it makes sense to depend less on these hard coded things.

For example, the find_project calls in this project's CMakeLists.txt should be able to provide the most paths that you need, for example:

find_package( LLVM REQUIRED )
find_package( VTK REQUIRED )
find_package( RapidJSON REQUIRED )
find_package( Python3 COMPONENTS Interpreter REQUIRED )

if(${CMAKE_SYSTEM_NAME} STREQUAL "Windows")
    set(PLATFORM Windows)
elseif(${CMAKE_SYSTEM_NAME} STREQUAL "Darwin")
    set(PLATFORM OSX)
else()
    set(PLATFORM Linux)
endif()

set( LIBCLANG_PATH ${LLVM_LIBRARY_DIR}/libclang${LLVM_PLUGIN_EXT} )
set( VTK_INCLUDE_DIR ${VTK_PREFIX_PATH}/include/vtk-${VTK_VERSION_MAJOR}.${VTK_VERSION_MINOR} )

message( STATUS "Python3_EXECUTABLE: ${Python3_EXECUTABLE}" )
message( STATUS "VTK_INCLUDE_DIR: ${VTK_INCLUDE_DIR}" )
message( STATUS "LIBCLANG_PATH: ${LIBCLANG_PATH}" )
message( STATUS "LLVM_INCLUDE_DIR: ${LLVM_INCLUDE_DIR}" )

I noticed CMake has trouble finding LLVM and VTK without "helping it", for example (on my arm64 macos machine) I had to use:

cmake .. -DLLVM_DIR=/opt/homebrew/Cellar/llvm/15.0.6/lib/cmake/llvm/ -DQt5_DIR=/opt/homebrew/opt/qt@5/lib/cmake/Qt5/

For CMake to find LLVM, and Qt5 (which VTK needed)

In short, I think it makes more sense to use the find_package result variables to find stuff, it will make it easier for packagers to build this thing, and when new releases come out for VTK / LLVM / CMake, the finding process hopefully even get's better and easier.

These are just my thoughts, and I thought i'd share them with you ;)

Thanks for making this project!

adam-urbanczyk commented 1 year ago

Thanks, the current setup works. You can always submit a PR (but please first open an issue explaining what you exactly want to achieve). BTW: I think you should be looking at the generated CMakeLists.txt and not the quoted one, unless you are trying to do something special like generate custom bindings.

I don't understand why are you mentioning that you had to help cmake find stuff. Nothing unusual/ I can do about it.

AFAIR there are no hard-coded paths in pywrap. Everything can be set from the command line. If not, please open an issue.

adam-urbanczyk commented 1 year ago

See #99

vespakoen commented 1 year ago

Awesome!! thanks!