Re: [PATCH] Add LDC.

[Ricardo Wurmus]

Roel Janssen <address@hidden> writes:

> +(define-module (gnu packages ldc)
> +  #:use-module ((guix licenses) #:prefix license:)
> +  #:use-module (guix packages)
> +  #:use-module (guix download)
> +  #:use-module (guix build-system cmake)
> +  #:use-module (gnu packages)
> +  #:use-module (gnu packages base)
> +  #:use-module (gnu packages libedit)
> +  #:use-module (gnu packages llvm)
> +  #:use-module (gnu packages textutils)
> +  #:use-module (gnu packages zip))
> +
> +(define-public ldc
> +  (package
> +    (name "ldc")
> +    (version "0.16.1")
> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append
> +                    "https://github.com/ldc-developers/ldc/archive/v";
> +                    version ".tar.gz"))
> +              (file-name (string-append name "-" version ".tar.gz"))
> +              (sha256
> +               (base32
> +                "1jvilxx0rpqmkbja4m69fhd5g09697xq7vyqp2hz4hvxmmmv4j40"))))
> +    (build-system cmake-build-system)
> +    (supported-systems '("x86_64-linux" "i686-linux")) ; other architectures 
> are
> +    (arguments                                         ; not supported (yet).

This comment would better be placed above the ‘(supported-systems...)’
line.  Having it be part of the ‘(arguments’ line as well is not nice.
As a line comment it would then start with a double-semicolon.

> +     `(#:phases
> +       (modify-phases %standard-phases
> +         (add-after 'unpack 'unpack-phobos-source
> +           (lambda* (#:key inputs #:allow-other-keys)
> +               (with-directory-excursion "runtime/phobos"
> +                 (zero? (system* "tar" "xvf" (assoc-ref inputs "phobos-src")
> +                                 "--strip-components=1")))))
> +         (add-after 'unpack 'unpack-druntime-source
> +           (lambda* (#:key inputs #:allow-other-keys)
> +               (with-directory-excursion "runtime/druntime"
> +                 (zero? (system* "tar" "xvzf" (assoc-ref inputs 
> "druntime-src")
> +                                 "--strip-components=1")))))
> +         (add-after 'unpack 'unpack-dmd-testsuite-source
> +           (lambda* (#:key inputs #:allow-other-keys)
> +               (with-directory-excursion "tests/d2/dmd-testsuite"
> +                 (zero? (system* "tar" "xvzf"
> +                                 (assoc-ref inputs "dmd-testsuite-src")
> +                                 "--strip-components=1")))))

I still think that using one phase for unpacking additional tarballs
would totally suffice.  Something like this, maybe:

        (add-after 'unpack 'unpack-phobos-source
          (lambda* (#:key inputs #:allow-other-keys)
            (let ((unpack (lambda (source target)
                            (with-directory-excursion target
                              (zero? (system* "tar" "xvf"
                                              (assoc-ref inputs source)
                                              "--strip-components=1"))))))
              (and (unpack "phobos-src" "runtime/phobos")
                   (unpack "druntime-src" "runtime/druntime")
                   (unpack "dmd-testsuite-src" "tests/d2/dmd-testsuite")))))

It’s just a matter of taste, but I really find that your proposed three
phases look rather noisy with lots of boilerplate, which I don’t think
needs repeating.

> +         (add-after
> +          'unpack-phobos-source 'patch-phobos

Please pull the symbols onto the same line as “add-after”.

> +          (lambda* (#:key inputs #:allow-other-keys)
> +            (begin

“begin” is not needed in “lambda”.

> +              (substitute* "runtime/phobos/std/process.d"
> +                (("/bin/sh") (which "sh"))
> +                (("echo") (which "echo")))
> +              (substitute* "runtime/phobos/std/datetime.d"
> +                (("/usr/share/zoneinfo/")
> +                 (string-append (assoc-ref inputs "tzdata") 
> "/share/zoneinfo")))
> +              (substitute* "tests/d2/dmd-testsuite/Makefile"
> +                (("/bin/bash") (which "bash"))))
> +            #t))


> +         (add-after 'unpack-dmd-testsuite-source 'patch-dmd-testsuite
> +          (lambda _
> +            #t)))))

I don’t think this phase is needed.

> +    (inputs
> +     `(("libconfig" ,libconfig)
> +       ("libedit" ,libedit)
> +       ("tzdata" ,tzdata)))
> +    (native-inputs
> +     `(("llvm" ,llvm)
> +       ("clang" ,clang)
> +       ("unzip" ,unzip)
> +       ("phobos-src"
> +        ,(origin
> +          (method url-fetch)
> +          (uri (string-append
> +                "https://github.com/ldc-developers/phobos/archive/ldc-v";
> +                version ".tar.gz"))
> +          (sha256
> +           (base32
> +            "0sgdj0536c4nb118yiw1f8lqy5d3g3lpg9l99l165lk9xy45l9z4"))
> +          (patches (list (search-patch "ldc-disable-tests.patch")))))
> +       ("druntime-src"
> +        ,(origin
> +          (method url-fetch)
> +          (uri (string-append
> +                "https://github.com/ldc-developers/druntime/archive/ldc-v";
> +                version ".tar.gz"))
> +          (sha256
> +           (base32
> +            "0z4mkyddx6c4sy1vqgqvavz55083dsxws681qkh93jh1rpby9yg6"))))
> +       ("dmd-testsuite-src"
> +        ,(origin
> +          (method url-fetch)
> +          (uri (string-append
> +                
> "https://github.com/ldc-developers/dmd-testsuite/archive/ldc-v";
> +                version ".tar.gz"))
> +          (sha256
> +           (base32
> +            "0yc6miidzgl9k33ygk7xcppmfd6kivqj02cvv4fmkbs3qz4yy3z1"))))))
> +    (home-page "http://wiki.dlang.org/LDC";)
> +    (synopsis "LLVM compiler for the D programming language")
> +    (description
> +     "LDC is a compiler for the D programming language.  It is based on the
> +latest DMD frontend and uses LLVM as backend.")
> +    (license license:bsd-3))) ; with exceptions for the DMD frontend 
> (custom) and code from GDC (GPLv2+)

This comment is too long for a margin comment.  Better place it above
the ‘(license ...’ line (with double semicolon).

I don’t understand the comment.  What exceptions apply to the DMD
frontend?  What does “(custom)” mean?  Is it a different license?  If
this package contains code under different licenses it should be made
clear by providing a list of licenses:

    ;; Most of the code is released under BSD-3, except for code from
    ;; GDC (what is this?), which is released under GPLv2+, and the DMD
    ;; frontend, which is released under the “whatever” license.
    (license (list license:bsd-3
                   license:gpl2+
                   license:whatever-custom-is))

If there is no matching license value for “custom” you can use
“(license:non-copyleft uri)”, where “uri” is a string holding the URL
where the license can be read.

I think with these changes it’s okay.

~~ Ricardo