qcscine / molassembler

Chemoinformatics toolkit with support for inorganic molecules
https://scine.ethz.ch/download/molassembler
BSD 3-Clause "New" or "Revised" License
31 stars 7 forks source link

Fails to detect installed dependencies #4

Open awvwgk opened 2 years ago

awvwgk commented 2 years ago

Installed versions of nauty 2.7.2, RingDecomposerLib 1.1.3 and nlohmann_json 3.10.5 are currently ignored by the build system and instead the dependencies are fetched again. Is there a way to avoid the redundant builds and make sure molassembler is build against the proper dependencies.

Issue #3 might be related to multiple versions of nauty being available on the system.

weymutht commented 2 years ago

This should be possible for nauty and RingDecomposerLib, as Molassembler executes a standard find_package call in these cases. If the packages can't be found because they're installed in a non-default location, you can help CMake to find them by setting the environment variable CMAKE_PREFIX_PATH accordingly. (Alternatively, you can also use package-specific variables like RingDecomposerLib_DIR).

For nlohmann/json, Molassembler doesn't provide a way to use it as a preinstalled dependency. Since this dependency consists only of two files which have to be downloaded and don't add a lot of compilation overhead, I think it's not worth the hassle to externalize this dependency.

awvwgk commented 2 years ago

For nlohmann/json, Molassembler doesn't provide a way to use it as a preinstalled dependency

This is a problem, even with a single header file the project should be able to find it as preinstalled dependency. Especially for a popular package like nlohmann/json which is available in most package managers this should be the preferred approach.


diff --git a/cmake/ImportJSON.cmake b/cmake/ImportJSON.cmake
index 7d93820..1a15583 100644
--- a/cmake/ImportJSON.cmake
+++ b/cmake/ImportJSON.cmake
@@ -5,6 +5,11 @@
 #

 macro(import_json)
+  if(NOT TARGET json)
+    find_package(json)
+  endif()
+
+  if(NOT TARGET json)
   set(JSON_VERSION "3.9.1")
   set(JSON_LICENSE_FILE "${CMAKE_CURRENT_BINARY_DIR}/nlohmann/LICENSE")
   set(JSON_HPP_FILE "${CMAKE_CURRENT_BINARY_DIR}/nlohmann/json.hpp")
@@ -31,4 +36,5 @@ macro(import_json)
     DESTINATION share/doc/molassembler/licenses
     RENAME nlohmann-json.txt
   )
+  endif()
 endmacro()
awvwgk commented 2 years ago

After checking the nauty import I noticed that I cannot provide my own installation since you require a CMake based build at:

https://github.com/qcscine/molassembler/blob/8be2afcf42aa9d8bf13bd54d406eccff352f0970/cmake/ImportNauty.cmake#L10-L12

However nauty is an autoconf package and does not work with find_package out of the box unless you provide a custom finder, which seems to not be available in this repository. I had to add the following rudimentary finder (which should also support your CMake export via conan):

# cmake/Findnauty.cmake
if(NOT TARGET "nauty")
  find_package("nauty" CONFIG)
endif()

if(NOT TARGET "nauty")
  find_library(NAUTY_LIBRARY "nauty")
  find_path(NAUTY_INCLUDE_DIR "include/nauty")

  add_library("nauty" INTERFACE IMPORTED)
  target_include_directories(
    "nauty"
    INTERFACE
    "${NAUTY_INCLUDE_DIR}"
  )
  target_link_libraries(
    "nauty"
    INTERFACE
    "${NAUTY_LIBRARY}"
  )
endif()

Unfortunately, I'm still hitting #3, but at least the right library is used now.

awvwgk commented 2 years ago

Similar finder works for RingDecomposerLib which doesn't export CMake package files in version 1.1.3

# cmake/FindRingDecomposerLib.cmake
if(NOT TARGET "RingDecomposerLib")
  find_package("RingDecomposerLib" CONFIG)
endif()

if(NOT TARGET "RingDecomposerLib")
  find_library(RingDecomposerLib_LIBRARY "RingDecomposerLib")
  find_path(RingDecomposerLib_HEADER "include/RingDecomposerLib.h")
  get_filename_component(RingDecomposerLib_INCLUDE_DIR "${RingDecomposerLib_HEADER}" DIRECTORY)

  add_library("RingDecomposerLib" INTERFACE IMPORTED)
  target_include_directories(
    "RingDecomposerLib"
    INTERFACE
    "${RingDecomposerLib_INCLUDE_DIR}"
  )
  target_link_libraries(
    "RingDecomposerLib"
    INTERFACE
    "${RingDecomposerLib_LIBRARY}"
  )
endif()