[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Patch 1/10] Add pjproject.
|
From: |
Lukas Gradl |
|
Subject: |
Re: [Patch 1/10] Add pjproject. |
|
Date: |
Tue, 20 Sep 2016 00:39:07 -0500 |
|
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Hi Ricardo!
Thank you for your review!
Ricardo Wurmus <address@hidden> writes:
>> 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?
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 <address@hidden>
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
signature.asc
Description: PGP signature
[Patch 2/10] Add dbus-c++., Lukas Gradl, 2016/09/12
[Patch 3/10] Add gsm., Lukas Gradl, 2016/09/12
[Patch 4/10] argon2: Install pkg-config file., Lukas Gradl, 2016/09/12