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