Re: RFC: Build system hacks for Guix do not belong in 'source'

[Mark H Weaver]

Ricardo Wurmus <address@hidden> writes:

> Mark H Weaver writes:
>
>> I think that both the 'ldconfig -> true' hack and the LIBDIR
>> substitution should be moved to a build phase for both of these
>> packages.
>
> Attached are updated patches, moving the build hacks from snippets to a
> build phase.
>
> ~~ Ricardo
>
> From abdbfec11164c61cfdb6fc88ddf9abd2e58aa027 Mon Sep 17 00:00:00 2001
> From: Ricardo Wurmus <address@hidden>
> Date: Wed, 4 Mar 2015 11:50:26 +0100
> Subject: [PATCH 1/2] gnu: zita-alsa-pcmi: set LIBDIR to "lib".
>
> * gnu/packages/audio.scm (zita-alsa-pcmi)[source, arguments]: Set LIBDIR to
>   "lib" in build phase.  Remove snippet.

I wouldn't write [source, arguments] here when the first sentence that
follows doesn't relate to 'source' at all.  Also, instead of "Remove
snippet", how about "Move snippet to build phase"?  Maybe something like
this:

--8<---------------cut here---------------start------------->8---
gnu: zita-alsa-pcmi: Move snippet to build phase and set LIBDIR.

* gnu/packages/audio.scm (zita-alsa-pcmi): Move snippet to build phase
  and set LIBDIR.
--8<---------------cut here---------------end--------------->8---

> @@ -1044,8 +1040,12 @@ interface.")
>         #:phases
>         (alist-cons-after
>          'unpack
> -        'enter-directory
> -        (lambda _ (chdir "libs"))
> +        'fix-makefile
> +        (lambda _
> +          (substitute* "libs/Makefile"
> +            (("ldconfig") "true")
> +            (("^LIBDIR =.*") "LIBDIR = lib\n"))
> +          (chdir "libs"))

It still enters the directory, so 'fix-makefile' doesn't quite cover it.
How about 'patch-Makefile-and-enter-directory' ?

Also, 'chdir' returns an unspecified value, but phases must return a
true value to indicate success.  So please add #t after the chdir,
maybe something like this:

--8<---------------cut here---------------start------------->8---
       (alist-cons-after
        'unpack 'patch-makefile-and-enter-directory
        (lambda _
          (substitute* "libs/Makefile"
            (("ldconfig") "true")
            (("^LIBDIR =.*") "LIBDIR = lib\n"))
          (chdir "libs")
          #t)
--8<---------------cut here---------------end--------------->8---

(It is true that in Guile, "an unspecified value" is normally a true
value, but it's bad practice to write code that depends on that).

All of these comments apply to the other patch as well.
After considering these suggestions, okay to push.

    Thanks!
      Mark