guix-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCHES] Add Kodi


From: Ludovic Courtès
Subject: Re: [PATCHES] Add Kodi
Date: Wed, 25 Nov 2015 14:38:42 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

David Thompson <address@hidden> skribis:

> This one was a real hairball, but here is a patch set that adds the Kodi
> media center!  I hope that I've explained all the craziness relatively
> well in the comments.

Wow, quite an achievement!

> From d708d0c36e202bbad7255c3b8a55ca0afdd18cb3 Mon Sep 17 00:00:00 2001
> From: David Thompson <address@hidden>
> Date: Tue, 24 Nov 2015 13:35:44 -0500
> Subject: [PATCH 1/3] gnu: Add tinyxml.
>
> * gnu/packages/xml.scm (tinyxml): New variable.
> * gnu/packages/patches/tinyxml-use-stl.patch: New file.
> * gnu-system.am (dist_patch_DATA): Add it.

[...]

> +From a53b6ee4519a7657164610ac14a82c57b1273bf6 Mon Sep 17 00:00:00 2001
> +From: David Thompson <address@hidden>
> +Date: Mon, 23 Nov 2015 06:54:36 -0500
> +Subject: [PATCH] Use STL.

Maybe just say why this is needed.

> +               ;; Generate and install pkg-config file.
> +               (mkdir-p pkgconfig)
> +               (call-with-output-file (string-append pkgconfig "/tinyxml.pc")

It’s OK to do that, but only if there are users that expect it (usually
because a major distro has been doing it for some time.)  Maybe just add
a something like “because Kodi expects it”, or “similar to what Debian
does”.

Otherwise LGTM.

> From 11b1b337dbd3b245c314b1d22eaeb1331a34e729 Mon Sep 17 00:00:00 2001
> From: David Thompson <address@hidden>
> Date: Tue, 24 Nov 2015 13:37:34 -0500
> Subject: [PATCH 2/3] gnu: Add jasper.
>
> * gnu/packages/image.scm (jasper): New variable.

LGTM.

> From feb1a97e9d3c0e28ee265861bb34c90aa3e06265 Mon Sep 17 00:00:00 2001
> From: David Thompson <address@hidden>
> Date: Mon, 16 Nov 2015 22:31:26 -0500
> Subject: [PATCH 3/3] gnu: Add kodi.

+ commit log and copyright header.

> +     '(#:configure-flags '("--with-ffmpeg=shared") ; don't use bundled ffmpeg

Would it be possible to delete the bundled ffmpeg in a snippet?

> +             (let ((cwd (getcwd)))
> +               (dynamic-wind
> +                 (const #t)
> +                 (lambda ()
> +                   (chdir "tools/depends/native/JsonSchemaBuilder/src")
> +                   (zero? (system* "sh" "autogen.sh")))
> +                 (lambda ()
> +                   (chdir cwd))))))

Use ‘with-directory-excursion’ instead.

> +    (native-inputs
> +     `(("autoconf" ,autoconf)
> +       ("automake" ,automake)
> +       ("cmake" ,cmake)
> +       ("doxygen" ,doxygen)
> +       ("gawk" ,gawk)
> +       ("gettext" ,gnu-gettext)

We could remove autoconf/automake/libtool/gettext if they used ‘make
dist’, but I guess they don’t.  Bah.

We should start a “make dist” campaign; makedist.org appears to be
available…  ;-)

> +       ("icedtea7" ,icedtea7)

Is it a build-only dependency (it’s in ‘native-inputs’)?  Can it be
avoided?

Otherwise LGTM.

Thank you!

Ludo’.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]