On Wed, Nov 25, 2015 at 8:38 AM, Ludovic Courtès <l...@gnu.org> wrote:
> David Thompson <dthomps...@worcester.edu> 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 <dthomps...@worcester.edu>
>> 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 <dthomps...@worcester.edu>
>> +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 <dthomps...@worcester.edu>
>> 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
