Re: [PATCH] Add LDC.

[Roel Janssen]

0001-gnu-Add-LDC.patch
Description: Text Data

Hello Ricardo,

Sorry for the late reply.  I hope I followed the style rules better this
time.  Could you take another look?

Ricardo Wurmus writes:

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

Done.  (I saw both variants in the commit log, and opted for the most
recent message at that time).

>> * 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”?

I renamed it to ldc.scm, following the example of GCC and LLVM compilers
for C. 

>> @@ -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?

I'm really sorry.  I should've catched this.  It's fixed now.

>> +(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.

I removed (guix git-download) since we don't use git-download
anymore. Sorry, this shouldn't have been in there. Fixed now.

>> +
>> +(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?

As Pjotr said, other platforms are not supported (yet) by the developers.

>> +    (arguments
>> +     `(#:tests? #t
>
> This is the default, so it’s not needed.

I removed it from the patch.

>> +       #: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?

Yes, that looks better and leaves some more space on the lines.  I
removed "source" from the keys.

> 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.)

Good question..  I applied your suggestion.

>> +         (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.

I suppose they could.  But I'm not sure why this would be
benificial.  What's the benefit of merging them into one phase?  I like
the separation..

>> +         (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.

Done.

>> +            #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?

As Pjotr said.  The comment was wrong, so I removed the comment.

>> +    (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?

I checked the output binaries and they don't link to llvm.  This is
statically compiled into to executable.  It therefore doesn't need llvm
as 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?

See Pjotr's comment.  Unfortunately they cannot.

>> +       ("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”?

You're right, it is http://wiki.dlang.org/LDC.  Fixed now.

>> +    (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”.

There are three compilers for D.  One is a reference compiler (DMD), one
is GDC which is based on GCC, and one is LDC which is based on LLVM.

So to be clear about which of the three compilers it is, I think "LLVM"
should be part of the synopsis. 

>> +    (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.

I added a comment to "supported-systems" and to "license".

>> +    (license license:bsd-3)))
>
> Please also mention in a comment the exceptions alluded to in the
> description.

Done.

>> 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?

I fixed the patch.

Not that I am aware of.  You could add the "(arguments `(#:tests? #f))"
to the package definition.

Thanks again for your time,
Roel Janssen