Re: [Patch 1/10] Add pjproject.

[Ricardo Wurmus]

Hi Lukas,

thanks for the patches!

> From d6a6a5ded95071a58a160a435ccf56d6828148b0 Mon Sep 17 00:00:00 2001
> From: Lukas Gradl <address@hidden>
> Date: Wed, 20 Jul 2016 21:26:32 -0500
> Subject: [PATCH 01/10] gnu: Add pjproject

> * gnu/packages/telephony.scm (pjproject-sfl): New variable.
> ---
>  gnu/packages/telephony.scm | 181 
> +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 181 insertions(+)

> diff --git a/gnu/packages/telephony.scm b/gnu/packages/telephony.scm
> index d8a33dd..4893dbd 100644
> --- a/gnu/packages/telephony.scm
> +++ b/gnu/packages/telephony.scm
> @@ -23,6 +23,7 @@
 
>  (define-module (gnu packages telephony)
>    #:use-module (gnu packages)
> +  #:use-module (gnu packages audio)
>    #:use-module (gnu packages autotools)
>    #:use-module (gnu packages gnupg)
>    #:use-module (gnu packages linux)
> @@ -34,6 +35,7 @@
>    #:use-module (guix licenses)
>    #:use-module (guix packages)
>    #:use-module (guix download)
> +  #:use-module (guix git-download)
>    #:use-module (guix build-system gnu))
 
>  (define-public commoncpp
> @@ -286,3 +288,182 @@ lists.  All you need to join an existing conference is 
> the host name or IP
>  address of one of the participants.")
>      (home-page "http://holdenc.altervista.org/seren/";)
>      (license gpl3+)))
> +
> +(define-public pjproject-sfl
> +  (let ((sfl-patches
> +         (let ((commit "3dbedf53e9cceebb1eed155b5143026f6d253cb8"))
> +           (origin
> +             (method git-fetch)
> +             (uri
> +              (git-reference
> +               (url
> +                "https://gerrit-ring.savoirfairelinux.com/ring-daemon.git";)
> +               (commit commit)))
> +             (file-name (string-append "sfl-patches" "-" "0.0.0-1."
> +                                       (string-take commit 7)  "-checkout"))
> +             (modules '((guix build utils)
> +                        (ice-9 ftw)))
> +             (snippet
> +              '(let ((files (scandir "." (lambda (file)
> +                                           (if (or (string=? file ".")
> +                                                   (string=? file ".."))
> +                                               #f
> +                                               #t)))))
> +                 (mkdir-p "sfl-patches")
> +                 (copy-recursively "contrib/src/pjproject/" "sfl-patches")
> +                 (for-each delete-file-recursively files)))

Why is this needed?

> +             (sha256
> +              (base32
> +               "1xnvkv0h24zr1dcmp7djjhqgzvrwic242jy4hb3m5qv71azvcsqg"))))))
> +    (package
> +      (name "pjproject")
> +      (version "2.4")
> +      (source
> +       (origin
> +         (method url-fetch)
> +         (uri (string-append
> +               "http://www.pjsip.org/release/";
> +               version "/" name "-" version ".tar.bz2"))
> +         (modules '((guix build utils)))
> +         (snippet
> +          '(begin
> +           ; The following removes bundled packages except for 'resample'.
> +           ; Pjproject uses some of the source files of resample and one own
> +           ; header (resamplesubs.h) not included with resample to build a
> +           ; shared library.  Upstream resample does not build this
> +           ; library. The license of resample is the LGPL2+
> +           ; The homepage of resample is at:
> +           ; 
> https://ccrma.stanford.edu/~jos/resample/Free_Resampling_Software.html

For line comments like this please use “;;”.  Can we split off the fork
of “resample” into a separate package?  This package definition is
already very big.

> +             (let ((third-party-directories
> +                    (list "BaseClasses" "bdsound" "bin" "g7221" "gsm"
> +                          "ilbc" "lib" "milenage" "mp3" "portaudio"
> +                          "speex" "srtp"  ; Keep only resample, build and
> +                                        ; README.txt.
> +                          "build/baseclasses" "build/g7221" "build/gsm"
> +                          "build/ilbc" "build/milenage" "build/portaudio"
> +                          "build/samplerate" "build/speex" "build/srtp")))
> +                          ; Keep only Makefiles related to resample.
> +               (for-each (lambda (file)
> +                           (delete-file-recursively
> +                            (string-append "third_party/" file)))
> +                         third-party-directories)
> +               #t)
> +             (let ((third-party-dirs
> +                    (list "gsm" "ilbc" "speex" "portaudio"
> +                          "g7221" "srtp")))
> +               (for-each
> +                (lambda (dirs)
> +                  (substitute* "third_party/build/os-linux.mak"
> +                    (((string-append "DIRS += " dirs)) "")))
> +                third-party-dirs))
> +             (substitute* "aconfigure.ac"
> +               (("third_party/build/portaudio/os-auto.mak") ""))))
> +         (sha256
> +          (base32
> +           "0n90n1p41svf23d4fag8jqbjnv82fz14z6zchb8j1kldvap1b00h"))))
> +           ;"13fx7kpf1sswj7r0zl7fqkzg3rl5xz3dl96wdnv15qxfviq5wvq8")))) 2.4.5

Please remove the extra hash.

> +      (build-system gnu-build-system)
> +      (inputs
> +       `(("portaudio" ,portaudio))) ; It might be possible to remove this.

Have you tried? :)

> +      (propagated-inputs ; These packages are referenced in the Libs field of
> +                         ; the pkg-config file that will be installed by
> +                         ; pjproject.

We normally use line comments for longer blocks like this.

> +       `(("speex" ,speex)
> +         ("libsrtp" ,libsrtp)
> +         ("gnutls" ,gnutls)
> +         ("util-linux" ,util-linux)))
> +      (native-inputs
> +       `(("sfl-patches" ,sfl-patches) ; These patches are distributed with 
> the
> +                                      ; libring source.  They contain various
> +                                      ; nontrivial changes such as the use of
> +                                      ; gnutls instead of openssl.
> +         ("autoconf" ,autoconf)
> +         ("automake" ,automake)

Why are the autotools needed?  Is this tarball not bootstrapped?

> +         ("pkg-config" ,pkg-config)
> +         ("libtool" ,libtool)))
> +      (arguments
> +       `(#:test-target
> +         "selftest"

Please put them on the same line.

> +         #:configure-flags
> +         (list "--disable-oss"
> +               "--disable-sound"
> +               "--disable-video"
> +               "--enable-ext-sound"
> +               "--disable-g711-codec"
> +               "--disable-l16-codec"
> +               "--disable-gsm-codec"
> +               "--disable-g722-codec"
> +               "--disable-g7221-codec"
> +               "--disable-ilbc-codec"
> +               "--disable-opencore-amr"
> +               "--disable-sdl"
> +               "--disable-ffmpeg"
> +               "--disable-v4l2"
> +               "--enable-ssl=gnutls"
> +               "--with-external-speex"
> +               "--with-external-pa"
> +               "--with-external-srtp")

Why are so many features disabled?  A comment would be nice.

> +         #:phases
> +         (modify-phases %standard-phases
> +           (add-after 'unpack 'apply-patches
> +             (lambda* (#:key inputs #:allow-other-keys)
> +               (begin

No need for “begin” here.

> +                 (mkdir "patch-dir")
> +                 (system* "tar" "-xvf" (assoc-ref inputs "sfl-patches")
> +                          "-C" "patch-dir" "--strip-components=1")
> +                 (let ((patch-dir
> +                        (string-append "patch-dir" "/sfl-patches"))
> +                       (patches
> +                        '("errno" "endianness" "gnutls" "ipv6" "ice_config"
> +                          "multiple_listeners" "pj_ice_sess")))
> +                   (for-each
> +                    (lambda (file)
> +                      (zero?
> +                       (system* "patch" "--force" "-p1" "-i"
> +                                (string-append patch-dir "/"
> +                                               file ".patch"))))
> +                    patches)))))

Normally, we don’t patch in build phases.  We use the “(patches…)” field
in the source definition instead.  Would this be possible here as well?

> +           (add-before 'build 'build-dep
> +             (lambda _
> +               (zero?
> +                (system* "make" "dep"))))

The lambda can go on one line.  Is this to build the remaining bundled
library?

> +           (add-before 'patch-source-shebangs 'autoconf
> +             (lambda _
> +               (zero?
> +                (system* "autoconf" "-v" "-f" "-i" "-o"
> +                         "aconfigure" "aconfigure.ac"))))

It would be better if we didn’t have to run autoconf.  Is it possible to
avoid it?

> +           (add-before 'autoconf 'disable-some-tests
> +             (lambda _
> +               (substitute* "Makefile"
> +                 (((string-append
> +                    "selftest: "
> +                    "pjlib-test "
> +                    "pjlib-util-test "
> +                    "pjnath-test "  ; This test fails.
> +                    "pjmedia-test "
> +                    "pjsip-test " ; This test fails.
> +                    "pjsua-test")) ; This test fails.  This test would need
> +                                        ; python.

Please don’t use “string-append” here.  You can just break the string
literal.  The comments would go on top then.

> +                  (string-append
> +                   "selftest: "
> +                   "pjlib-test "
> +                   "pjlib-util-test "
> +                   "pjmedia-test")))))
> +           (add-before 'autoconf 'set-flags
> +             (lambda _
> +               (setenv "CFLAGS"
> +                       (string-append
> +                        "-DPJ_ICE_MAX_CAND=32" " "
> +                        "-DPJ_ICE_MAX_CHECKS=150" " "
> +                        "-DPJ_ICE_COMP_BITS=2")))))))

We can pass flags in #:make-flags or #:configure-flags.  That’s better
than using a build phase.

~~ Ricardo