GNU bug report logs - #37284
[PATCH] added gnu/packages/fmit.scm (Free Musical Instrument Tuner)

Previous Next

Full log

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Goaziou <mail <at> nicolasgoaziou.fr>
To: raingloom via Guix-patches via <guix-patches <at> gnu.org>
Cc: 37284 <at> debbugs.gnu.org
Subject: Re: [bug#37284] [PATCH] added gnu/packages/fmit.scm (Free Musical
 Instrument Tuner)
Date: Mon, 02 Sep 2019 19:29:34 +0200

Hello,

raingloom via Guix-patches via <guix-patches <at> gnu.org> writes:

> * gnu/packages/fmit.scm: created

Thank you!

> ---
>  gnu/packages/fmit.scm | 64 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>  create mode 100644 gnu/packages/fmit.scm
>
> diff --git a/gnu/packages/fmit.scm b/gnu/packages/fmit.scm

Is there any reason to create a new module instead of putting the
package into "music.scm"?

> +    (arguments
> +     '(#:make-flags (let ((out (assoc-ref %outputs "out"))) (list (string-append "PREFIX=" out) (string-append "PREFIXSHORTCUT=" out)))

This line is too long. Moreover, I think "PREFIX=" is not necessary, as
Guix adds it automatically. I'm not sure about "PREFIXSHORTCUT=", tho.

It seems PREFIX and PREFIXSHORTCUT are used in qmake, not in make, so it
may be possible to remove the whole #:make-flags altogether. WDYT?

> +       #:phases
> +       (modify-phases %standard-phases
> +	 (delete 'configure)

Indentation looks wrong. Could you take care about it?

> +	 (add-before 'build 'qmake
> +	   (lambda _
> + (let ((out (assoc-ref %outputs "out"))) (invoke "qmake" "fmit.pro"
> (string-append "PREFIX=" out) (string-append "PREFIXSHORTCUT=" out)
> "CONFIG+=acs_qt acs_alsa acs_jack acs_portaudio"))))

The line is too long. I suggest:

  (let ((out (assoc-ref %outputs "out")))
    (invoke "qmake"
            "fmit.pro"
            (string-append "PREFIX=" out)
            (string-append "PREFIXSHORTCUT=" out)
            "CONFIG+=acs_qt acs_alsa acs_jack acs_portaudio"))

> +    (inputs
> +     `(("fftw" ,fftw)
> +       ("portaudio" ,portaudio)
> +       ("qtmultimedia" ,qtmultimedia)
> +       ("qtsvg" ,qtsvg)
> +       ("alsa-lib" ,alsa-lib)
> +       ("jack" ,jack-1)
> +       ("qtbase" ,qtbase)))
> +    (native-inputs
> +     `(("itstool" ,itstool)
> +       ("qttools" ,qttools)
> +       ("hicolor-icon-theme" ,hicolor-icon-theme)
> +       ("gettext" ,gnu-gettext)))

Could you re-order (native-)inputs alphabetically?

> +    (synopsis "Free Musical Instrument Tuner")

I understand Free is the F from FMIT, but, as a synopsis, I don't think
it brings much value. Maybe "Musical instrument tuner" is enough?

> +    (description "FMIT is a graphical utility for tuning musical instruments, with error and volume history, and advanced features")
> +    (home-page "http://gillesdegottex.github.io/fmit/")

Prefer https:// here.

> +    (license gpl3+)))

If you move it to music.scm, I believe it should be prefixed with
license:

Also, the license seems wrong, it should be (list license:gpl2+
license:lgpl2.1) with a comment explaining that most of the code is
under GPL2+, but some abstract or helper classes are under LGPL2.1.

Regards,

-- 
Nicolas Goaziou

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.