[PATCH] Add Elixir (was: )

[Ricardo Wurmus <rekado@elephly.net>]

Hi Pjotr

> From 5fd8f64794b27f59f6688177a7a9e532b5d57f01 Mon Sep 17 00:00:00 2001
> Date: Tue, 19 Jul 2016 11:13:27 +0000
> Subject: [PATCH] gnu: Add elixir.
> To: guix-devel@gnu.org
> From: Pjotr Prins <pjotr.public01@thebird.nl>
> References: <578e47d0.i8Ovns6KhzHqzVNC%pjotr.public12@thebird.nl>
>
> * gnu/packages/elixir.scm: New file.
> * gnu/local.mk (GNU_SYSTEM_MODULES): Add it.

back to the original patch, which I didn’t look at as the ensuing
discussion on review process and registry proposals took up more time
than I anticipated.

I’m a little uncertain on how to proceed.  I have a couple of things
here that I’d like to change before commiting.  I’ll add some comments
below.  Indentation changes won’t be mentioned ;)

If you are okay with the proposed changes I can apply them on top of
your patch and resubmit the squashed patch to the ML for a final
look-over.  Deal?

> +   (native-inputs
> +    `(("patch" ,patch)
> +      ("patch/elixir-disable-failing-tests"
> +       ,(search-patch "elixir-disable-failing-tests.patch"))
> +      ("patch/elixir-disable-mix-tests"
> +       ,(search-patch "elixir-disable-mix-tests.patch"))))

This has been mentioned already and I’d like to move these to the
“source” field after identifying the reason why the build fails
otherwise.  I see that you’re doing this in order to patch after the
build phase.  Let’s see if this can be avoided.

> +      (add-after 'build 'disable-breaking-elixir-tests
> +        ;; when patching tests as part of source the build breaks, so we do
> +        ;; it after the build phase
> +        (lambda* (#:key inputs #:allow-other-keys)
> +          (and
> +           (zero? (system* "patch" "--force" "-p1" "-i"
> +                           (assoc-ref inputs "patch/elixir-disable-failing-tests")))
> +           (zero? (system* "patch" "--force" "-p1" "-i"
> +                           (assoc-ref inputs "patch/elixir-disable-mix-tests")))
> +           ;; Tests currently fail in these two files:
> +           (delete-file "./lib/mix/test/mix/tasks/deps.git_test.exs")
> +           (delete-file "./lib/mix/test/mix/shell_test.exs"))))

“delete-file” has an unspecified return value, so chaining it up in
“and” isn’t guaranteed to work.  Should this patch-after-build business
turn out to be unavoidable I suggest just deleting the files first, then
and-ing the “zero?” expressions.

> +      (replace 'check
> +               (lambda _
> +                 (zero? (system* "make" "test"))))) ;; 3124 tests, 0 failures, 11 skipped

We can use “#:test-target "test"” instead of replacing the “check” phase.

> +      #:make-flags (list (string-append "PREFIX=" %output))))
> +   (home-page "http://elixir-lang.org/")
> +   (synopsis "The Elixir programming language")

s/The//

> +   (description "Elixir is a dynamic, functional language used to
> +build scalable and maintainable applications.  Elixir leverages the
> +Erlang VM, known for running low-latency, distributed and
> +fault-tolerant systems, while also being successfully used in web
> +development and the embedded software domain.")
> +   (license license:asl2.0)))

Looks good!

> diff --git a/gnu/packages/patches/elixir-disable-failing-tests.patch b/gnu/packages/patches/elixir-disable-failing-tests.patch
> new file mode 100644
> index 0000000..802cb1e
> --- /dev/null
> +++ b/gnu/packages/patches/elixir-disable-failing-tests.patch

While I’m generally okay with disabling failing tests (see the
embarassing situation we have with the “icedtea” packages), I think
these can be fixed with little effort.  Many of them seem to be related
to the location of the temp directory.

> +diff --git a/lib/elixir/test/elixir/node_test.exs b/lib/elixir/test/elixir/node_test.exs
> +index d1f1fe6..5c2d469 100644
> +--- a/lib/elixir/test/elixir/node_test.exs
> ++++ b/lib/elixir/test/elixir/node_test.exs
> +@@ -6,8 +6,10 @@ defmodule NodeTest do
> +   doctest Node
> + 
> +   test "start/3 and stop/0" do
> +-    assert Node.stop == {:error, :not_found}
> +-    assert {:ok, _} = Node.start(:hello, :shortnames, 15000)
> +-    assert Node.stop() == :ok
> ++    IO.puts "Skipping test because GNU Guix does not allow the HOME environment variable."
> ++
> ++    # assert Node.stop == {:error, :not_found}
> ++    # assert {:ok, _} = Node.start(:hello, :shortnames, 15000)
> ++    # assert Node.stop() == :ok
> +   end
> + end

This was already addressed earlier.  We can probably just setenv HOME
before the tests.

Some of the remaining tests don’t seem to have any obvious fixes, so
I’ll get to them after making the above changes first.  Maybe the
failures disappear then.

Thanks again for the patch.  I hope you are willing to provide some
guidance when I have some problems understanding certain bits of the
build.

~~ Ricardo


PS: Elixir is big and getting it accepted in Guix upstream is a
precondition for more Elixir packages.  This is why I think it’s worth
picking this up.  For other patches this amount of effort may not be
justified (as I cannot get other stuff done at the same time).  I do
hope that we can find a way to upstream the 140+ bioinfo packages you
have locally, even it it’s at a trickle rate :)