Hi Ricardo!

Thank you for your review!  

Ricardo Wurmus <rek...@elephly.net> writes:
>> From d6a6a5ded95071a58a160a435ccf56d6828148b0 Mon Sep 17 00:00:00 2001
>> From: Lukas Gradl <lgr...@openmailbox.org>
>> 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?

My idea here is the following:  The source tree downloaded here contains
the complete source for libring, which includes the patches to
pjproject.  In this snippet I try to get rid of all the other libring
files that are not needed and only keep the patches to pjproject.  Then
I try to make the directory tree that contains these patches as shallow
as possible.  I remember I had some problems with copying them to "."
but trying again now it works.  I attached an updated patch that does
this.

>
>> +             (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 “;;”.

Done.

> Can we split off the fork of “resample” into a separate package?
> This package definition is already very big.

I tried this a few weeks (months?) ago.  The biggest roadblocks were the
missing header added by pjproject and some dependencies of 'resample'.

I will look into it again.

>
>> +             (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.

This is a leftover from a previous attempt at a different version.
I am very sorry about this.  I will be more careful next time before
submitting. 

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

Yes, but so far without success.

Sorry that this comment was still there.  The configure phase checks for
portaudio and fails if it is not available.  I had a vague hunch at the
time that portaudio is actually not being used although configure fails if
it can not find it.  I based this off the observations that there is no
store reference to portaudio and that a lot of flags in the build log
seem to disable portaudio.  I will look more into this but I think that
most likely portaudio can not be omitted.


>> +      (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.

OK, fixed.

>> +       `(("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?

Some of the patches introduce changes to '.in' and '.ac' files and the
snippet changes "aconfigure.ac", so I think it is necessary to run
autoconf to make sure these changes will be used.

>> +         ("pkg-config" ,pkg-config)
>> +         ("libtool" ,libtool)))
>> +      (arguments
>> +       `(#:test-target
>> +         "selftest"
>
> Please put them on the same line.

Sure, Sorry about that.

>> +         #: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.

The version that is bundled with libring had these flags, so I think
ring will probably not use them.  I can try to enable more of these.

>> +         #:phases
>> +         (modify-phases %standard-phases
>> +           (add-after 'unpack 'apply-patches
>> +             (lambda* (#:key inputs #:allow-other-keys)
>> +               (begin
>
> No need for “begin” here.

OK.  I changed this section a little to work with the patches sitting in
the top level of the 'sfl-patches' tarball instead of the 'sfl-patches'
sub-directory.

>
>> +                 (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?

I think it should, but I was not able to get it to work.  I tried
"(patches (list sfl-patches))" because I think that the 'patches' field
should accept origin objects, however this just yielded the following:

---8<--- cut here -------------------- start --->8---
patch unexpectedly ends in middle of line
/gnu/store/ni491r4ffm03v0cr70df12lwiq826das-patch-2.7.5/bin/patch: **** Only 
garbage was found in the patch input.
source is under 'pjproject-2.4'
applying 
'/gnu/store/mjh1a1fjlz53psz4qxx0kh09qmbqj0jc-sfl-patches-0.0.0-1.tar.xz'...
builder for 
`/gnu/store/kb35an4z14adc0kn592dawxqlq6dzyhd-pjproject-2.4.tar.xz.drv' failed 
to produce output path 
`/gnu/store/lhrw7zwcxgginsi4w1pfgrkhpims1rza-pjproject-2.4.tar.xz'
---8<--- cut here -------------------- end ----->8---

It seems like it is trying to apply the tarball as a patch, so it is
probably not as easy as "(patches (list sfl-patches))".  Are there any
packages that use an 'origin' object in the 'patches' field?  Grepping
with

grep -r "(patches " gnu/packages | grep -v "search-patch"

does not yield any results that were helpful to me.

>> +           (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?

Yes, that is right.

>> +           (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?

The file "aconfigure.ac" gets changed by the patch "gnutls.patch", but
that patch also changes "aconfigure", so it might not need to rerun
autoconf.  However, I also change this file in a snippet (to disable the
requirement of building the third-party directory), so running autoconf
can not be avoided here.

>
>> +           (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.

OK, done.

>
>> +                  (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.

Done.  Sorry about that.  I remember having problems with this but it
worked just now.

> ~~ Ricardo
>




Thank you for your detailed review!

I apologize for the high number of trivial issues!  I will review my own
work more careful next time before submitting.

The attached patch does not yet address the following:
- Separate package for resample.
- Apply patches in the (patches ...) field instead of a build phase.
- Maybe remove portaudio from inputs.

I will work on all of these, but I think some of these items might take some
time.

Thank you!
Best,
Lukas

From 060f49eb22788ee5a331ae12c5094066b341efcd Mon Sep 17 00:00:00 2001
From: Lukas Gradl <lgr...@openmailbox.org>
Date: Wed, 20 Jul 2016 21:26:32 -0500
Subject: [PATCH] gnu: Add pjproject

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

diff --git a/gnu/packages/telephony.scm b/gnu/packages/telephony.scm
index d8a33dd..ce2034a 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,161 @@ 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)))))
+                 (copy-recursively "contrib/src/pjproject/" ".")
+                 (for-each delete-file-recursively files)))
+             (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
+             (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"))))
+      (build-system gnu-build-system)
+      (inputs
+       `(("portaudio" ,portaudio)))
+      (propagated-inputs
+       ;; These packages are referenced in the Libs field of the pkg-config
+       ;; file that will be installed by pjproject.
+       `(("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)
+         ("pkg-config" ,pkg-config)
+         ("libtool" ,libtool)))
+      (arguments
+       `(#:test-target "selftest"
+         #:configure-flags  ;; The disabled features are not used by libring.
+         (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"
+               "CFLAGS=-DPJ_ICE_MAX_CAND=32 -DPJ_ICE_MAX_CHECKS=150 \
+-DPJ_ICE_COMP_BITS=2")
+         #:phases
+         (modify-phases %standard-phases
+           (add-after 'unpack 'apply-patches
+             (lambda* (#:key inputs #:allow-other-keys)
+               (let ((patch-dir "patch-dir")
+                     (patches
+                        '("errno" "endianness" "gnutls" "ipv6" "ice_config"
+                          "multiple_listeners" "pj_ice_sess")))
+                 (mkdir patch-dir)
+                 (system* "tar" "-xvf" (assoc-ref inputs "sfl-patches")
+                          "-C" patch-dir "--strip-components=1")
+                 (for-each
+                  (lambda (file)
+                    (zero?
+                     (system* "patch" "--force" "-p1" "-i"
+                              (string-append patch-dir "/"
+                                             file ".patch"))))
+                  patches))))
+           (add-before 'build 'build-dep
+             (lambda _ (zero? (system* "make" "dep"))))
+           (add-before 'patch-source-shebangs 'autoconf
+             (lambda _
+               (zero?
+                (system* "autoconf" "-v" "-f" "-i" "-o"
+                         "aconfigure" "aconfigure.ac"))))
+           (add-before 'autoconf 'disable-some-tests
+             ;; Three of the six test programs fail due to missing network
+             ;; access.
+             (lambda _
+               (substitute* "Makefile"
+                 (("selftest: pjlib-test pjlib-util-test pjnath-test \
+pjmedia-test pjsip-test pjsua-test")
+                  "selftest: pjlib-test pjlib-util-test pjmedia-test")))))))
+      (home-page "www.pjsip.org")
+      (synopsis "Session Initiation Protocol (SIP) stack")
+      (description "PJProject provides an implementation of the Session
+Initiation Protocol (SIP) and a multimedia framework.
+
+This package is intended for use with libring.  There are several custom
+patches, most notably the use of gnutls instead of openssl for encryption.")
+      (license '(gpl2+ ; pjproject
+                 gpl3+ ; sfl-patches
+                 lgpl2+))))) ; bundled resample
+
-- 
2.9.0

Attachment: signature.asc
Description: PGP signature

Reply via email to