Re: [PATCH] gnu: Add ldc-1.1.0-beta6

[Ludovic Courtès]

Hello!

Sorry for the late reply!

Frederick Muriithi <address@hidden> skribis:

> Based on thread
> https://lists.gnu.org/archive/html/guix-devel/2017-01/msg00465.html
>
> - Fixed some issues raised by Danny Milosavljevic on thread above
> - Updated the beta version to 6, up from 4
> - Started up a thread on ldc forum to have a version flag added to
> deactivate network tests when building ldc
> https://forum.dlang.org/post/address@hidden

Nice!

> From a0185dc1f5881bae8094643251335b04560900a0 Mon Sep 17 00:00:00 2001
> From: Muriithi Frederick Muriuki <address@hidden>
> Date: Fri, 6 Jan 2017 17:51:18 +0300
> Subject: [PATCH] gnu: Add ldc-1.1.0-beta6
>
> * gnu/packages/ldc.scm (ldc-1.1.0-beta6): New variable
> * gnu/packages/ldc.scm (ldc-beta): New variable
> * gnu/packages/patches/ldc1.1.0-disable-dmd-tests.patch: New patch
> * gnu/packages/patches/ldc1.1.0-disable-phobos-tests.patch: New patch

Overall the patch looks good to me.

One question: We usually avoid packaging software that has no release or
has an “alpha” or “beta” label.  Do you think we could wait for 1.1.0 to
be officially released?  Or are there good reasons why we should not
wait?

Some minor issues:

> +(define-public ldc-1.1.0-beta6
> +  (let ((older-version "1.1.0-beta4"))

[...]

> +       ("phobos-src"
> +        ,(origin
> +           (method url-fetch)
> +           (uri (string-append
> +                 "https://github.com/ldc-developers/phobos/archive/ldc-v";
> +                 older-version ".tar.gz"))
> +           (sha256
> +            (base32
> +             "1iwy5rs0rqkicj1zfsa5yqvk8ard99bfr8g69qmhlbzb98q0kpks"))
> +           (patches (search-patches "ldc1.1.0-disable-phobos-tests.patch"))))
> +       ("druntime-src"
> +        ,(origin
> +           (method url-fetch)
> +           (uri (string-append
> +                 "https://github.com/ldc-developers/druntime/archive/ldc-v";
> +                 older-version ".tar.gz"))
> +           (sha256
> +            (base32
> +             "1qsiw5lz1pr8ms9myjf8b94nqi7f1781k226jvxwnhkjg11d0s63"))))
> +       ("dmd-testsuite-src"
> +        ,(origin
> +           (method url-fetch)
> +           (uri (string-append
> +                 
> "https://github.com/ldc-developers/dmd-testsuite/archive/ldc-v";
> +                 older-version ".tar.gz"))
> +           (sha256
> +            (base32
> +             "0jp54hyi75i9g41rvgmm3zg21yzv57q8dghrhb432rb0n9j15mbp"))
> +           ;; Deactivate some failing gdb tests. Most other gdb tests pass
> +           (patches (search-patches 
> "ldc1.1.0-disable-dmd-tests.patch")))))))))

Could you add a comment explaining why the previous version of these is
needed, instead of the current version?

> --- /dev/null
> +++ b/gnu/packages/patches/ldc1.1.0-disable-dmd-tests.patch

Could you add a line or two explaining at the top of patch explaining
what it does and why, and what its upstream status is?

For example, I think this one disables a test that would require GDB,
which is not an input (?), and presumably it won’t be submitted
upstream.

> @@ -0,0 +1,28 @@
> +diff --git a/d_do_test.d b/d_do_test.d
> +index aa67169..7a4dcc1 100755
> +--- a/d_do_test.d
> ++++ b/d_do_test.d
> +@@ -645,8 +645,8 @@ int main(string[] args)
> +                     auto gdb_output = execute(fThisRun, command, true, 
> result_path);
> +                     if (testArgs.gdbMatch !is null)
> +                     {
> +-                        enforce(match(gdb_output, regex(testArgs.gdbMatch)),
> +-                                "\nGDB regex: '"~testArgs.gdbMatch~"' 
> didn't match output:\n----\n"~gdb_output~"\n----\n");
> ++                        //enforce(match(gdb_output, 
> regex(testArgs.gdbMatch)),
> ++                                //"\nGDB regex: '"~testArgs.gdbMatch~"' 
> didn't match output:\n----\n"~gdb_output~"\n----\n");

I think it’s better to just delete the two lines instead of commenting
them out: that makes the patch easier to read.

Hope this makes sense.

Thanks for your contribution!

Ludo’.