On Fri, 28 Aug 2015 09:48:48 +0200
ludo@gnu.org (Ludovic Courtès) wrote:

> Eric Bavier <ericbavier@openmailbox.org> skribis:
> 
> > From 0311d5b383003600ac43d3a9bfdec0ad3c398db2 Mon Sep 17 00:00:00 2001
> > From: Eric Bavier <bavier@member.fsf.org>
> > 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.

Thanks for the review!
 
> I would prefer it to make a separate linter, like ‘source-file-name’.
> The reason is that ‘source’ 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.

Makes sense.

> 
> > --- 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 ‘core-updates’ or such.

I've left the package updates out of the attached patches.

> 
> > +  (define (origin-version-name? origin)
> > +    ;; Return #t if the source file name contains only a version; indicates
> > +    ;; 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
> (‘with-store’) is Bad Practice.  ;-)
> 
> I think this can actually be made simpler, with something akin to what
> ‘node-full-name’ does in guix/scripts/graph.scm.  Maybe we could extract
> an ‘origin-actual-file-name’ procedure from that and move it to (guix
> packages).  WDYT?

The first attached patch does this.  Is using the basename of the
source URI always accurate?  I.e. are there cases where the store file
name might not match the URI's basename?  This uncertainty, I think, is
what caused me to use store-path-package-name initially.

This revised patch might actually be considered "more accurate" in that
the checker now flags origins from 'git-reference' et al where no
'file-name' field is declared.

`~Eric