[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Patch 1/10] Add pjproject.
From: |
Ricardo Wurmus |
Subject: |
Re: [Patch 1/10] Add pjproject. |
Date: |
Mon, 19 Sep 2016 09:41:43 +0200 |
User-agent: |
mu4e 0.9.16; emacs 25.1.1 |
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
[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