Re: [PATCH] gnu: openblas: Fix configure flags on armhf and take target

[Mark H Weaver]

Andreas Enge <address@hidden> writes:

> The attached patch fixes the build of openblas (on which 48 packages depend
> according to "guix refresh -l") on armhf.

Excellent!

> From 8802c118fed2c93ee60d90a5cc883c1c3328d531 Mon Sep 17 00:00:00 2001
> From: Andreas Enge <address@hidden>
> Date: Thu, 1 Oct 2015 10:32:29 +0200
> Subject: [PATCH] gnu: openblas: Fix configure flags on armhf and take target
>  system into   account for cross building.
>
> * gnu/packages/maths.scm (openblas)[arguments]: Make package substitutable
>   and enable runtime cpu detection only on x86 systems. Take the target
>   system into account when cross compiling.
> ---
>  gnu/packages/maths.scm | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/gnu/packages/maths.scm b/gnu/packages/maths.scm
> index 45fda14..d67f7fa 100644
> --- a/gnu/packages/maths.scm
> +++ b/gnu/packages/maths.scm
> @@ -1474,10 +1474,13 @@ constant parts of it.")
>      (build-system gnu-build-system)
>      (arguments
>       `(#:tests? #f  ;no "check" target
> -       ;; DYNAMIC_ARCH is not supported on MIPS.  When it is disabled,
> +       ;; DYNAMIC_ARCH is only supported on x86.  When it is disabled,
>         ;; OpenBLAS will tune itself to the build host, so we need to disable
>         ;; substitutions.
> -       #:substitutable? ,(not (string-prefix? "mips" (%current-system)))
> +       #:substitutable?
> +        ,(let ((system (or (%current-target-system) (%current-system))))
> +           (or (string-prefix? "x86_64" system)
> +               (string-prefix? "i686" system)))

It's not your fault, but the fact that (%current-system) returns a
"system" string (e.g. "mips64el-linux") but (%current-target-system)
returns a GNU triplet (e.g. "mips64el-unknown-linux-gnu") is very
confusing.  The fact that we use (or (%current-target-system)
(%current-system)) in many places, and name variables like the one
above "system" reinforces that confusion.

To mitigate this, how about renaming the variable above to
'system-or-triplet'?

Another possible issue is that our Hurd port seems to use "i586" in
several places.  I'm not sure if we have to check for that as well.
Maybe we need a helper procedure to check for Intel systems.

* * *

More importantly, it seems a shame for non-Intel users to have to
compile OpenBLAS and ~48 other packages from source code, especially
since it's not practical to compile large packages on many smaller armhf
systems.

Debian provides pre-compiled openblas binaries for armhf, so there must
be a way to disable tuning for the build host, and the answer must be in
the Debian packaging.  Any takers?

Anyway, your patch seems like a clear improvement for now, so I'd say go
ahead and push it, possibly with the suggested variable renaming.

     Thanks!
       Mark