Re: [PATCHES] Add Kodi

[Thompson, David]

On Wed, Nov 25, 2015 at 8:38 AM, Ludovic Courtès <address@hidden> wrote:
> 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.

Sure.  I added an explanation.  Kodi, and presumably others, assume
that TinyXML was built with STL support on.

>> +               ;; 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”.

Added a note along those lines.

>> 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?

Sure, I removed it.  I also added a TODO to delete more bundled things
that are safe to delete in the future.

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

I should have known that this was already implemented.  :)

>> +    (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…  ;-)

I'm on board.  I spent 99% of packaging time trying to get the
bootstrapping process to work.  I don't think any of the devs realize
that they conflate bootstrapping with configuration, or maybe they
know and don't see a problem.

>> +       ("icedtea7" ,icedtea7)
>
> Is it a build-only dependency (it’s in ‘native-inputs’)?  Can it be
> avoided?

Yes, it is a build-only dependency.  Unfortunately, it is mandatory to
build some part of the application.  I added a comment for clarity.

Pushed with the suggested changes.  Thanks!

- Dave