From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pjotr Prins Subject: Re: [PATCH] Add LDC. Date: Tue, 29 Dec 2015 16:37:05 +0100 Message-ID: <20151229153705.GB9031@thebird.nl> 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]:40978) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aDwKv-0002Xu-U2 for guix-devel@gnu.org; Tue, 29 Dec 2015 10:37:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aDwKq-00052t-UQ for guix-devel@gnu.org; Tue, 29 Dec 2015 10:37:21 -0500 Content-Disposition: inline In-Reply-To: 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: Ricardo Wurmus Cc: guix-devel@gnu.org Just a few comments from my end: On Tue, Dec 29, 2015 at 04:02:00PM +0100, Ricardo Wurmus wrote: > Why =E2=80=9Cdlanguage.scm=E2=80=9D and not just =E2=80=9Cd.scm=E2=80=9D= ? We could do. But in general it is referred to as the D language. I don't know why. > > + (supported-systems '("x86_64-linux" "i686-linux")) >=20 > Could you add a comment here? Does upstream say that only these two > systems are supported? Other targets are untested - the authors say. I think someone is working on ARM - there is evidence of Android in the tests. > > + (add-after 'unpack 'unpack-druntime-source > [...] > > + (add-after 'unpack 'unpack-dmd-testsuite-source >=20 > I think all these three phases could be merged into one appropriately > named phase. The order matters. Something to keep in mind. > > + ("tzdata" ,tzdata))) ;; needed for tests >=20 > If it=E2=80=99s needed for tests shouldn=E2=80=99t it be among the nati= ve-inputs then? The comment is wrong, sorry (it is mine). > > + (native-inputs > > + `(("llvm" ,llvm) >=20 > The home page says that the compiler =E2=80=9Crelies on the LLVM Core l= ibraries > for code generation=E2=80=9D. Doesn=E2=80=99t this mean that llvm shou= ld be a regular > input? Perhaps, but the compiler works fine. > > + (patches (list (search-patch "ldc-disable-tests.patch"))))= ) >=20 > Why is this patch needed? Can they not be disabled elsewhere? Unfortunately not. D compiles with all unit tests. Also we don't want to disable the other tests in the files by removing them from the Makefile. Only 4 tests out of almost 700 are patched out. I think it is amazingly good. Actually 1 test works after this patch. We'll send it upstream. > > + (synopsis "LLVM compiler for the D programming language") >=20 > Must =E2=80=9CLLVM=E2=80=9D be part of the synopsis? I=E2=80=99d think= of this as just a > compiler, not an =E2=80=9CLLVM compiler=E2=80=9D. Yes, there are 3 different D compilers. This one is specific for LLVM. > Is there no other way to disable tests, e.g. by name or by passing some > kind of variable to the build system? No, there is no easy way that I am aware. Actually we spent quite some time getting the tests to pass. The patch is simple and easy to maintain. I am very excited to get a working D compiler in GNU Guix! This is a nice bit of team work. Pj.