Hi Nicolas,

> Hello,
>
> phodina via Guix-patches via guix-patches@gnu.org writes:
>
> > -   gnu/packages/qt.scm (inkbox): New variable.
>
> Thank you. Some comments follow.
>
> First, this should go to "ebook.scm" instead of "qt.scm".

Thanks, that is a better place to put it.
>
> > -   (source (origin
>
> You should instead a newline before "(origin".

Fixed.
>
> > -                (method git-fetch)
> >
> >
> > -                (uri
> >
> >
> > -                 (git-reference
> >
> >
> > -                  (url "https://alpinekobox.ddns.net/InkBox/inkbox/")
> >
> >
> > -                  (commit version)))
> >
> >
> > -                (file-name (git-file-name name version))
> >
> >
> > -                (sha256
> >
> >
> > -                 (base32
> >
> >
> > -                  "126cqn0ixcn608lv2hd9f7zmzj4g448bnpxc7wv9cvg83qqajh5n"))))
> >
> >
>
> Nitpick: hash should go on the same line as base32.

Fixed.
>
> > -   (build-system qt-build-system)
> > -   (arguments
> > -       '(#:tests? #f
> >
> >
>
> You should add a comment explaining why tests are disabled (i.e.: there
>
> is no tests).

Fixed.
>
> > -         #:phases
> >
> >
> > -         (modify-phases %standard-phases
> >
> >
> > -           (add-after 'unpack 'prefix-opt
> >
> >
> > -             (lambda* _
> >
> >
> > -               (substitute* "inkbox.pro"
> >
> >
> > -                 (("/opt/\\\\$\\\\$\\\\{TARGET\\\\}") (string-append (assoc-ref %outputs "out"))))))
> >
> >
>
> You should use (lambda* (#:key outputs #:allow-other-keys) ...) and then
>
> (assoc-ref outputs "out") instead.
>
> > -           (replace 'configure
> >
> >
> > -             (lambda* (#:key make-flags #:allow-other-keys)
> >

Thanks, that is the correct syntax.
> >
> > -               (apply invoke (cons "qmake" make-flags)))))))
> >
> >
> > -   (native-inputs
> > -       `(("qtbase" ,qtbase-5)))
> >
> >
>
> Does it fail with qtbase 6?
Unfortunately it does not compile though it shouldn't be that hard to port it. But here it is about packaging.
>
> > -   (license license:gpl1)))
>
> License is actually gpl3, according to LICENSE file.

Sorry, my mistake.
>
> Could you send an updated patch?
>
> Regards,
> --------------------------------------------------------------------------------------------------
>
> Nicolas Goaziou

Thanks for the remarks!

----
Petr