Re: [PATCH] guix: gnu-build-system: add new phase patch-dot-desktop-files

[Ludovic Courtès]

Hello!

John Darrington <address@hidden> skribis:

> Ludo wanted something like this, I think.  To be pushed to core-updates of 
> course...

This Ludo is a very demanding person…  ;-)

> * guix/build/gnu-build-system.scm (patch-dot-desktop-files): New procedure.

The approach looks good to me, so I’m just commenting on the style:

> +      ;; Search for BINARY in the output directory,
> +      ;; then all the input directories.
> +      (let lp ((dir-list (cons output-dir (map (lambda (i) (cdr i)) 
> inputs))))
> +        (if (null? dir-list)
> +            ;; Leave unchanged if we cannot find the binary.
> +            binary
> +            (let ((resolv (find-files
> +                           (car dir-list)
> +                           (lambda (file stat)
> +                             ;; The candidate file must be a regular file,
> +                             ;; have execute permission and the correct name.
> +                             (and stat
> +                                  (eq? 'regular (stat:type stat))
> +                                  (not (zero? (logand #o001 (stat:perms 
> stat))))
> +                                  ((file-name-predicate
> +                                    (string-append "^" binary "$")) file 
> stat))))))
> +
> +              (if (null? resolv)
> +                  (lp (cdr dir-list))
> +                  (car resolv))))))

Please use ‘match’ instead of car, cdr, etc. (see ‘patch-shebangs’ in
the same file for an example), and use full words such as “directories”
instead of “dir-list” (info "(guix) Coding Style").

I think you can write:

  (string=? binary file)

instead of using ‘file-name-predicate’.

> +    (for-each (match-lambda
> +                (( _ . output-dir)
> +                 (for-each (lambda (f)
> +                             (substitute* f
> +                               (("^Exec=([^/[:blank:]\r\n]*)(.*)$" _ binary 
> rest)
> +                                (string-append
> +                                 "Exec=" (find-binary binary output-dir 
> inputs) rest))
> +
> +                               (("^TryExec=([^/[:blank:]\r\n]*)(.*)$" _ 
> binary rest)
> +                                (string-append
> +                                 "TryExec=" (find-binary binary output-dir 
> inputs) rest))))
> +                           (find-files output-dir ".desktop$"))))

The ‘find-files’ regexp should be "\\.desktop$", or a predicate:

  (lambda (file stat)
    (string-suffix? ".desktop" file))

Could you send an updated patch?

Thanks for working on it!

Ludo’.