[1/10] Add pjproject.

Hi Ricardo!

Thank you for your review!  

Ricardo Wurmus <rekado@elephly.net> writes:
>> From d6a6a5ded95071a58a160a435ccf56d6828148b0 Mon Sep 17 00:00:00 2001
>> From: Lukas Gradl <lgradl@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

Hi Lukas,

Thanks for sending an updated patch!

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

Is there no release that includes these patches?  Why do we have to
apply them ourselves?  In Guix we try to avoid patching upstream sources
with functional patches (as opposed to patches that fix problems with
building the software on Guix), as this is really upstream’s work.

Is this really pjproject version 2.4 when we apply a massive patch set
like that?

~~ Ricardo

Hi Ricardo,

Sorry for the delay!

Ricardo Wurmus <rekado@elephly.net> writes:
>> 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.
>
> Is there no release that includes these patches?  Why do we have to
> apply them ourselves?  In Guix we try to avoid patching upstream sources
> with functional patches (as opposed to patches that fix problems with
> building the software on Guix), as this is really upstream’s work.

There is no release that I know of that contains these patches.  The
Ring project ships a Makefile in the 'contrib' directory of the
'libring' source tree that downloads the upstream Pjproject source
and applies these very same patches to it.  It then continues to build a
bundled version of pjproject + patches.

> Is this really pjproject version 2.4 when we apply a massive patch set
> like that?

It is not.  It is a fork of pjproject that introduces several functional
changes, the most notable one being the use of GnuTLS instead of
OpenSSL.  I suspect the Ring project's motivation for doing this are at
least in part legal issues as the OpenSSL license seems to be
incompatible with the GPL according to
"http://www.gnu.org/licenses/license-list.html".  I don't know if there
are technical benefits of using one over the other.

I tried to denote the fact that this is a fork of pjproject by naming
the variable 'pjproject-sfl' instead of just 'pjproject'.  I could go
further and make it local to the definition of libring or change the
name of the package?


Thank you!

Best,
Lukas

Hi Lukas,

> Sorry for the delay!

Likewise! :)

> Ricardo Wurmus <rekado@elephly.net> writes:
>>> 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.
>>
>> Is there no release that includes these patches?  Why do we have to
>> apply them ourselves?  In Guix we try to avoid patching upstream sources
>> with functional patches (as opposed to patches that fix problems with
>> building the software on Guix), as this is really upstream’s work.
>
> There is no release that I know of that contains these patches.  The
> Ring project ships a Makefile in the 'contrib' directory of the
> 'libring' source tree that downloads the upstream Pjproject source
> and applies these very same patches to it.  It then continues to build a
> bundled version of pjproject + patches.
>
>> Is this really pjproject version 2.4 when we apply a massive patch set
>> like that?
>
> It is not.  It is a fork of pjproject that introduces several functional
> changes, the most notable one being the use of GnuTLS instead of
> OpenSSL.  I suspect the Ring project's motivation for doing this are at
> least in part legal issues as the OpenSSL license seems to be
> incompatible with the GPL according to
> "http://www.gnu.org/licenses/license-list.html".  I don't know if there
> are technical benefits of using one over the other.
>
> I tried to denote the fact that this is a fork of pjproject by naming
> the variable 'pjproject-sfl' instead of just 'pjproject'.  I could go
> further and make it local to the definition of libring or change the
> name of the package?

Thanks for the explanations!

I think it would be better to change the name to “pjproject-sfl” as
well.  Not sure about what to do about the version number, because it
clearly isn’t the same as upstream “pjproject”.

I still think that the definition of “sfl-patches” is particularly
ugly.  Considering that you actually get the very same sources for the
“libring” package and “pjproject-sfl” is only used by “libring” (please
correct me if I’m wrong), I think it’s not pretty to take the sources
and delete everything but the patches.  In this case I think it may be
better to merge the definitions of “pjproject” and “libring”.

It seems to me that much of what is now “pjproject” (e.g. the snippet to
leave only the patches, the extra build phases, etc) would no longer be
needed when building “pjproject” as part of “libring”.  Merging the two
might make this all a lot clearer.

What do you think?

~~ Ricardo


PS: for future reference, this

(lambda (file)
 (if (or (string=? file ".")
         (string=? file ".."))
 #f
 #t))

could more concisely be written as

(cute (not (or (string=? <> ".")
               (string=? <> ".."))))

Hi,

Ricardo Wurmus <rekado@elephly.net> writes:

> PS: for future reference, this
>
> (lambda (file)
>  (if (or (string=? file ".")
>          (string=? file ".."))
>  #f
>  #t))
>
> could more concisely be written as
>
> (cute (not (or (string=? <> ".")
>                (string=? <> ".."))))

I think you mean something like this instead:

(lambda (file)
  (not (or (string=? file ".")
           (string=? file ".."))))

Did I miss something?

Mathieu Lirzin <mthl@gnu.org> writes:

> Hi,
>
> Ricardo Wurmus <rekado@elephly.net> writes:
>
>> PS: for future reference, this
>>
>> (lambda (file)
>>  (if (or (string=? file ".")
>>          (string=? file ".."))
>>  #f
>>  #t))
>>
>> could more concisely be written as
>>
>> (cute (not (or (string=? <> ".")
>>                (string=? <> ".."))))
>
> I think you mean something like this instead:
>
> (lambda (file)
>   (not (or (string=? file ".")
>            (string=? file ".."))))
>
> Did I miss something?

No, *I* did :) I mixed up two things and didn’t check before sending.
Thanks for the correction.

~~ Ricardo