GNU bug report logs - #33059
[PATCH 00/10] Add the FEniCS Project

Previous Next

Full log

Message #71 received at 33059 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Paul Garlick <pgarlick <at> tourbillion-technology.com>
Cc: 33059 <at> debbugs.gnu.org
Subject: Re: [bug#33059] [PATCH 08/10] gnu: Add fenics-dolfin.
Date: Thu, 25 Oct 2018 00:12:19 +0200

Paul Garlick <pgarlick <at> tourbillion-technology.com> skribis:

> * gnu/packages/simulation.scm (fenics-dolfin): New variable.

[...]

> +         (add-before 'configure 'pre-configure
> +           (lambda _
> +             (use-modules (ice-9 regex)
> +                          (ice-9 rdelim)
> +                          (guix build utils)
> +                          (rnrs io ports))

Please use #:modules instead of an inner ‘use-modules’ form (which may
or may not work in future Guile versions.)

> +             ;; Add extra include directories required by the unit tests.
> +             (with-atomic-file-replacement "test/unit/cpp/CMakeLists.txt"
> +               (let ((rx (make-regexp "target_link_libraries")))
> +                 (lambda (in out)
> +                   (let loop ()
> +                     (let ((line (read-line in 'concat)))
> +                       (if (eof-object? line)
> +                           #t
> +                           (begin
> +                             (display line out)
> +                             (when (regexp-exec rx line)
> +                               (display
> +                                 (string-append
> +                                  "target_include_directories("
> +                                  "unittests PRIVATE "
> +                                  "${DOLFIN_SOURCE_DIR} "
> +                                  "${DOLFIN_SOURCE_DIR}/dolfin "
> +                                  "${DOLFIN_BINARY_DIR})\n") out))
> +                             (loop))))))))

Could this be achieved with a single ‘substitute*’?  It looks like that
would be more compact.

Also, perhaps this should be done in a ‘snippet’?

> +             ;; Add extra include directories required by the demo tests.
> +             (with-atomic-file-replacement "demo/CMakeLists.txt"
> +               (let ((rx (make-regexp "find_package")))
> +                 (lambda (in out)
> +                   (let loop ()
> +                     (let ((line (read-line in 'concat)))
> +                       (if (eof-object? line)
> +                           #t
> +                           (begin
> +                             (display line out)
> +                             (when (regexp-exec rx line)
> +                               (display
> +                                 (string-append
> +                                  "include_directories("
> +                                  "${DOLFIN_SOURCE_DIR} "
> +                                  "${DOLFIN_SOURCE_DIR}/dolfin "
> +                                  "${DOLFIN_BINARY_DIR})\n") out))
> +                             (loop))))))))))

Same question here.

> +             (call-with-output-file "CTestCustom.cmake"
> +               (lambda (port)
> +                 (display
> +                   (string-append
> +                    "set(CTEST_CUSTOM_TESTS_IGNORE "
> +                    "demo_bcs_serial "
> +                    "demo_bcs_mpi "
> +                    "demo_eigenvalue_serial "
> +                    "demo_eigenvalue_mpi "
> +                    "demo_navier-stokes_serial "

Could we avoid listing all the files here?  I’m thinking about something
like ‘scandir’ + ‘delete’.

> +    ;; The source code files for the DOLFIN C++ library are licensed under the
> +    ;; GNU Lesser General Public License, version 3 or later, with the
> +    ;; following exceptions:
> +    ;;
> +    ;; bsd-2:         cmake/modules/FindAMD.cmake
> +    ;;                cmake/modules/FindBLASHeader.cmake
> +    ;;                cmake/modules/FindCHOLMOD.cmake
> +    ;;                cmake/modules/FindEigen3.cmake
> +    ;;                cmake/modules/FindMPFR.cmake
> +    ;;                cmake/modules/FindNumPy.cmake
> +    ;;                cmake/modules/FindPETSc.cmake
> +    ;;                cmake/modules/FindPETSc4py.cmake
> +    ;;                cmake/modules/FindParMETIS.cmake
> +    ;;                cmake/modules/FindSCOTCH.cmake
> +    ;;                cmake/modules/FindSLEPc.cmake
> +    ;;                cmake/modules/FindSLEPc4py.cmake
> +    ;;                cmake/modules/FindSphinx.cmake
> +    ;;                cmake/modules/FindSUNDIALS.cmake
> +    ;;                cmake/modules/FindUFC.cmake
> +    ;;
> +    ;; bsd-3:         cmake/modules/FindBLAS.cmake
> +    ;;                cmake/modules/FindLAPACK.cmake
> +    ;;                cmake/modules/FindMPI.cmake
> +    ;;
> +    ;; public-domain: dolfin/geometry/predicates.cpp
> +    ;;                dolfin/geometry/predicates.h
> +    ;;
> +    ;; zlib:          dolfin/io/base64.cpp
> +    ;;                dolfin/io/base64.h
> +    ;;
> +    ;; expat:         dolfin/io/pugiconfig.hpp
> +    ;;                dolfin/io/pugixml.cpp
> +    ;;                dolfin/io/pugixml.hpp
> +    ;;
> +    ;; boost1.0:      test/unit/cpp/catch/catch.hpp

Thanks for the detailed licensing review!

IMO we don’t need to list the license of the .cmake files, which are
just build files (likewise, we usually ignore M4 files and shell scripts
found in Autotools-based projects.)

I wonder if we could use our ‘catch’ package and remove ‘catch.hpp’.

That’s it!

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.