Re: [PATCH] Add Elixir (was: )

[Pjotr Prins <pjotr.public12@thebird.nl>]

On Mon, Jul 25, 2016 at 08:13:33AM +0200, Ricardo Wurmus wrote:
> 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?

Sure. I have no ego in this. I am happy if you take over.

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

I tried and failed. Elixir people do not know either:

  https://github.com/elixir-lang/elixir/issues/5043

I think it is Mix magic. Probably tracking files in some way.

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

Cool.

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

Yes, I forgot.

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

Note we are talking a rather small minority of tests. 11 out of 2000+
for Elixir. For Mix 10% fails, mostly because of git. The Elixir
people wrote there should be no network access involved.

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

I am happy to help out if you take the lead.

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

Yes, very visible language and rapidly growing community.

Pj.

--