Re: [PATCH v4] Add hplip

[Andy Wingo <wingo@igalia.com>]

Hi!

Thank you for the quick reactions on the formatting.  I know it can be
irritating, but in the future things will be much smoother :)

Just a couple additional fixes.

On Tue 22 Mar 2016 20:12, Danny Milosavljevic <dannym@scratchpost.org> writes:

> +    (build-system gnu-build-system)
> +    (home-page "http://hplipopensource.com/")
> +    (synopsis "HP Printer Drivers")
> +    (description "Hewlett-Packard Printer Drivers and PPDs.")
> +    (license (list license:gpl2 license:bsd-3)) ; FIXME and which MIT
> +    ;; FIXME remove proprietary plug-in installer, hp-plugin (plugin.py)
> +    ;; FIXME PPDs use "hpcups" in cupsFilter. In which directory?
> +    ;; TODO install apparmor profile files

Probably you will need to fix the plug-in installer issue before adding
to Guix.  For the other ones, the style is to write complete sentences
in comments starting with ";;", including "." at the end.

> +    (arguments
> +     `(#:configure-flags
> +       `("--disable-network-build"
> +         ,(string-append "--prefix=" (assoc-ref %outputs "out"))
> +         ,(string-append "--sysconfdir=" (assoc-ref %outputs "out") "/etc")
> +         ;; Disable until mime.types merging works (FIXME)
> +         "--disable-fax-build"
> +         ,(string-append "--with-cupsfilterdir="
> +                         (assoc-ref %outputs "out")
> +                         "/lib/cups/filter") ; TODO merge
> +         ,(string-append "--with-cupsbackenddir="
> +                         (assoc-ref %outputs "out")
> +                         "/lib/cups/backend") ; TODO merge
> +         ,(string-append "--with-icondir="
> +                         (assoc-ref %outputs "out")
> +                         "/share/applications") ; TODO merge
> +         ,(string-append "--with-systraydir="
> +                         (assoc-ref %outputs "out")
> +                         "/etc/xdg")) ; TODO merge

With regards to "TODO merge", that is something to be done outside the
package -- either in the service, or when building a profile.  Please
remove these comments and instead be at peace with installing things in
#$out :)

> +       #:phases (modify-phases %standard-phases
> +                  (add-after 'unpack 'fix-libusb
> +                   (lambda* (#:key inputs outputs #:allow-other-keys)
> +                     (let ((out (assoc-ref outputs "out")))
> +                       (substitute* "base/g.py"
> +                                    (("'/etc/hp/hplip.conf'")
> +                                    (string-append "'" out
> +                                                   "/etc/hp/hplip.conf" "'")))

Can you re-indent?  It should look like this:

  (substitute* filename
    ((pattern)
     substitution))

FWIW, in emacs, the .dir-locals.el should add rules to make C-M-q do
this automatically.

> +    ; Python3 support is available starting from HPLIP-3.15.2

";;" and a full stop please.

Thank you again for your patience!

Andy