Subject: Re: [bug#70880] [PATCH v2 4/8] gnu: libigl: Update to 2.4.0.

From: Maxim Cournoyer <[email-masked]>
Date: Tue, 09 Jul 2024 22:20:42 -0400

"Artyom V. Poptsov" <[email-masked]> writes:

> * gnu/packages/engineering.scm (libigl): Update to 2.4.0.
>   [arguments]: Use Gexps.  Update configure flags.  Disable GLFW tests as they
>   are failing with SEGFAULT errors (see
>   <https://github.com/libigl/libigl/issues/2313>.)
>   Update "unpack-external" phase: add new dependencies.
>   Update "patch-cmake" phase to prevent CMake from trying to download external
>   dependencies.  Help CMake to find the package inputs.
>   Add "fix-assertions" and "install-copyleft-modules" phase.
>   [native-inputs]: Add catch2.
>   [inputs]: Use glfw-3.4.  Add openblas, imgui and spectra.  Add libigl
>   packages and imguizmo.  Remove catch2.

Nitpick: no need for hanging indents in GNU ChangeLog messages.

> Change-Id: Ib4ac842dc14fe1cbc6b0b87674bae1f261a1a970
> ---
>  gnu/packages/engineering.scm | 258 ++++++++++++++++++++++++-----------
>  1 file changed, 179 insertions(+), 79 deletions(-)
>
> diff --git a/gnu/packages/engineering.scm b/gnu/packages/engineering.scm
> index fcb73eab12..7f29e2c365 100644
> --- a/gnu/packages/engineering.scm
> +++ b/gnu/packages/engineering.scm
> @@ -33,7 +33,7 @@
>  ;;; Copyright © 2022 Malte Frank Gerdes <[email-masked]>
>  ;;; Copyright © 2022 Konstantinos Agiannis <[email-masked]>
>  ;;; Copyright © 2022 Greg Hogan <[email-masked]>
> -;;; Copyright © 2022 Artyom V. Poptsov <[email-masked]>
> +;;; Copyright © 2022-2024 Artyom V. Poptsov <[email-masked]>
>  ;;; Copyright © 2022 Maxim Cournoyer <[email-masked]>
>  ;;; Copyright © 2022, 2023 Felix Gruber <[email-masked]>
>  ;;; Copyright © 2023 Theofilos Pechlivanis <[email-masked]>
> @@ -162,6 +162,7 @@ (define-module (gnu packages engineering)
>    #:use-module (gnu packages text-editors)
>    #:use-module (gnu packages time)
>    #:use-module (gnu packages tls)
> +  #:use-module (gnu packages toolkits)
>    #:use-module (gnu packages tree-sitter)
>    #:use-module (gnu packages version-control)
>    #:use-module (gnu packages web)
> @@ -3510,7 +3511,7 @@ (define-public xfoil
>  (define-public libigl
>    (package
>      (name "libigl")
> -    (version "2.3.0")
> +    (version "2.4.0")
>      (source
>       (origin
>         (method git-fetch)
> @@ -3520,90 +3521,151 @@ (define-public libigl
>         (file-name (git-file-name name version))
>         (sha256
>          (base32
> -         "004a22ifq2vibgkgvrlyihqimpsfizvq5l448204kwfg3lkycajj"))))
> +         "0qlnpp8nxbahcky4d67dzn0ynbv3v037nbx1akq6h5rzhvkzq40x"))))
>      (build-system cmake-build-system)
>      (arguments
> -     `(#:configure-flags
> -       '("-DLIBIGL_USE_STATIC_LIBRARY=OFF"
> -         "-DLIBIGL_BUILD_TESTS=ON"
> -         "-DLIBIGL_BUILD_TUTORIALS=OFF"
> -         "-DLIBIGL_EXPORT_TARGETS=ON"
> -         ;; The following options disable tests for the corresponding libraries.
> -         ;; The options do not affect whether the libraries are linked to
> -         ;; libigl or not, they are used for tests.
> -         "-DLIBIGL_WITH_COMISO=OFF"
> -         "-DLIBIGL_WITH_CORK=OFF"
> -         "-DLIBIGL_WITH_MATLAB=OFF"
> -         "-DLIBIGL_WITH_MOSEK=OFF"
> -         "-DLIBIGL_WITH_TRIANGLE=OFF" ;; Undefined reference to "triangulate".
> -         "-DLIBIGL_WITH_OPENGL_GLFW_IMGUI=OFF")
> +     (list #:configure-flags
> +           #~(list "-DLIBIGL_USE_STATIC_LIBRARY=OFF"
> +                   "-DLIBIGL_BUILD_TESTS=ON"
> +                   "-DLIBIGL_BUILD_TUTORIALS=OFF"
> +                   "-DLIBIGL_INSTALL=ON"
> +                   "-DFETCHCONTENT_FULLY_DISCONNECTED=ON"
> +                   (format #f "-DCatch2_DIR=~a/lib/cmake/catch2/"
> +                           #$(this-package-input "catch2"))
> +                   (format #f "-DSpectra_DIR=~a/share/pectra/cmake/"
> +                           #$(this-package-input "spectra"))
> +                   ;; The following options disable tests for the corresponding libraries.
> +                   ;; The options do not affect whether the libraries are linked to
> +                   ;; libigl or not, they are used for tests.
> +                   "-DLIBIGL_WITH_COMISO=OFF"
> +                   "-DLIBIGL_WITH_CORK=OFF"
> +                   "-DLIBIGL_MATLAB=OFF"
> +                   "-DLIBIGL_MOSEK=OFF"
> +                   ;; XXX: GLFW tests are failing with SEGFAULT.  See
> +                   ;;      <https://github.com/libigl/libigl/issues/2313>
> +                   "-DLIBIGL_GLFW_TESTS=OFF")
> +           #:build-type "Release"
>         #:phases
> -       (modify-phases %standard-phases
> +       #~(modify-phases %standard-phases
>           (add-after 'unpack 'unpack-external
>             (lambda _
>               (setenv "HOME" (getcwd)) ;; cmake needs this to export modules
>               (mkdir "external")
>               (copy-recursively (assoc-ref %build-inputs "libigl-glad") "external/glad")
> -             (copy-recursively (assoc-ref %build-inputs "libigl-stb") "external/stb")
> +             (copy-recursively (assoc-ref %build-inputs "libigl-test-data") "external/test-data")
> +             (copy-recursively (assoc-ref %build-inputs "libigl-comiso") "external/comiso")
>               (copy-recursively (assoc-ref %build-inputs "libigl-tetgen") "external/tetgen")
> -             (copy-recursively (assoc-ref %build-inputs "libigl-predicates") "external/predicates")))
> +             (copy-recursively (assoc-ref %build-inputs "libigl-predicates") "external/predicates")
> +             (copy-recursively (assoc-ref %build-inputs "imguizmo") "external/imguizmo")
> +             (copy-recursively (assoc-ref %build-inputs "eigen") "external/eigen")))

You'll want to break these long lines; we aim to limit line width below
80 columns.  'guix lint' should complain.

>           (add-after 'unpack-external 'patch-cmake
> -           (lambda _
> +           (lambda* (#:key inputs #:allow-other-keys)
> +             (define (source-dir library-name)
> +               (format #f "SOURCE_DIR \"~a\""
> +                       (assoc-ref %build-inputs library-name)))
> +             (define (fix-external-library cmake source)
> +               (substitute* (format #f "cmake/recipes/external/~a.cmake"
> +                                    cmake)
> +                 (("GIT_REPOSITORY.*") (source-dir source))
> +                 (("GIT_TAG.*")        "")))
>               ;; Fix references to external libraries
> -             (substitute* "cmake/libigl.cmake"
> -               (("if\\(NOT TARGET Eigen3::Eigen\\)" all)
> -                (string-append "find_package(Eigen3 CONFIG REQUIRED)\n" all))
> -               (("if\\(NOT TARGET CGAL::CGAL\\)" all)
> -                (string-append "find_package(CGAL CONFIG COMPONENTS Core)\n" all))
> -               (("if\\(NOT TARGET tinyxml2\\)" all)
> -                (string-append "find_package(tinyxml2 CONFIG REQUIRED)\n"
> -                               "if (NOT TARGET tinyxml2::tinyxml2)"))
> -               (("if\\(NOT TARGET embree\\)" all)
> -                (string-append "find_package(embree 3 CONFIG REQUIRED)\n" all))
> -               (("if\\(NOT TARGET glfw\\)" all)
> -                (string-append "find_package(glfw3 CONFIG REQUIRED)\n" all))
> -               (("igl_download_glad\\(\\)" all) "")
> -               (("igl_download_stb\\(\\)" all) "")
> -               (("igl_download_tetgen\\(\\)" all) "")
> -               (("igl_download_triangle\\(\\)" all) "")
> -               (("igl_download_predicates\\(\\)" all) ""))
> -             (substitute* "tests/CMakeLists.txt"
> -               (("igl_download_test_data\\(\\)") "")
> -               (("set\\(IGL_TEST_DATA.*")
> -                (format #f "set(IGL_TEST_DATA ~a)\n"
> -                        (assoc-ref %build-inputs "libigl-test-data")))
> -               (("igl_download_catch2\\(\\)") "find_package(Catch2 CONFIG REQUIRED)")
> -               (("list\\(APPEND CMAKE_MODULE_PATH \\$\\{LIBIGL_EXTERNAL\\}/catch2/contrib\\)")
> -                "")
> -               (("add_subdirectory\\(\\$\\{LIBIGL_EXTERNAL\\}/catch2 catch2\\)") ""))
> -             ;; Install otherwise missing headers
> -             (substitute* "cmake/libigl.cmake"
> -               (("install_dir_files\\(copyleft\\)" all)
> -                (string-join (list all
> -                                   "install_dir_files(copyleft/cgal)"
> -                                   "install_dir_files(copyleft/opengl)"
> -                                   "install_dir_files(copyleft/tetgen)"
> -                                   "install_dir_files(embree)"
> -                                   "install_dir_files(opengl)"
> -                                   "install_dir_files(png)"
> -                                   "install_dir_files(predicates)"
> -                                   "install_dir_files(xml)")
> -                             "\n"))))))))
> +             (fix-external-library "comiso" "libigl-comiso")
> +             (fix-external-library "tetgen" "libigl-tetgen")
> +             (fix-external-library "triangle" "libigl-triangle")
> +             (fix-external-library "predicates" "libigl-predicates")
> +             (fix-external-library "glad" "libigl-glad")
> +             (fix-external-library "libigl_tests_data" "libigl-test-data")
> +             (fix-external-library "stb" "libigl-stb")
> +             (substitute* "cmake/recipes/external/imguizmo.cmake"
> +               (("if\\(TARGET imguizmo::imguizmo\\)")
> +                "if(true)")
> +               (("target_link_libraries.*")
> +                (format #f "include_directories(~a/include/imgui/)"
> +                        (assoc-ref inputs "imgui"))))
> +
> +             (substitute* "cmake/igl/igl_add_test.cmake"
> +               (("include\\(\".*/contrib/Catch.cmake\"\\)")
> +                (format #f
> +                        "include(\"~a/lib/cmake/Catch2/Catch.cmake\")"
> +                        (assoc-ref inputs "catch2"))))
> +             (substitute* "cmake/recipes/external/cgal.cmake"
> +               (("FetchContent_Populate\\(cgal\\)")
> +                "find_package(CGAL CONFIG COMPONENTS Core)\nreturn()"))
> +             (substitute* "cmake/recipes/external/eigen.cmake"
> +               (("FetchContent_Populate\\(eigen\\)")
> +                "find_package(Eigen3 CONFIG REQUIRED)\nreturn()"))
> +             (substitute* "cmake/recipes/external/catch2.cmake"
> +               (("message.*")
> +                "find_package(Catch2 CONFIG)\nreturn()"))
> +             (substitute* "cmake/recipes/external/libigl_imgui_fonts.cmake"
> +               (("if\\(TARGET igl::imgui_fonts\\)")
> +                "if(true)"))
> +             (substitute* "cmake/recipes/external/tinyxml2.cmake"
> +               (("FetchContent_Populate\\(tinyxml2\\)")
> +                "find_package(tinyxml2 CONFIG REQUIRED)\nreturn()"))
> +             (substitute* "cmake/recipes/external/embree.cmake"
> +               (("FetchContent_MakeAvailable\\(embree\\)")
> +                (string-join (list "find_package(Embree 3 CONFIG)"
> +                                   "add_library(embree::embree ALIAS embree)"
> +                                   "return()")
> +                             "\n")))
> +             (substitute* "cmake/recipes/external/glfw.cmake"
> +               (("FetchContent_MakeAvailable\\(glfw\\)")
> +                (string-join
> +                 (list "find_package(glfw3 CONFIG REQUIRED)"
> +                       "add_library(glfw::glfw ALIAS glfw)"
> +                       "return()")
> +                 "\n")))
> +             (substitute* "cmake/recipes/external/imgui.cmake"
> +               (("FetchContent_MakeAvailable\\(imgui\\)")
> +                "return()"))))

All these FetchContent substitutions could perhaps be unnecessary if
they were referring to the right package name and with the use of the
-DFETCHCONTENT_TRY_FIND_PACKAGE_MODE=ALWAYS configure flag, which forces
CMake to look at the packages from the system and fail if they aren't
found instead of fetching their source and building them.

> +         (add-after 'unpack-external 'fix-assertions
> +           (lambda _
> +             ;; Current Tetgen version has a bug.
> +             (substitute* "include/igl/copyleft/tetgen/tetgenio_to_tetmesh.cpp"
> +               (("assert\\(out.numberofpoints == out.numberofpointmarkers\\);")
> +                ";"))
> +             ;; CGAL has a bug in assertion as well.
> +             (substitute* "include/igl/copyleft/cgal/trim_with_solid.cpp"
> +               (("assert\\(I.size\\(\\) == Vr.rows\\(\\)\\);")
> +                ";"))))
> +
> +         ;; XXX: Install modules as CMake fails to install them.
> +         (add-after 'install 'install-includes
> +           (lambda* (#:key inputs outputs #:allow-other-keys)
> +             (let* ((out (assoc-ref outputs "out"))
> +                    (include-dir (string-append out "/include/igl/")))
> +               (for-each (lambda (module)
> +                           (copy-recursively (format #f "../source/include/igl/~a"
> +                                                     module)
> +                                             (format #f "~a/~a" include-dir module)))
> +                         (list "copyleft/cgal"
> +                               "copyleft/opengl2"
> +                               "copyleft/tetgen"
> +                               "embree"
> +                               "opengl"
> +                               "predicates"
> +                               "xml"))))))))
> +
> +    (native-inputs (list catch2))
>      ;; XXX: Inputs are currently only used to build tests.
>      ;;      We would need to patch the CMake recipe to build a shared library
>      ;;      with all of these.
>      (inputs
>       `(("boost" ,boost)
> -       ("catch2" ,catch2)
>         ("cgal" ,cgal)
>         ("eigen" ,eigen)
>         ("embree" ,embree)
> -       ("glfw" ,glfw)
> +       ("glfw" ,glfw-3.4)
>         ("gmp" ,gmp)
>         ("mesa" ,mesa)
>         ("mpfr" ,mpfr)
>         ("tbb" ,tbb)
>         ("tinyxml2" ,tinyxml2)
> +       ("openblas" ,openblas)
> +       ("imgui" ,imgui)
> +       ("spectra" ,spectra)
>         ;; When updating this package, update commit fields below according to
>         ;; the hashes listed in "cmake/LibiglDownloadExternal.cmake".
>         ("libigl-test-data"
> @@ -3615,13 +3677,16 @@ (define-public libigl
>             (file-name (git-file-name "libigl-test-data" version))
>             (sha256 (base32 "1wxglrxw74xw4a4jmmjpm8719f3mnlbxbwygjb4ddfixxxyya4i2"))))
>         ("libigl-glad"
> -        ,(origin
> -           (method git-fetch)
> -           (uri (git-reference
> -                 (url "https://github.com/libigl/libigl-glad")
> -                 (commit "09b4969c56779f7ddf8e6176ec1873184aec890f")))
> -           (file-name (git-file-name "libigl-glad" version))
> -           (sha256 (base32 "0rwrs7513ylp6gxv7crjzflapcg9p7x04nzfvywgl665vl53rawk"))))
> +        ,(let* ((commit "ead2d21fd1d9f566d8f9a9ce99ddf85829258c7a")
> +                (revision "0")
> +                (version (git-version "0.0.0" revision commit)))
> +           (origin
> +             (method git-fetch)
> +             (uri (git-reference
> +                   (url "https://github.com/libigl/libigl-glad")
> +                   (commit commit)))
> +             (file-name (git-file-name "libigl-glad" version))
> +             (sha256 (base32 "079fd5yrbd713nq7slhhgq79wns85pc564ydlkjl9gf43d3220ay")))))
>         ("libigl-stb"
>          ,(origin
>             (method git-fetch)
> @@ -3631,22 +3696,57 @@ (define-public libigl
>             (file-name (git-file-name "libigl-stb" version))
>             (sha256 (base32 "0wwlb370z40y63ic3ny6q7lxibhixg2k1pjdkl4ymzv79zld28kj"))))
>         ("libigl-predicates"
> +        ,(let* ((commit "50c2149e7a520d13cd10e9aeff698bd68edd5a4f")
> +                (revision "0")
> +                (version (git-version "0.0.0" revision commit)))
> +           (origin
> +             (method git-fetch)
> +             (uri (git-reference
> +                   (url "https://github.com/libigl/libigl-predicates.git")
> +                   (commit commit)))
> +             (file-name (git-file-name "libigl-predicates" version))
> +             (sha256 (base32 "0yiqhzry2qhb1p0v9sldlnpqsn4y8cln8r6y08lafkc9kc4qy8jz")))))
> +       ;; TODO: Package tetgen separately from <http://www.tetgen.org>
> +       ("libigl-tetgen"
> +        ,(let* ((commit "4f3bfba3997f20aa1f96cfaff604313a8c2c85b6")
> +                (revision "0")
> +                (version (git-version "0.0.0" revision commit)))
> +           (origin
> +             (method git-fetch)
> +             (uri (git-reference
> +                   (url "https://github.com/libigl/tetgen.git")
> +                   (commit commit)))
> +             (file-name (git-file-name "libigl-tetgen" version))
> +             (sha256 (base32 "1k724syssw37py7kwmibk3sfwkkgyjyy7qkijnhn6rjm91g8qxsg")))))
> +       ("libigl-comiso"
> +        ,(let* ((commit "562efe333edc8e649dc101469614f43378b1eb55")
> +                (revision "0")
> +                (version (git-version "0.0.0" revision commit)))
> +           (origin
> +             (method git-fetch)
> +             (uri (git-reference
> +                   (url "https://github.com/libigl/comiso.git")
> +                   (commit commit)))
> +             (file-name (git-file-name "libigl-comiso" version))
> +             (sha256 (base32 "048zryh9ydd1dqwzs14vj7r3fd6yyq6n4zl6d1b0yb1iwrqfy6ba")))))
> +       ("libigl-triangle"
>          ,(origin
>             (method git-fetch)
>             (uri (git-reference
> -                 (url "https://github.com/libigl/libigl-predicates.git")
> -                 (commit "488242fa2b1f98a9c5bd1441297fb4a99a6a9ae4")))
> -           (file-name (git-file-name "libigl-predicates" version))
> -           (sha256 (base32 "13bd98g8lgcq37i3crj66433z09grnb2xjrcqpwqmyn147rp5wyh"))))
> -       ;; TODO: Package tetgen separately from <http://www.tetgen.org>
> -       ("libigl-tetgen"
> +                 (url "https://github.com/libigl/triangle.git")
> +                 (commit "6bbd92c7ddd6c803c403e005e1132eadb38fbe68")))
> +           (file-name (git-file-name "libigl-triangle" version))
> +           (sha256 (base32 "0d35mfqwdk99xn1lpjzz9w5axq016r6xy5vr00lb4mvb05limxl3"))))
> +       ;; XXX: This is a source-only library which is currently required only for
> +       ;;      libigl.
> +       ("imguizmo"
>          ,(origin
>             (method git-fetch)
>             (uri (git-reference
> -                 (url "https://github.com/libigl/tetgen.git")
> -                 (commit "4f3bfba3997f20aa1f96cfaff604313a8c2c85b6")))
> -           (file-name (git-file-name "libigl-tetgen" version))
> -           (sha256 (base32 "1k724syssw37py7kwmibk3sfwkkgyjyy7qkijnhn6rjm91g8qxsg"))))))
> +                 (url "https://github.com/CedricGuillemet/ImGuizmo")
> +                 (commit "1.83")))
> +           (file-name (git-file-name "imguizmo" version))
> +           (sha256 (base32 "14ywf96nvxf5c081pwypyzjwx9vyq78glbzinc81558v1sxiy2v0"))))))
>      (home-page "https://libigl.github.io/")
>      (synopsis "Simple C++ geometry processing library")
>      (description "This library provides functionality for shape modelling,

The rest LGTM, although it'd be nice to modernize the definition to use
gexps and remove the input labels.

-- 
Thanks,
Maxim