Re: [PATCH] gnu: Add ECL.

[Mark H Weaver]

address@hidden (Taylan Ulrich "Bayırlı/Kammer") writes:

> The license declaration of this recipe is noteworthy.  I diligently
> commented about the places each license appears in, from the non-LGPL2
> places mentioned in the "Copyright" file, so as to make it easier to
> verify nothing has been left out.  Tell me if I overdid it; I couldn't
> see a better way to keep some sanity amid the license jungle.
>
>
> From 8852980acdfbcec00cb794fa924e03a95d60d59f Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Taylan=20Ulrich=20Bay=C4=B1rl=C4=B1/Kammer?=
>  <address@hidden>
> Date: Thu, 12 Feb 2015 00:15:02 +0100
> Subject: [PATCH] gnu: Add ECL.
>
> * gnu/packages/lisp.scm (ecl): New variable.
> ---
>  gnu/packages/lisp.scm | 74 
> ++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 71 insertions(+), 3 deletions(-)
>
>
> diff --git a/gnu/packages/lisp.scm b/gnu/packages/lisp.scm
> index 0bacac4..2e5770a 100644
> --- a/gnu/packages/lisp.scm
> +++ b/gnu/packages/lisp.scm
> @@ -1,5 +1,6 @@
>  ;;; GNU Guix --- Functional package management for GNU
>  ;;; Copyright © 2014 John Darrington <address@hidden>
> +;;; Copyright © 2015 Taylan Ulrich Bayırlı/Kammer <address@hidden>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -20,13 +21,17 @@
>    #:use-module (gnu packages)
>    #:use-module ((guix licenses) #:prefix license:)
>    #:use-module (guix packages)
> +  #:use-module (guix download)
> +  #:use-module (guix utils)
> +  #:use-module (guix build-system gnu)
>    #:use-module (gnu packages readline)
>    #:use-module (gnu packages texinfo)
>    #:use-module (gnu packages texlive)
>    #:use-module (gnu packages m4)
> -  #:use-module (guix download)
> -  #:use-module (guix utils)
> -  #:use-module (guix build-system gnu))
> +  #:use-module (gnu packages which)
> +  #:use-module (gnu packages multiprecision)
> +  #:use-module (gnu packages bdw-gc)
> +  #:use-module (gnu packages libffi))
>  
>  (define-public gcl
>    (package

In general, it's probably better to avoid unnecessary rearrangements
like this, since it will tend to cause conflicts when other people have
pending patches in the same module.

> @@ -81,3 +86,66 @@ stratified garbage collection strategy, a source-level 
> debugger and a built-in
>  interface to the Tk widget system.")
>      (license license:lgpl2.0+)))
>  
> +(define-public ecl
> +  (package
> +    (name "ecl")
> +    (version "13.5.1")
> +    (source
> +     (origin
> +       (method url-fetch)
> +       (uri (string-append "mirror://sourceforge/ecls/ecls/"
> +                           (version-prefix version 2) "/ecl-" version 
> ".tgz"))

Our convention is to write (version-major+minor version) since it
results in more readable code, even though it is a few more characters
and will force the remaining arguments to the next line.

> +       (sha256
> +        (base32 "18ic8w9sdl0dh3kmyc9lsrafikrd9cg1jkhhr25p9saz0v75f77r"))))
> +    (build-system gnu-build-system)
> +    (native-inputs `(("which" ,which)))
> +    (inputs `(("gmp" ,gmp)
> +              ("libatomic-ops" ,libatomic-ops)
> +              ("libgc" ,libgc)
> +              ("libffi" ,libffi)))
> +    (arguments
> +     '(#:phases
> +       ;; The test-suite seems to assume that ECL is installed.  So re-order
> +       ;; the phases, then reference the installed executable.
> +       (let* ((phases %standard-phases)
> +              (check-phase (assq-ref phases 'check))
> +              (phases (alist-delete 'check phases))
> +              (phases (alist-cons-after 'install 'check check-phase phases))
> +              (phases
> +               (alist-cons-before
> +                'check 'pre-check
> +                (lambda* (#:key outputs #:allow-other-keys)
> +                  (substitute* '("build/tests/Makefile")
> +                    (("ECL=ecl")
> +                     (string-append
> +                      "ECL=" (assoc-ref outputs "out") "/bin/ecl"))))
> +                phases)))
> +         phases)

Hmm.  This is very far from our conventional style for phases.  I don't
doubt that our conventional style could be improved, but I'm fond of the
style above either.  Although there is no mutation, it is essentially
written in an imperative style.

How about something like this instead?

--8<---------------cut here---------------start------------->8---
     '(#:phases
       ;; The test-suite seems to assume that ECL is installed.  So re-order
       ;; the phases, then reference the installed executable.
       (let* ((check-phase (assq-ref %standard-phases 'check))
              (rearranged-phases (alist-cons-after
                                  'install 'check check-phase
                                  (alist-delete 'check %standard-phases))))
         (alist-cons-before
          'check 'pre-check
          (lambda* (#:key outputs #:allow-other-keys)
            (substitute* '("build/tests/Makefile")
              (("ECL=ecl")
               (string-append
                "ECL=" (assoc-ref outputs "out") "/bin/ecl"))))
          rearranged-phases))
--8<---------------cut here---------------end--------------->8---

> +       ;; Parallel builds explicitly not supported:
> +       ;; http://sourceforge.net/p/ecls/bugs/98/
> +       #:make-flags '("--jobs=1")))

Instead of this, please add the following build arguments:

       #:parallel-build? #f
       #:parallel-tests? #f

> +    (home-page "http://ecls.sourceforge.net/";)
> +    (synopsis "Embeddable Common Lisp")
> +    (description "ECL is an implementation of the Common Lisp language as
> +defined by the ANSI X3J13 specification.  Its most relevant features are: a
> +bytecode compiler and interpreter, being able to compile Common Lisp with any
> +C/C++ compiler, being able to build standalone executables and libraries, and
> +supporting ASDF, Sockets, Gray streams, MOP, and other useful components.")
> +    ;; The file "Copyright" points to some files and directories which aren't
> +    ;; under the lgpl2.0+ and instead contain many different licenses.  The
> +    ;; comments below should exhaust those files and directories, except for
> +    ;; the contrib/unicode/ directory whose .lisp files have no copyright or
> +    ;; license notice.
> +    (license
> +     (list
> +      license:lgpl2.0+                  ;contrib/: encodings, bytecomp,
> +                                        ;ecl-cdb, win32
> +      (license:x11-style "file://src/lsp/loop2.lsp")
> +      license:public-domain             ;src/lsp/: pprint.lsp, format.lsp,
> +                                        ;profile, serve-event, sockets
> +      license:expat                     ;contrib/: asdf, cl-simd, quicklisp
> +      license:x11                       ;contrib/: deflate
> +      (license:bsd-style "file://contrib/defsystem/defsystem.lisp")
> +      license:bsd-2                     ;contrib/: ecl-curl
> +      (license:x11-style "file://contrib/rt/rt.lisp")
> +      (license:bsd-style "file://src/clx/clx.lisp"))))) ;TI License

Yowza!  I appreciate you being so thorough, but this may be a bit over
the top :)  I'd like to hear what Ludovic thinks before okaying a push.

     Thanks!
       Mark