GNU bug report logs - #66801
[PATCH] mix-build-system: draft 1

Previous Next

Full log

Message #221 received at 66801 <at> debbugs.gnu.org (full text, mbox):

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: Pierre-Henry Fröhring <phfrohring <at> deeplinks.com>
Cc: 66801 <at> debbugs.gnu.org
Subject: Re: [PATCH v3 01/14] build-system: Add mix-build-system.
Date: Sat, 18 Nov 2023 08:12:14 +0100

Am Samstag, dem 18.11.2023 um 05:44 +0100 schrieb Pierre-Henry
Fröhring:
> * WAITING Comment
> ** lilyp
> > * guix/build-system/mix.scm,
> > * guix/build/mix-build-system.scm: New modules.
> Avoid spanning a change across multiple files.  Use
> file: Change.
> file2: Likewise.
> 
> >  gnu/packages/elixir.scm         |  62 ++++++++++-
> >  guix/build-system/mix.scm       | 187
> > ++++++++++++++++++++++++++++++++
> >  guix/build/mix-build-system.scm | 171
> > +++++++++++++++++++++++++++++
> You committed two changes at once here.  Split them.
> 
> 
> ** phf
> How are changes counted? I've counted one because all of these
> changes
> are needed
> to introduce the ~mix-build-system~. Should it be:
> #+begin_example
> ,* gnu/packages/elixir.scm (elixir): Search path GUIX_ELIXIR_LIBS
> added.
> ,* gnu/packages/elixir.scm (elixir): Wrap programs with
> ERL_LIBS=${GUIX_ELIXIR_LIBS}.
> ,* guix/build-system/mix.scm: New modules.
> ,* guix/build/mix-build-system.scm: New modules.
> #+end_example
> or something else?
You can add the two build-system files in one go.  The changes to
elixir and the new elixir-hex package are one patch each.

> * WAITING Comment
> ** lilyp
> > +(define* (set-mix-env #:key inputs mix-path mix-exs #:allow-other-
> > keys)
> > +  "Set environment variables.
> > +See:
> > https://hexdocs.pm/mix/1.15.7/Mix.html#module-environment-variables
> > "
> > +  (setenv "MIX_HOME" (getcwd))
> > +  (setenv "MIX_ARCHIVES" "archives")
> > +  (setenv "MIX_BUILD_ROOT" "_build")
> > +  (setenv "MIX_DEPS_PATH" "deps")
> > +  (setenv "MIX_PATH" (or mix-path ""))
> > +  (setenv "MIX_REBAR3" (string-append (assoc-ref inputs "rebar3")
> > "/bin/rebar3"))
> > +  (setenv "MIX_EXS" mix-exs)
> > +  (%elixir-version (elixir-version inputs)))
> %elixir-version is not an environment variable.  You should set this
> up
> separately or at the very least add a big fat comment explaining what
> you're doing here :)
> 
> 
> ** phf
> What about both?
> #+begin_src scheme
> (define* (set-elixir-version #:key inputs #:allow-other-keys)
>   "Set Elixir version.
> Compiled libraries are specific to each Elixir version. If a library
> is intended
> to be used with multiple Elixir versions, it needs to be compiled
> separately for
> each version. The parameter %elixir-version, set for the current
> build, is used
> to differentiate between the compiled versions of libraries
> corresponding to
> different Elixir versions."
>   (%elixir-version (elixir-version inputs))
>   (format #t "Elixir version: ~a~%" (%elixir-version)))
> #+end_src
I just noticed that, but do prefer to spaces after sentence ending
periods.  For the docstring, I think you should make clear what is
going on from the beginning, e.g. 
"Store the version number of the elixir input in a parameter." 

> * WAITING Comment
> ** lilyp
> > +     (list (search-path-specification
> > +            (variable "GUIX_ERL_LIBS")
> > +            (files (list "lib/erlang/lib"
> > +                         (string-append "lib/elixir/" (version-
> > major+minor version)))))))
> I suppose ERL is for erlang?  What do we use for elixir then?
> 
> 
> ** phf
> Changed for ~GUIX_ELIXIR_LIBS~. Is that OK?
No, because it's still ERL on the other side.  A quick web search
reveals that this belongs to erlang. 

> 
> * WAITING Comment
> ** lilyp
> > +(define* (install-dependencies . _)
> > +  "Install dependencies."
> > +  (setenv "ERL_LIBS" (getenv "GUIX_ERL_LIBS")))
> Why not do this as part of setting up mix-env?
> 
> 
> ** phf
> If we have this phase in the ~elixir~ package:
> #+begin_src scheme
> (add-after 'install 'wrap-programs
>             (lambda* (#:key inputs outputs #:allow-other-keys)
>               (let* ((out (assoc-ref outputs "out"))
>                      (programs '("elixir" "elixirc" "iex" "mix")))
>                 (for-each (lambda (program)
>                             (wrap-program (string-append out "/bin/"
> program)
>                               '("ERL_LIBS" prefix
> ("${GUIX_ELIXIR_LIBS}"))))
>                           programs))))
> #+end_src
> 
> and this native search path:
> #+begin_src scheme
> (search-path-specification
>             (variable "GUIX_ELIXIR_LIBS")
>             (files (list "lib/erlang/lib"
>                          (string-append "lib/elixir/"
> (version-major+minor version)))))
> #+end_src
> 
> then, ~(setenv "ERL_LIBS" (getenv "GUIX_ERL_LIBS"))~ is not needed
> anymore.
True, but it's still given in the source files. :)
So you can delete it of course (if it's already done by the native-
search-path and package as you claim), or you can make it part of the
environment setup (if it's not).

Cheers

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.