From mboxrd@z Thu Jan 1 00:00:00 1970 From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Subject: Re: [PATCH] guix: lint: Check for version-only origin file names. Date: Fri, 28 Aug 2015 09:48:48 +0200 Message-ID: <87y4gvvonz.fsf@gnu.org> References: <1440371134-2699-1-git-send-email-ericbavier@openmailbox.org> <87pp2cmgss.fsf@netris.org> <20150824191024.3da87f41@openmailbox.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:44498) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZVEPD-0005y3-FF for guix-devel@gnu.org; Fri, 28 Aug 2015 03:49:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZVEPC-0003jb-Bf for guix-devel@gnu.org; Fri, 28 Aug 2015 03:48:59 -0400 In-Reply-To: <20150824191024.3da87f41@openmailbox.org> (Eric Bavier's message of "Mon, 24 Aug 2015 19:10:24 -0500") 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: Eric Bavier Cc: guix-devel@gnu.org, Eric Bavier Eric Bavier skribis: > From 0311d5b383003600ac43d3a9bfdec0ad3c398db2 Mon Sep 17 00:00:00 2001 > From: Eric Bavier > Date: Sun, 23 Aug 2015 18:00:45 -0500 > Subject: [PATCH] guix: lint: Check for version-only origin file names. > > * guix/scripts/lint.scm (check-source): Emit warning if source filename > contains only the version of the package. > * tests/lint.scm ("source: filename", "source: filename v", > "source: filename valid"): New tests. > * doc/guix.texi (Invoking guix lint): Mention file name check. > Offending packages updated. This is useful, thanks for looking into it. I would prefer it to make a separate linter, like =E2=80=98source-file-name= =E2=80=99. The reason is that =E2=80=98source=E2=80=99 is a relatively expensive check= , since it needs to probe URLs (so you might want to skip it in some cases), whereas the linter your propose is lightweight. WDYT? > --- a/gnu/packages/algebra.scm > +++ b/gnu/packages/algebra.scm > @@ -386,6 +386,7 @@ cosine/ sine transforms or DCT/DST).") > (method url-fetch) > (uri (string-append "https://bitbucket.org/eigen/eigen/get= /" > version ".tar.bz2")) > + (file-name (string-append name "-" version ".tar.bz2")) Could you make these package updates a separate patch? Some may trigger large rebuilds, so you may have to keep them for =E2=80=98core-updates=E2= =80=99 or such. > + (define (origin-version-name? origin) > + ;; Return #t if the source file name contains only a version; indica= tes > + ;; that the origin needs a 'file-name' field. > + (let ((filename (store-path-package-name > + (with-store store > + (derivation->output-path > + (package-source-derivation store origin))))) > + (version (package-version package))) > + (or (string-prefix? version filename) > + ;; Common in many projects is for the filename to start with a= "v" > + ;; followed by the version, e.g. "v3.2.0.tar.gz". > + (string-prefix? (string-append "v" version) filename)))) Opening a connection to the store in the middle of the code (=E2=80=98with-store=E2=80=99) is Bad Practice. ;-) I think this can actually be made simpler, with something akin to what =E2=80=98node-full-name=E2=80=99 does in guix/scripts/graph.scm. Maybe we = could extract an =E2=80=98origin-actual-file-name=E2=80=99 procedure from that and move i= t to (guix packages). WDYT? > +(test-assert "source: filename" =E2=80=9Cfile name=E2=80=9D (two words). Could you send an updated patch? Thanks, Ludo=E2=80=99.