[bug#66801] [PATCH v3 01/14] build-system: Add mix-build-system.

[Liliana Marie Prikler]

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