Re: [PATCH]: Add CUPS.

[Ludovic Courtès]

Ricardo Wurmus <address@hidden> skribis:

> * IJS is part of ghostscript, but for some reason is not built even when
>   passing "--with-ijs" as a configure flag.  There is no configure
>   script and no usable Makefile in ./ijs, so we actually have to run
>   ./autogen.sh after generating macros that work with libtool 2.4.2.
>   The second patch adds a new very ugly phase that does just that.  I'd
>   love to change this if someone can come up with a better way to do
>   this.

It might be clearer and also easier (because all of %standard-phases
would be readily usable) to make ijs a separate package, similar to the
libtool/libltdl split.  WDYT?

> * cups-filters also depends on CUPS libraries and binaries, so
>   cups-minimal has been added to satisfy this dependency.  During
>   installation, cups-filters attempts to install files to the CUPS
>   package output directory.  The install phase is modified to direct the
>   files to the output directory of cups-filters.

OK.

> * the CUPS package itself then adds links to the required files in
>   cups-filters.  CUPS does not come with a mechanism to load filters,
>   backends, charsets, mime files, etc from alternative directories, so I
>   chose to use symlinks.

Makes sense.

> * the CUPS tests are run before the installation of any symlinks, so to
>   establish a working test environment quite a lot of fixes to the test
>   phase are needed.  Without the files from cups-filters seven tests
>   fail.  With the files in place only three tests fail.  I have not been
>   able to fix these remaining failures, even though I have been trying
>   for a long time.

Fair enough.

> From 8e671fdd888bf9548bf52acf8789500c90d4b072 Mon Sep 17 00:00:00 2001
> From: Ricardo Wurmus <address@hidden>
> Date: Wed, 7 Jan 2015 17:49:00 +0100
> Subject: [PATCH 1/3] gnu: Add QPDF.
>
> * gnu/packages/pdf.scm (qpdf): New variable.

LGTM.

> From 36eac76f1aa129f2e22d4341fe4d64d4c5b8fd0a Mon Sep 17 00:00:00 2001
> From: Ricardo Wurmus <address@hidden>
> Date: Fri, 9 Jan 2015 15:19:54 +0100
> Subject: [PATCH 2/3] gnu: ghostscript: build IJS library
>
> * gnu/packages/ghostscript.scm (ghostscript): build and install IJS library
>   and header files.

[...]

> +    `(#:configure-flags '("--with-ijs")

If this flag has no effect, better remove it.  However, it would be good
to add a comment explaining that --with-ijs cannot be used because it
doesn’t work as advertised.

> +       (alist-cons-after
> +        'build 'install-ijs-so
> +        (lambda* (#:key outputs #:allow-other-keys)
> +          ;; need to regenerate macros
> +          (system* "autoreconf" "-if")
> +          (substitute* "autogen.sh"
> +            (("\\$srcdir/configure") (string-append (which "bash") " 
> $srcdir/configure")))
> +          (system* "bash" "autogen.sh")
> +
> +          ;; build and install ijs
> +          (with-directory-excursion "ijs"
> +            (substitute* "autogen.sh"
> +              (("/bin/sh") (which "bash")))
> +            (system* "bash" "autogen.sh")
> +            (substitute* "configure"
> +              (("/bin/sh") (which "bash")))
> +            (setenv "SHELL" (which "sh"))
> +            (setenv "CONFIG_SHELL" (which "sh"))
> +            (system* "./configure" (string-append "--prefix=" (assoc-ref 
> outputs "out")))
> +            (system* "make")
> +            (zero? (system* "make" "install"))))
> +         (alist-cons-after
> +          'build 'build-so
> +          (lambda _ (system* "make" "so"))
> +          (alist-cons-after
> +           'install 'install-so
> +           (lambda _ (system* "make" "install-so"))
> +           (alist-cons-after
> +            'install 'install-ijs-headers
> +            (lambda _
> +              (let* ((out        (assoc-ref %outputs "out"))
> +                     (ijsinclude (string-append out "/include/ijs/")))
> +                (mkdir-p ijsinclude)
> +                (for-each (lambda (file)
> +                            (copy-file file (string-append ijsinclude 
> (basename file))))
> +                          (find-files "ijs/" "\\.h$"))))
> +            %standard-phases)))))))

I suspect this will be simplified by making IJS a separate package.

BTW, make sure that phases return #t on success on #f on failure.  A
phase that finishes with a call type ‘system*’ must typically do:

  (lambda _
    ;; ...
    (zero? (system* "make" "install")))

> From d42e6a3233b7c5142d1c91d8c2b1c751b74bd444 Mon Sep 17 00:00:00 2001
> From: Ricardo Wurmus <address@hidden>
> Date: Mon, 5 Jan 2015 13:56:51 +0100
> Subject: [PATCH 3/3] gnu: Add CUPS.
>
> * gnu/packages/cups.scm: New file.
> * gnu-system.am (GNU_SYSTEM_MODULES): Add it.

[...]

> +         "--with-gnu-ld"

I suspect this is not needed; ./configure should be able to determine it
automatically.

> +         "--disable-static"

Why?  By default we let packages provide both .so and .a files.

> +       #:phases
> +       (alist-cons-before
> +        'configure
> +        'patch-target-dirs
> +        (lambda _
> +          (let ((out (assoc-ref %outputs "out")))
> +            ;; install backends, banners and filters to cups-filters output
> +            ;; directory, not the cups server directory
> +            (substitute* "Makefile.in"
> +              (("CUPS_DATADIR = @CUPS_DATADIR@")
> +               (string-append "CUPS_DATADIR = " out "/share/cups"))
> +              (("pkgcupsserverrootdir = \\$\\(CUPS_SERVERROOT\\)")
> +               (string-append "pkgcupsserverrootdir = " out))
> +              (("pkgbackenddir = \\$\\(CUPS_SERVERBIN\\)/backend")
> +               (string-append "pkgbackenddir = "
> +                              out
> +                              "/backend"))
> +              (("pkgfilterdir = \\$\\(CUPS_SERVERBIN\\)/filter")
> +               (string-append "pkgfilterdir = "
> +                              out
> +                              "/filter")))))

Could you make this phase a ‘snippet’, by using a literal “$(prefix)” or
address@hidden instead of ‘out’?

> +    (synopsis "OpenPrinting CUPS filters and backends")
> +    (description
> +     "Contains backends, filters, and other software that was once part of 
> the
> +core CUPS distribution but is no longer maintained by Apple Inc.  In addition
> +it contains additional filters developed independently of Apple, especially
> +filters for the PDF-centric printing workflow introduced by OpenPrinting.")
> +    ;; see COPYING for details
> +    (license (list license:gpl2
> +                   license:gpl2+
> +                   license:gpl3
> +                   license:gpl3+
> +                   license:lgpl2.0+
> +                   license:expat))))

Could you augment the comment to mention what this license list means?
Each filter has its own license, or something like that?

> +(define-public cups-minimal
> +  (package
> +    (name "cups-minimal")

Could you add a comment with the explanation that you gave in this
message as to why it is needed?

> +(define-public cups
> +  (package
> +    (name "cups")

I think this should inherit from ‘cups-minimal’, and just change the
‘name’ and ‘arguments’ fields.  WDYT?

> +        (alist-cons-before
> +         'check
> +         'patch-tests

This is a hairy phase ;-), but I guess there’s no real alternative.
Please try to keep lines below 80 chars.

> +                (system* "rm" "-rf" (string-append out banners))

Use ‘delete-file-recursively’ instead.

> +                (system* "rm" "-rf" (string-append out data))

And here too.

> +    (synopsis "CUPS printing system")

What about “The Common Unix Printing System”?

> +    (description
> +     "CUPS printing system provides a portable printing layer for UNIX®
> +operating systems.  It has been developed by Apple Inc. to promote a standard
> +printing solution for all UNIX vendors and users.  CUPS provides the System V
> +and Berkeley command-line interfaces.")

What about this (based on README.txt):

  CUPS is a printing system that uses the Internet Printing Protocol
  (IPP).  It provides System V and BSD command-line interfaces, as well
  as a Web interface and a C programming interface to manage printers
  and print jobs.  It supports printing to both local (parallel, serial,
  USB) and networked printers, and printers can be shared from one
  computer to another.  Internally, CUPS uses PostScript Printer
  Description (PPD) files to describe printer capabilities and features
  and a wide variety of generic and device-specific programs to convert
  and print many types of files.

Thanks for working on it!

Ludo’.