Subject: Re: [PATCH v2 06/13] gnu: Add gemmi.

From: Liliana Marie Prikler <[email-masked]>
Date: Fri, 14 Oct 2022 23:32:44 +0200

Am Donnerstag, dem 13.10.2022 um 21:00 +0000 schrieb David Elsing:
> Liliana Marie Prikler <[email-masked]> writes:
> 
> > Am Freitag, dem 07.10.2022 um 15:21 +0000 schrieb David Elsing:
> > > * gnu/packages/chemistry.scm (gemmi): New variable.
> > > ---
> > >  gnu/packages/chemistry.scm                    | 118 +++++++++++
> > >  .../patches/gemmi-fix-sajson-types.patch      |  11 +
> > >  .../sajson-for-gemmi-build-with-gcc10.patch   |  45 ++++
> > >  .../sajson-for-gemmi-numbers-as-strings.patch | 195
> > > ++++++++++++++++++
> > >  4 files changed, 369 insertions(+)
> > >  create mode 100644 gnu/packages/patches/gemmi-fix-sajson-
> > > types.patch
> > >  create mode 100644 gnu/packages/patches/sajson-for-gemmi-build-
> > > with-
> > > gcc10.patch
> > >  create mode 100644 gnu/packages/patches/sajson-for-gemmi-
> > > numbers-as-
> > > strings.patch
> > > 
> > > diff --git a/gnu/packages/chemistry.scm
> > > b/gnu/packages/chemistry.scm
> > > index c517610fe8..d8f1608a3a 100644
> > > --- a/gnu/packages/chemistry.scm
> > > +++ b/gnu/packages/chemistry.scm
> > > @@ -6,6 +6,7 @@
> > >  ;;; Copyright © 2020 Björn Höfling
> > > <[email-masked]>
> > >  ;;; Copyright © 2020 Vincent Legoll <[email-masked]>
> > >  ;;; Copyright © 2021 Ricardo Wurmus <[email-masked]>
> > > +;;; Copyright © 2022 David Elsing <[email-masked]>
> > >  ;;;
> > >  ;;; This file is part of GNU Guix.
> > >  ;;;
> > > @@ -23,6 +24,7 @@
> > >  ;;; along with GNU Guix.  If not, see
> > > <http://www.gnu.org/licenses/>.
> > >  
> > >  (define-module (gnu packages chemistry)
> > > +  #:use-module (guix gexp)
> > >    #:use-module (guix packages)
> > >    #:use-module (guix utils)
> > >    #:use-module ((guix licenses) #:prefix license:)
> > > @@ -35,6 +37,7 @@ (define-module (gnu packages chemistry)
> > >    #:use-module (gnu packages boost)
> > >    #:use-module (gnu packages check)
> > >    #:use-module (gnu packages compression)
> > > +  #:use-module (gnu packages cpp)
> > >    #:use-module (gnu packages documentation)
> > >    #:use-module (gnu packages fontutils)
> > >    #:use-module (gnu packages gl)
> > > @@ -50,8 +53,10 @@ (define-module (gnu packages chemistry)
> > >    #:use-module (gnu packages qt)
> > >    #:use-module (gnu packages serialization)
> > >    #:use-module (gnu packages sphinx)
> > > +  #:use-module (gnu packages stb)
> > >    #:use-module (gnu packages xml)
> > >    #:use-module (guix build-system cmake)
> > > +  #:use-module (guix build-system copy)
> > >    #:use-module (guix build-system gnu)
> > >    #:use-module (guix build-system python))
> > >  
> > > @@ -566,3 +571,116 @@ (define-public python-pymol
> > >  used to prepare publication-quality figures, to share
> > > interactive
> > > results with
> > >  your colleagues, or to generate pre-rendered animations.")
> > >      (license license:bsd-3)))
> > > +
> > > +(define-public sajson-for-gemmi
> > > +  (package/inherit sajson
> > > +    (name "sajson-for-gemmi")
> > > +    (source (origin
> > > +              (inherit (package-source sajson))
> > > +              (patches (search-patches
> > > +                        "sajson-for-gemmi-numbers-as-
> > > strings.patch"
> > > +                        "sajson-for-gemmi-build-with-
> > > gcc10.patch"))))
> > > +    (arguments
> > > +     (substitute-keyword-arguments (package-arguments sajson)
> > > +       ((#:tests? _ #f) #f)
> > Don't forget the comment as to why they're disabled.
> > > +       ((#:phases phases)
> > > +        #~(modify-phases #$phases
> > > +            (delete 'build)))))))
> > Why is the build deleted?
> It fails due to the change and only builds the tests anyway.
> > 
> > Split here.
> > > +(define-public gemmi
> > > +  (package
> > > +    (name "gemmi")
> > > +    (version "0.5.7")
> > > +    (source (origin
> > > +              (method git-fetch)
> > > +              (uri (git-reference
> > > +                    (url
> > > "https://github.com/project-gemmi/gemmi")
> > > +                    (commit (string-append "v" version))))
> > > +              (file-name (git-file-name name version))
> > > +              (sha256
> > > +               (base32
> > > +               
> > > "00km5q726bslrw7xbfwb3f3mrsk19qbimfnl3hvr4wi1y3z8i18a"))
> > > +              (patches
> > > +               (search-patches "gemmi-fix-sajson-types.patch"))
> > > +              (modules '((guix build utils)))
> > > +              (snippet
> > > +               '(begin
> > > +                  (delete-file-recursively
> > > "include/gemmi/third_party")
> > > +                  (delete-file-recursively "third_party")))))
> > > +    (outputs '("out" "bin" "python"))
> > > +    (build-system cmake-build-system)
> > > +    (native-inputs
> > > +     (list fast-float
> > > +           optionparser
> > > +           pegtl
> > > +           pocketfft-cpp
> > > +           pybind11
> > > +           sajson-for-gemmi
> > > +           stb-sprintf
> > > +           tinydir))
> > > +    (inputs (list python zlib))
> > > +    (arguments
> > > +     (list
> > > +      #:configure-flags
> > > +      #~(let* ((python-lib
> > > +                (string-append
> > > +                 #$output:python "/lib/python"
> > > +                 #$(version-major+minor (package-version
> > > python))
> > > +                 "/site-packages")))
> > Note that python-build-system has a (site-packages) procedure. 
> > Writing
> > the boilerplate to include it in the build shouldn't be much worse
> > in
> > terms of lines than what you're doing here.
> > > +          (list "-DUSE_PYTHON=ON"
> > > +                (string-append "-DPYTHON_INSTALL_DIR=" python-
> > > lib)))
> > > +      #:phases
> > > +      #~(modify-phases %standard-phases
> > > +          (add-after 'unpack 'patch-includes
> > > +            (lambda _
> > > +              (substitute* "include/gemmi/sprintf.hpp"
> > > +                (("<stb/stb_sprintf.h>") "<stb_sprintf.h>"))
> > > +              (substitute* "include/gemmi/dirwalk.hpp"
> > > +                (("\"third_party/tinydir.h\"") "<tinydir.h>"))
> > > +              (substitute* "include/gemmi/cif.hpp"
> > > +                (("\"third_party/tao/pegtl.hpp\"")
> > > "<tao/pegtl.hpp>"))
> > > +              (substitute* "include/gemmi/json.hpp"
> > > +                (("\"third_party/sajson.h\"") "<sajson.h>"))
> > > +              (substitute* "python/gemmi.cpp"
> > > +               
> > > (("\"gemmi/third_party/tao/pegtl/parse_error.hpp\"")
> > > +                 "<tao/pegtl/parse_error.hpp>"))
> > > +              (substitute* '("include/gemmi/atof.hpp"
> > > +                             "include/gemmi/numb.hpp")
> > > +                (("\"third_party/fast_float.h\"")
> > > +                 "<fast_float/fast_float.h>"))
> > > +              (substitute* "include/gemmi/fourier.hpp"
> > > +                (("\"third_party/pocketfft_hdronly.h\"")
> > > +                 "<pocketfft_hdronly.h>"))))
> > I'd collect all the files into a (list ...) and then write out the
> > substitutes.
> Is it preferred to put list elements on separate lines (also in
> inputs)?
> 
Depends on the length, but in this case I'd say yes.
However, if things neatly fit into one line, you don't have to add a
new line.

Cheers