Subject: Re: [PATCH v3 01/14] build-system: Add mix-build-system.

From: Liliana Marie Prikler <[email-masked]>
Date: Thu, 16 Nov 2023 16:11:50 +0100

Am Donnerstag, dem 16.11.2023 um 14:01 +0100 schrieb Pierre-Henry
Fröhring:
> Do you mind if I paste the conversation using the following org
> format?
I do mind you pasting it in HTML format :P

> * Comment
> ** lilyp
> > +(define (elixir-version elixir)
> > +  "Return an X.Y string where X and Y are respectively the major
> > and
> > minor version number of ELIXIR.
> > +Example: /gnu/store/…-elixir-1.14.0 → 1.14"
> > +  ((compose
> > +    (cut string-join <> ".")
> > +    (cut take <> 2)
> > +    (cut string-split <> #\.)
> > +    last)
> > +   (string-split elixir #\-)))
> 
> I don't think we need to be overly cute here.  The let-binding
> version from python-build-system is less surprising to the
> uninitiated reader.
> 
> See also strip-store-file-name and package-name->name+version.
> 
> 
> ** phf
> Maybe:
> #+begin_src scheme
> (define (elixir-version elixir)
>   "Return an X.Y string where X and Y are respectively the major and
> minor version number of ELIXIR.
> Example: /gnu/store/…-elixir-1.14.0 → 1.14"
>   (receive (_ version) (package-name->name+version (strip-store-file-
> name elixir))
>     (let* ((components  (string-split version #\.))
>            (major+minor (take components 2)))
>       (string-join major+minor "."))))
> #+end_src
> 
> or:
> #+begin_src scheme
> (define (elixir-version elixir)
>   "Return an X.Y string where X and Y are respectively the major and
> minor version number of ELIXIR.
> Example: /gnu/store/…-elixir-1.14.0 → 1.14"
>   (let* ((version     (last (string-split elixir #\-)))
>          (components  (string-split version #\.))
>          (major+minor (take components 2)))
>     (string-join major+minor ".")))
> #+end_src
> 
> or: just inline the code as it is used just once. See [[id:76abe0e4-
> a0e2-4176-bdc0-9ff241e8ba42][next comment]].
Note that you can use SRFI-71 let and let* to bind multiple values at
once.  So you can write the first one without receive.

> 
> * Comment
> :PROPERTIES:
> :ID:       76abe0e4-a0e2-4176-bdc0-9ff241e8ba42
> :END:
> 
> ** lilyp
> > +(define (elixir-libdir elixir path)
> > +  "Return the path where all libraries for a specified ELIXIR
> > version are installed."
> > +  (string-append path "/lib/elixir/" (elixir-version elixir)))
> 
> You probably want to swap path and elixir and perhaps also find a way
> to implicitly parameterize the latter so as to make it optional.
> 
> 
> ** phf
> Is this what you mean?
> #+begin_src scheme
> ;; The Elixir version is constant as soon as it is computable from
> the current
> ;; execution. It is a X.Y string where X and Y are respectively the
> major and
> ;; minor version number of the Elixir used in the build.
> (define elixir-version (make-parameter "X.Y"))
> 
> (define* (elixir-libdir path #:optional (version (elixir-version)))
>   "Return the path where all libraries for a specified ELIXIR version
> are installed."
>   (string-append path "/lib/elixir/" version))
> 
> (define* (configure #:key inputs mix-path mix-exs #:allow-other-keys)
>   …
>   (elixir-version
>    (receive (_ version) (package-name->name+version (strip-store-
> file-name (assoc-ref inputs "elixir")))
>      (let* ((components  (string-split version #\.))
>             (major+minor (take components 2)))
>        (string-join major+minor ".")))))
> #+end_src
I'd use %elixir-version and perhaps make it a fluent rather than a
parameter (not quite sure whether parameters get reset when a function
goes out of scope).

> * Comment
> ** lilyp
> > +(define* (configure #: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 "LC_ALL" "en_US.UTF-8")
> > +  (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))
> 
> This does not appear to be a configure phase in the traditional sense
> of the wording.  Instead, it should be a post 'set-paths' 'set-mix-
> env' imho.
> 
> 
> ** phf
> After ~install-locale~ since ~(setenv "LC_ALL" "en_US.UTF-8")~ is
> called in this phase.
> #+begin_src scheme
> (define %standard-phases
>   (modify-phases gnu:%standard-phases
>     …
>     (delete 'configure)
>     (add-after 'install-locale 'set-mix-env set-mix-env)
>     (replace 'unpack unpack)
>     …))
> #+end_src
Good point: you shouldn't be setting LC_ALL anyway, that's already done
by install-locale.

> * Comment
> ** lilyp
> > +(define* (install-hex #:key name inputs outputs #:allow-other-
> > keys)
> > +  "Install Hex."
> > +  (let ((hex-archive-path (string-append (getenv "MIX_ARCHIVES")
> > "/hex")))
> > +    (mkdir-p hex-archive-path)
> > +    (symlink (car (list-directories (elixir-libdir (assoc-ref
> > inputs
> > "elixir")
> > +                                                   (assoc-ref
> > inputs
> > "elixir-hex"))))
> > +             (string-append hex-archive-path "/hex"))))
> 
> Why do we need this?  It looks like we'll be pasting the same
> (native?) input all over the store, which imho would be bad.
> 
> 
> ** phf
> Without ~Hex~, you get this error:
> #+begin_example
> starting phase `build'
> Could not find Hex, which is needed to build dependency :ex_doc
> Shall I install Hex? (if running non-interactively, use "mix
> local.hex --force") [Yn]
> #+end_example
> This message is called from ~handle_rebar_not_found~ in
> ~lib/mix/lib/mix/tasks/deps.compile.ex~ ; which is called from
> ~rebar_cmd~ because
> a ~manager~ (~Hex~) could not be found. Hex must be present =>
> install-hex must be executed.
> 
> I thought that this should not be a problem since Hex is needed at
> build time, and just symlinked.  Maybe should it be copied instead?
Is hex not an (implicit) native-input in your build system?  Anything
that keeps it from functioning is a packaging bug imho.

> * Comment
> ** lilyp
> > +  (define (install-input mix-env input)
> > +    (let ((dir (mix-build-dir mix-env)))
> > +      (mkdir-p dir)
> > +      (match input
> > +        ((_ . path)
> > +         ((compose
> > +           (cut for-each (cut install-lib <> dir) <>)
> > +           (cut append-map list-directories <>)
> > +           (cut filter directory-exists? <>))
> > +          (list (elixir-libdir (assoc-ref inputs "elixir") path)
> > +                (erlang-libdir path)))))))
> 
> I think you're at the wrong layer of abstraction here.
> 
>   (match input
>     ((_ . prefix)
>      (begin
>       (install-subdirectories (elixir-libdir path))
>       (install-subdirectories (erlang-libdir path)))))
> 
> where (install-subdirectories PATH) is basically
>   (when (directory-exists? PATH)
>     (for-each (cute install-lib <> (mix-build-dir mix-env))
>               (list-directories PATH)))
> 
> 
> ** phf
> Does this work?
> #+begin_src scheme
> (define (install-lib lib dir)
>     (let ((lib-name (last (string-split lib #\/))))
>       (symlink lib (string-append dir "/" lib-name))))
> 
>   (define (install-subdirectories mix-env path)
>     (let ((build-dir (mix-build-dir mix-env)))
>       (mkdir-p build-dir)
>       (when (directory-exists? path)
>         (for-each (cute install-lib <> build-dir)
>                   (list-directories path)))))
Maybe move the mkdir-p into the when or at the start of install-lib.

>   (define (install-input mix-env input)
>     (match input
>       ((_ . path)
>        (begin
>          (install-subdirectories mix-env (elixir-libdir path))
>          (install-subdirectories mix-env (erlang-libdir path))))))
> #+end_src
> 
> 
> * Comment
> ** lilyp
> > +  (define (install-inputs mix-env)
> > +    (for-each (cut install-input mix-env <>)
> > +              (append inputs native-inputs)))
> 
> Installing native inputs: probably a bad idea (inhibits cross-
> compilation).
> 
> 
> ** phf
> A slight variant that does not link things when it should not:
> #+begin_src scheme
> (define (install-inputs mix-env)
>     (for-each (cut install-input mix-env <>)
>               (cond
>                 ((string=? mix-env "prod") inputs)
>                 ((member mix-env '("shared" "test")) (append inputs
> native-inputs))
>                 (else (error (format #f "Unexpected Mix env: ~a~%"
> mix-env))))))
> #+end_src
> 
> I did not consider cross-compilation yet. The following might be
> wrong be here we go. I far as I understand, Erlang applications are
> largely platform independent. Once the code is compiled to BEAM
> bytecode, it can run on any platform that has the Erlang VM
> installed.  If an Erlang library uses native extensions, then cross-
> compilation might be required.  For a build to succeed
> in a given environment (one of "prod", "test", "shared"), the BEAM
> files of all dependencies should be present on the build machine. So,
> all dependencies must be installed
Not an expert on elixir, but that sounds borked.  You might get around
this with propagated-inputs maybe?  That being said, native-inputs
shouldn't blow up a build if missing.

> * Comment
> ** lilyp
> > +(define (library-name pkg-name)
> > +  "Return the library name deduced from PKG-NAME.
> > +A package should be named: elixir-lib-name-X.Y.Z from which the
> > library name
> > +lib_name is deduced."
> > +  ((compose
> > +    (cut string-join <> "_")
> > +    (cut drop-right <> 1)
> > +    (cut string-split <> #\-))
> > +   (strip-elixir-prefix pkg-name)))
> 
> Consider defining (package-name-version->elixir-name) analogous to
> (package-name-version->erlang-name) in rebar-build-system.  Also
> allow overriding it through a build system argument.  The thing you
> currently have will blow up with git-version.
> 
> 
> ** phf
> Faily close to the ~rebar-build-system~ version.
> #+begin_src scheme
> (define (package-name-version->elixir-name name+ver)
>   "Convert the Guix package NAME-VER to the corresponding Elixir
> name-version format.  Essentially drop the prefix used in Guix and
> replace dashes by underscores."
>   (let* ((name- (package-name->name+version name+ver)))
>     (string-join
>      (string-split
>       (if (string-prefix? "elixir-" name-)
>           (string-drop name- (string-length "elixir-"))
>           name-)
>       #\-)
>      "_")))
> #+end_src
Looks okay.