* [PATCH] gnu: openblas: Fix configure flags on armhf and take target
@ 2015-10-01 8:46 Andreas Enge
2015-10-01 13:25 ` Ludovic Courtès
2015-10-01 16:59 ` Mark H Weaver
0 siblings, 2 replies; 7+ messages in thread
From: Andreas Enge @ 2015-10-01 8:46 UTC (permalink / raw
To: guix-devel
[-- Attachment #1: Type: text/plain, Size: 293 bytes --]
The attached patch fixes the build of openblas (on which 48 packages depend
according to "guix refresh -l") on armhf. Quick comments would be welcome.
In particular, did I correctly handle the target system for cross building?
My impression was that the previous code had a problem.
Andreas
[-- Attachment #2: 0001-gnu-openblas-Fix-configure-flags-on-armhf-and-take-t.patch --]
[-- Type: text/plain, Size: 2462 bytes --]
From 8802c118fed2c93ee60d90a5cc883c1c3328d531 Mon Sep 17 00:00:00 2001
From: Andreas Enge <privat@xobs-novena>
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)))
#:make-flags
(list (string-append "PREFIX=" (assoc-ref %outputs "out"))
"SHELL=bash"
@@ -1485,10 +1488,13 @@ constant parts of it.")
;; Build the library for all supported CPUs. This allows
;; switching CPU targets at runtime with the environment variable
;; OPENBLAS_CORETYPE=<type>, where "type" is a supported CPU type.
- ;; Unfortunately, this is not supported on MIPS.
- ,@(if (string-prefix? "mips" (%current-system))
- '()
- '("DYNAMIC_ARCH=1")))
+ ;; Unfortunately, this is not supported on non-x86 architectures,
+ ;; where it leads to failed builds.
+ ,@(let ((system (or (%current-target-system) (%current-system))))
+ (if (or (string-prefix? "x86_64" system)
+ (string-prefix? "i686" system))
+ '("DYNAMIC_ARCH=1")
+ '())))
;; no configure script
#:phases (alist-delete 'configure %standard-phases)))
(inputs
--
2.5.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] gnu: openblas: Fix configure flags on armhf and take target
2015-10-01 8:46 [PATCH] gnu: openblas: Fix configure flags on armhf and take target Andreas Enge
@ 2015-10-01 13:25 ` Ludovic Courtès
2015-10-01 17:36 ` Andreas Enge
2015-10-01 16:59 ` Mark H Weaver
1 sibling, 1 reply; 7+ messages in thread
From: Ludovic Courtès @ 2015-10-01 13:25 UTC (permalink / raw
To: Andreas Enge; +Cc: guix-devel
Andreas Enge <andreas@enge.fr> skribis:
>From 8802c118fed2c93ee60d90a5cc883c1c3328d531 Mon Sep 17 00:00:00 2001
> From: Andreas Enge <privat@xobs-novena>
> 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.
[...]
> + ,@(let ((system (or (%current-target-system) (%current-system))))
> + (if (or (string-prefix? "x86_64" system)
^
Please indent like the previous ‘let’.
Other than that LGTM, thank you!
Ludo’.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] gnu: openblas: Fix configure flags on armhf and take target
2015-10-01 8:46 [PATCH] gnu: openblas: Fix configure flags on armhf and take target Andreas Enge
2015-10-01 13:25 ` Ludovic Courtès
@ 2015-10-01 16:59 ` Mark H Weaver
2015-10-01 20:50 ` Ludovic Courtès
` (2 more replies)
1 sibling, 3 replies; 7+ messages in thread
From: Mark H Weaver @ 2015-10-01 16:59 UTC (permalink / raw
To: Andreas Enge; +Cc: guix-devel
Andreas Enge <andreas@enge.fr> 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 <privat@xobs-novena>
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] gnu: openblas: Fix configure flags on armhf and take target
2015-10-01 13:25 ` Ludovic Courtès
@ 2015-10-01 17:36 ` Andreas Enge
0 siblings, 0 replies; 7+ messages in thread
From: Andreas Enge @ 2015-10-01 17:36 UTC (permalink / raw
To: Ludovic Courtès; +Cc: guix-devel
On Thu, Oct 01, 2015 at 03:25:30PM +0200, Ludovic Courtès wrote:
> Please indent like the previous ‘let’.
Ah, and I was so proud I got the first "let" correctly! :-)
Thanks for the review, I just pushed.
Andreas
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] gnu: openblas: Fix configure flags on armhf and take target
2015-10-01 16:59 ` Mark H Weaver
@ 2015-10-01 20:50 ` Ludovic Courtès
2015-10-02 18:01 ` Andreas Enge
2015-10-13 6:20 ` Ricardo Wurmus
2 siblings, 0 replies; 7+ messages in thread
From: Ludovic Courtès @ 2015-10-01 20:50 UTC (permalink / raw
To: Mark H Weaver; +Cc: guix-devel
Mark H Weaver <mhw@netris.org> skribis:
> Andreas Enge <andreas@enge.fr> writes:
[...]
>> + ,(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'?
I think for a local variable keeping ‘system’ is OK. However, what
about renaming ‘%current-target-system’ to ‘%current-target-triplet’?
That may help a bit.
> 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.
Yes, good idea.
Ludo’.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] gnu: openblas: Fix configure flags on armhf and take target
2015-10-01 16:59 ` Mark H Weaver
2015-10-01 20:50 ` Ludovic Courtès
@ 2015-10-02 18:01 ` Andreas Enge
2015-10-13 6:20 ` Ricardo Wurmus
2 siblings, 0 replies; 7+ messages in thread
From: Andreas Enge @ 2015-10-02 18:01 UTC (permalink / raw
To: Mark H Weaver; +Cc: guix-devel
Hi Mark,
On Thu, Oct 01, 2015 at 12:59:51PM -0400, Mark H Weaver wrote:
> 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'?
sorry, your message arrived in my inbox right after I pushed the patch.
> 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.
First one would need to know what will happen on hurd, after that, this
might be a good idea.
Andreas
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] gnu: openblas: Fix configure flags on armhf and take target
2015-10-01 16:59 ` Mark H Weaver
2015-10-01 20:50 ` Ludovic Courtès
2015-10-02 18:01 ` Andreas Enge
@ 2015-10-13 6:20 ` Ricardo Wurmus
2 siblings, 0 replies; 7+ messages in thread
From: Ricardo Wurmus @ 2015-10-13 6:20 UTC (permalink / raw
To: Mark H Weaver; +Cc: guix-devel
Mark H Weaver <mhw@netris.org> writes:
> 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?
I agree. We should provide binaries for OpenBLAS for non-Intel users.
Here’s the rules file used by the Debian packages:
https://sources.debian.net/src/openblas/0.2.14-1/debian/rules/
DYNAMIC_ARCH is enabled for x86 archs; for other architectures they pass
the TARGET variable. I have a patch for setting the TARGET for mips,
which I’m planning to test on a build slave by the end of this week.
~~ Ricardo
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-10-13 6:20 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-01 8:46 [PATCH] gnu: openblas: Fix configure flags on armhf and take target Andreas Enge
2015-10-01 13:25 ` Ludovic Courtès
2015-10-01 17:36 ` Andreas Enge
2015-10-01 16:59 ` Mark H Weaver
2015-10-01 20:50 ` Ludovic Courtès
2015-10-02 18:01 ` Andreas Enge
2015-10-13 6:20 ` Ricardo Wurmus
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/guix.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.