From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark H Weaver Subject: Re: [PATCH] gnu: openblas: Fix configure flags on armhf and take target Date: Thu, 01 Oct 2015 12:59:51 -0400 Message-ID: <87twqar0c8.fsf@netris.org> References: <20151001084613.GA6497@debian> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:53586) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZhhDK-0004ad-9T for guix-devel@gnu.org; Thu, 01 Oct 2015 13:00:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZhhDF-0001fJ-Cx for guix-devel@gnu.org; Thu, 01 Oct 2015 13:00:13 -0400 Received: from world.peace.net ([50.252.239.5]:46087) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZhhDF-0001eF-AB for guix-devel@gnu.org; Thu, 01 Oct 2015 13:00:09 -0400 In-Reply-To: <20151001084613.GA6497@debian> (Andreas Enge's message of "Thu, 1 Oct 2015 10:46:13 +0200") List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: Andreas Enge Cc: guix-devel@gnu.org Andreas Enge 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 > 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