From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ricardo Wurmus Subject: Re: [Patch 1/10] Add pjproject. Date: Mon, 19 Sep 2016 09:41:43 +0200 Message-ID: <87shswmj48.fsf@elephly.net> References: <87mvjc1quq.fsf@openmailbox.org> <87inu01qsj.fsf@openmailbox.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:43672) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bltDE-0004fL-M3 for guix-devel@gnu.org; Mon, 19 Sep 2016 03:42:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bltDB-00071W-H0 for guix-devel@gnu.org; Mon, 19 Sep 2016 03:42:00 -0400 Received: from sender163-mail.zoho.com ([74.201.84.163]:21328) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bltDB-00071B-8L for guix-devel@gnu.org; Mon, 19 Sep 2016 03:41:57 -0400 In-reply-to: <87inu01qsj.fsf@openmailbox.org> List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: Lukas Gradl Cc: guix-devel@gnu.org Hi Lukas, thanks for the patches! > From d6a6a5ded95071a58a160a435ccf56d6828148b0 Mon Sep 17 00:00:00 2001 > From: Lukas Gradl > 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