Re: [PATCH 2/3] gnu: Add ledger.

[Alex Kost]

Alex Griffin (2016-05-12 19:31 +0300) wrote:

> On Thu, May 12, 2016, at 04:12 AM, Alex Kost wrote:
>> Hello, was this package built successfully for you?  I tried it but the
>> build phase failed (I'm attaching the last part of the build process [1]
>> just in case).  I don't know, maybe I just don't have enough memory
>> (3GB) to build it (I had such problems with cmake before).
>
> Yes, it builds fine for me. It looks like the important line in your
> build log is "c++: internal compiler error: Killed (program cc1plus)",
> which could be from running out of memory. Does it still happen if you
> add `#:parallel-build? #f` to the build system arguments?

Oh indeed, if parallel-build is disabled, it is built successfully.
Thank you (and Leo)!

So I don't know, should ‘#:parallel-build? #f’ be used in the final
package recipe?  I guess not, as it looks like a problem on my side (not
enough memory for parallel-build).

>> Every phase should return non-false value if it succeeded.  So since the
>> returned value of 'install-file' is not specified, we add #t to the end
>> of such phases.  Please add #t to all phases except the following
>> (build-doc) because zero? returns #t if succeeded.
>
> OK, done.
>
>> Unlike configure-flags where we can use only %build-inputs, in phases,
>> it is better to use a functional style using 'inputs' passed to a phase
>> as argument:
>
> Done.
>
>> It doesn't matter but usually we put #:configure-flags before #:phases.
>
> Done. Attached is an updated patch.

Thank you!  Overall the patch looks good to me, I have only a couple of
comments more (I'm sorry I didn't notice it before :-)).  But no need to
resent the patch, I can fix it on my side and commit it.  The only thing
that is unclear for me is whether this parallel-build should be disabled
or not.

> From 0090f1d526f35a72144a3d32158cbad987ef4d10 Mon Sep 17 00:00:00 2001
> From: Alex Griffin <address@hidden>
> Date: Sat, 7 May 2016 12:20:47 -0500
> Subject: [PATCH 2/2] gnu: Add ledger.
>
> * gnu/packages/finance.scm (ledger): New variable.
[...]
> +(define-public ledger
> +  (package
> +    (name "ledger")
> +    (version "3.1.1")
> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append 
> "https://github.com/ledger/ledger/archive/v";
> +                                  version
> +                                  ".tar.gz"))
> +              (file-name (string-append name "-" version ".tar.gz"))
> +              (sha256
> +               (base32
> +                "12jlv3gsjhrja25q9hrwh73cdacd2l3c2yyn8qnijav9mdhnbw4h"))))
> +    (build-system cmake-build-system)
> +    (arguments
> +     `(#:configure-flags
> +       `("-DBUILD_DOCS:BOOL=ON"
> +         "-DBUILD_WEB_DOCS:BOOL=ON"
> +         "-DBUILD_EMACSLISP:BOOL=ON"
> +         "-DUSE_PYTHON:BOOL=ON"
> +         "-DCMAKE_INSTALL_LIBDIR:PATH=lib"
> +         ,(string-append "-DUTFCPP_INCLUDE_DIR:PATH="
> +                         (assoc-ref %build-inputs "utfcpp")
> +                         "/include"))
> +       #:phases
> +       (let* ((out (assoc-ref %outputs "out"))
> +              (examples (string-append out "/share/doc/ledger/examples"))
> +              (site-lisp (string-append out "/share/emacs/site-lisp")))
> +         (modify-phases %standard-phases

I only notice just now that you have 'modify-phases' inside this let.  I
would rather make local 'let'-s with necessary variables inside phases.
I mean 'examples' directory is needed only inside 'install-examples'
phase; 'site-lisp' inside 'relocate-elisp' phase.  Also it is better to
use 'outputs' argument passed to phases instead of the global %outputs.

> +           (add-before 'configure 'install-examples
> +             (lambda _
> +               (install-file "test/input/sample.dat" examples)
> +               (install-file "test/input/demo.ledger" examples)
> +               #t))
> +           (add-after 'build 'build-doc
> +             (lambda _ (zero? (system* "make" "doc"))))
> +           (add-before 'check 'check-setup
> +             ;; one test fails if it can't set the timezone
> +             (lambda* (#:key inputs #:allow-other-keys)
> +               (setenv "TZDIR"
> +                       (string-append (assoc-ref inputs "tzdata")
> +                                      "/share/zoneinfo"))
> +               #t))
> +           (add-after 'install 'relocate-elisp
> +             (lambda _
> +               (mkdir-p (string-append site-lisp "/guix.d"))
> +               (rename-file (string-append site-lisp "/ledger-mode")
> +                            (string-append site-lisp "/guix.d/ledger-mode"))
> +               #t))))))

It is also good to generate "ledger-autoloads.el" file.  This allows a
user to have "M-x ledger-mode" available by default without any
additional settings.  This can be done by calling:

  (emacs-generate-autoloads "ledger" "<elisp-dir>")

However it is a bit tricky: 'emacs-generate-autoloads' procedure is
placed in (guix build emacs-utils) module which is not available for
cmake build system by default, so this module should be:

1) imported (via #:imported-modules argument), i.e. available for using
2) used (via #:modules argument).

Overall, here is how I would write 'arguments' field of this package:

    (arguments
     `(#:modules ((guix build cmake-build-system)
                  (guix build utils)
                  (guix build emacs-utils))
       #:imported-modules (,@%cmake-build-system-modules
                           (guix build emacs-utils))
       #:configure-flags
       `("-DBUILD_DOCS:BOOL=ON"
         "-DBUILD_WEB_DOCS:BOOL=ON"
         "-DBUILD_EMACSLISP:BOOL=ON"
         "-DUSE_PYTHON:BOOL=ON"
         "-DCMAKE_INSTALL_LIBDIR:PATH=lib"
         ,(string-append "-DUTFCPP_INCLUDE_DIR:PATH="
                         (assoc-ref %build-inputs "utfcpp")
                         "/include"))
       #:parallel-build? #f  ; needed?
       #:phases
       (modify-phases %standard-phases
         (add-before 'configure 'install-examples
           (lambda* (#:key outputs #:allow-other-keys)
             (let ((examples (string-append (assoc-ref outputs "out")
                                            "/share/doc/ledger/examples")))
               (install-file "test/input/sample.dat" examples)
               (install-file "test/input/demo.ledger" examples))
             #t))
         (add-after 'build 'build-doc
           (lambda _ (zero? (system* "make" "doc"))))
         (add-before 'check 'check-setup
           ;; One test fails if it can't set the timezone.
           (lambda* (#:key inputs #:allow-other-keys)
             (setenv "TZDIR"
                     (string-append (assoc-ref inputs "tzdata")
                                    "/share/zoneinfo"))
             #t))
         (add-after 'install 'relocate-elisp
           (lambda* (#:key outputs #:allow-other-keys)
             (let* ((site-dir (string-append (assoc-ref outputs "out")
                                             "/share/emacs/site-lisp"))
                    (guix-dir (string-append site-dir "/guix.d"))
                    (orig-dir (string-append site-dir "/ledger-mode"))
                    (dest-dir (string-append guix-dir "/ledger-mode")))
               (mkdir-p guix-dir)
               (rename-file orig-dir dest-dir)
               (emacs-generate-autoloads ,name dest-dir))
             #t)))))

The rest looks good to me, so if there will be no other comments, I will
commit it (without ‘#:parallel-build? #f’).

> +    (inputs `(("boost" ,boost)
> +              ("gmp" ,gmp)
> +              ("libedit" ,libedit)
> +              ("mpfr" ,mpfr)
> +              ("python" ,python-2)
> +              ("tzdata" ,tzdata)
> +              ("utfcpp" ,utfcpp)))
> +    (native-inputs `(("emacs-no-x" ,emacs-no-x)
> +                     ("groff" ,groff)
> +                     ("texinfo" ,texinfo)))
> +    (home-page "http://ledger-cli.org/";)
> +    (synopsis "Command-line double-entry accounting program")
> +    (description
> +     "Ledger is a powerful, double-entry accounting system that is
> +accessed from the UNIX command-line.  This may put off some users, since
> +there is no flashy UI, but for those who want unparalleled reporting
> +access to their data there are few alternatives.
> +
> +Ledger uses text files for input.  It reads the files and generates
> +reports; there is no other database or stored state.  To use Ledger,
> +you create a file of your account names and transactions, run from the
> +command line with some options to specify input and requested reports, and
> +get output.  The output is generally plain text, though you could generate
> +a graph or html instead.  Ledger is simple in concept, surprisingly rich
> +in ability, and easy to use.")
> +    ;; There are some extra licenses in files which do not presently get
> +    ;; installed when you build this package.  Different versions of the GPL
> +    ;; are used in the contrib and python subdirectories.  The bundled 
> version
> +    ;; of utfcpp is under the Boost 1.0 license. Also the file
> +    ;; `tools/update_copyright_year` has an Expat license.
> +    (license (list license:bsd-3
> +                   license:asl2.0     ; src/strptime.cc
> +                   (license:non-copyleft
> +                    "file://src/wcwidth.cc"
> +                    "See src/wcwidth.cc in the distribution.")
> +                   license:gpl2+))))  ; lisp/*

-- 
Alex