From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ricardo Wurmus Subject: Re: [PATCH] Add LDC. Date: Tue, 29 Dec 2015 16:02:00 +0100 Message-ID: References: <87ziwtec5x.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:60132) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aDvmy-0000TY-9U for guix-devel@gnu.org; Tue, 29 Dec 2015 10:02:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aDvmr-0005Gm-Sw for guix-devel@gnu.org; Tue, 29 Dec 2015 10:02:16 -0500 In-Reply-To: <87ziwtec5x.fsf@gnu.org> List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: Roel Janssen Cc: guix-devel@gnu.org Hi Roel, Roel Janssen 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 t= o > LDC, they are described in a single package. I think that=E2=80=99s 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 > Date: Tue, 29 Dec 2015 12:06:25 +0100 > Subject: [PATCH] gnu: add LDC. Please capitalise =E2=80=9Cadd=E2=80=9D. > * 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 =E2=80=9Cdlanguage.scm=E2=80=9D is a new file it should b= e 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 =E2=80=9Cdlanguage.scm=E2=80=9D and not just =E2=80=9Cd.scm=E2=80=9D? > @@ -175,6 +175,7 @@ GNU_SYSTEM_MODULES =3D \ > 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=E2=80=99s 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=3D1"))))) We usually align the =E2=80=9C(lambda=E2=80=9D with the first =E2=80=9Cd=E2= =80=9D in =E2=80=9Cadd-after=E2=80=9D. Why is =E2=80=9Csource=E2=80=9D among the keys when you don=E2=80=99t use it?= Why copy the file from the inputs to some other place rather than using (assoc-ref inputs ...) as the argument to =E2=80=9Ctar=E2=80=9D? (I may have done the same= in the icedtea packages=E2=80=94this 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 =E2=80=9C(string-append=E2=80=9D 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=E2=80=99s needed for tests shouldn=E2=80=99t it be among the native= -inputs then? > + (native-inputs > + `(("llvm" ,llvm) The home page says that the compiler =E2=80=9Crelies on the LLVM Core lib= raries for code generation=E2=80=9D. Doesn=E2=80=99t 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/ld= c-v" > + version ".tar.gz")) > + (sha256 > + (base32 > + "0z4mkyddx6c4sy1vqgqvavz55083dsxws681qkh93jh1rpby9yg6")))) > + ("dmd-testsuite-src" > + ,(origin > + (method url-fetch) > + (uri (string-append > + "https://github.com/ldc-developers/dmd-testsuite/archi= ve/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 =E2=80=9Chttp://wiki.dlang.org/LDC=E2=80=9D? > + (synopsis "LLVM compiler for the D programming language") Must =E2=80=9CLLVM=E2=80=9D be part of the synopsis? I=E2=80=99d think o= f this as just a compiler, not an =E2=80=9CLLVM compiler=E2=80=9D. > + (description > + "LDC is a compiler for the D programming Language. It is based o= n the > +latest DMD frontend and uses LLVM as backend. LLVM provides a fast an= d 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 developme= nt takes > +place mostly on x86-32 and x86-64 Linux and that is where LDC works be= st.") =E2=80=9CD programming Language=E2=80=9D =E2=80=94> =E2=80=9CD programmin= g language=E2=80=9D. Please remove the third sentence (=E2=80=9CLLVM provides...=E2=80=9D) bec= ause it doesn=E2=80=99t really describe LDC. Also the next sentence (=E2=80=9CLDC is released under...=E2=80=9D) should not be part of the description (that=E2=80=99s = what the =E2=80=9Clicense=E2=80=9D field is there for). Also the last sentence probably isn=E2=80=99t a good fit for the descript= ion. It would make a good comment for the =E2=80=9Csupported-systems=E2=80=9D = 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/package= s/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