Re: [PATCH] Add LDC.

[Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de>]

Hi Roel,

Roel Janssen <roel@gnu.org> writes:

> Here is a patch to add the LLVM-based D compiler.  The developers split
> the source code in "submodules".  Since these submodules are specific to
> LDC, they are described in a single package.

I think that’s okay.

> I tried to conform to all style/syntax "rules", so please let me know
> when some formatting is wrong.

thanks for the patch!  Below is a style critique with additional
ramblings.

> From 02ac3c5d71e16dc0851018e04aec817e86c8597c Mon Sep 17 00:00:00 2001
> From: Roel Janssen <roel@gnu.org>
> Date: Tue, 29 Dec 2015 12:06:25 +0100
> Subject: [PATCH] gnu: add LDC.

Please capitalise “add”.

> * gnu/packages/dlanguage.scm (ldc): New variable.
> * gnu/packages/patches/ldc-disable-tests.patch: New file.
> * gnu-system.am (GNU_SYSTEM_MODULES): Add (gnu packages dlanguage).
> * gnu-system.am (dist_patch_DATA): Add patch file.

Actually, since “dlanguage.scm” is a new file it should be something
like this:

  * gnu/packages/dlanguage.scm: New file.
  * gnu/packages/patches/ldc-disable-tests.patch: New file.
  * gnu-system.am (GNU_SYSTEM_MODULES): Add dlanguage.scm.
  (dist_patch_DATA): Add patch file.

Why “dlanguage.scm” and not just “d.scm”?

> @@ -175,6 +175,7 @@ GNU_SYSTEM_MODULES =				\
>    gnu/packages/key-mon.scm			\
>    gnu/packages/kodi.scm				\
>    gnu/packages/language.scm			\
> +  gnu/packages/dlanguage.scm			\
>    gnu/packages/less.scm				\
>    gnu/packages/lesstif.scm			\
>    gnu/packages/libcanberra.scm			\

Could you please place this in alphabetic order?

> +(define-module (gnu packages dlanguage)
> +  #:use-module ((guix licenses) #:prefix license:)
> +  #:use-module (gnu packages)
> +  #:use-module (guix packages)
> +  #:use-module (guix download)
> +  #:use-module (guix build-system cmake)
> +  #:use-module (gnu packages textutils)
> +  #:use-module (gnu packages base)
> +  #:use-module (gnu packages libedit)
> +  #:use-module (gnu packages llvm)
> +  #:use-module (gnu packages zip)
> +  #:use-module (guix git-download))

It would be nice if (guix git-download) were up there with the other
(guix ...) imports.

> +
> +(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"))

Could you add a comment here?  Does upstream say that only these two
systems are supported?

> +    (arguments
> +     `(#:tests? #t

This is the default, so it’s not needed.

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

We usually align the “(lambda” with the first “d” in “add-after”.  Why
is “source” among the keys when you don’t use it?  Why copy the file
from the inputs to some other place rather than using (assoc-ref inputs
...) as the argument to “tar”?  (I may have done the same in the icedtea
packages—this probably could also be changed.)

> +         (add-after 'unpack 'unpack-druntime-source
[...]
> +         (add-after 'unpack 'unpack-dmd-testsuite-source

I think all these three phases could be merged into one appropriately
named phase.

> +         (add-after
> +          'unpack-phobos-source 'patch-phobos
> +          (lambda* (#:key source inputs #:allow-other-keys)
> +            (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")))

Please put the “(string-append” on a new line.

> +            #t))
> +         (add-after
> +          'unpack-dmd-testsuite-source 'patch-dmd-testsuite
> +          (lambda _
> +            (substitute* "tests/d2/dmd-testsuite/Makefile"
> +              (("/bin/bash") (which "bash")))
> +            #t)))))
> +    (inputs
> +     `(("libconfig" ,libconfig)
> +       ("libedit" ,libedit)
> +       ("tzdata" ,tzdata)))  ;; needed for tests

If it’s needed for tests shouldn’t it be among the native-inputs then?

> +    (native-inputs
> +     `(("llvm" ,llvm)

The home page says that the compiler “relies on the LLVM Core libraries
for code generation”.  Doesn’t this mean that llvm should be a regular
input?

> +       ("clang" ,clang)
> +       ("unzip" ,unzip) ;; needed for tests
> +       ("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")))))

Why is this patch needed?  Can they not be disabled elsewhere?

> +       ("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 "https://github.com/ldc-developers/ldc")

Is this really the project home page?  Or is it
“http://wiki.dlang.org/LDC”?

> +    (synopsis "LLVM compiler for the D programming language")

Must “LLVM” be part of the synopsis?  I’d think of this as just a
compiler, not an “LLVM compiler”.

> +    (description
> +     "LDC is a compiler for the D programming Language.  It is based on the
> +latest DMD frontend and uses LLVM as backend.  LLVM provides a fast and modern
> +backend for high quality code generation.  LDC is released under a BSD license
> +with exceptions for the DMD frontend and code from GDC.  The development takes
> +place mostly on x86-32 and x86-64 Linux and that is where LDC works best.")

“D programming Language” —> “D programming language”.
Please remove the third sentence (“LLVM provides...”) because it doesn’t
really describe LDC.  Also the next sentence (“LDC is released
under...”) should not be part of the description (that’s what the
“license” field is there for).

Also the last sentence probably isn’t a good fit for the description.
It would make a good comment for the “supported-systems” field, though.

> +    (license license:bsd-3)))

Please also mention in a comment the exceptions alluded to in the
description.

> diff --git a/gnu/packages/patches/ldc-disable-tests.patch b/gnu/packages/patches/ldc-disable-tests.patch
> new file mode 100644
> index 0000000..42e394d
> --- /dev/null
> +++ b/gnu/packages/patches/ldc-disable-tests.patch
> @@ -0,0 +1,90 @@
> +This patch fixes a failing unit test by feeding buildNormalizedPath to the
> +tzdata properly. Three other tests are disables, one assumes /root and the
> +two others use networking. Not bad out of almost 700 tests!

Please use two spaces between sentences.
s/disables/disabled/.

Is there no other way to disable tests, e.g. by name or by passing some
kind of variable to the build system?

~~ Ricardo