Bug#469397: quick XBMC review

[Bruno Kleinert]

Am Donnerstag, den 26.11.2009, 22:35 -0500 schrieb Andres Mejia:
> Please CC the bug too.
> 
> On Wednesday 25 November 2009 16:35:59 Bruno Kleinert wrote:
> > Hi there,
> > 
> > I had a quick look at XBMC's source tree and wrote some kind of
> > "protocol" about everything that didn't look that good to me. Here we
> > go...
> > 
> > ---[ snip ]---
> > Copies of external packages' source code
> > ========================================
> > tinyXML version unknown
> > lib/fontconfig_win32 version 2.7.3
> > lib/freetype version 2.3.9
> > lib/libSDL-WIN32 unknown version, patched with
> > lib/libSDL-WIN32/SDL_SetWidthHeight.diff
> > system/python/spyce
> > cdrip/oggvorbis/*
> > cores/dvdplayer/Codecs/ffmpeg
> > cores/dvdplayer/Codecs/liba52
> > cores/dvdplayer/Codecs/libdts
> > cores/dvdplayer/Codecs/libdvd
> > cores/dvdplayer/Codecs/libfaad2
> > cores/dvdplayer/Codecs/libmad
> > cores/dvdplayer/Codecs/libmpeg2
> > cores/dvdplayer/DVDCodecs/Audio/liba52
> > cores/dvdplayer/DVDCodecs/Audio/libdts
> > cores/dvdplayer/DVDCodecs/Audio/libfaad
> > cores/dvdplayer/DVDCodecs/Audio/libmad
> > cores/dvdplayer/DVDInputStreams/dvdnav
> > cores/dvdplayer/DVDInputStreams/mms
> > cores/ffmpeg
> > cores/paplayer/AC3Codec
> > cores/paplayer/FLACCodec/flac-1.2.1
> > cores/paplayer/ModuleCodec/dumb also it's non-DFSG free
> > cores/paplayer/ogg
> > cores/paplayer/SIDCodec/libsidplay
> > cores/paplayer/SIDCodec/builders/hardsid-build
> > cores/paplayer/SIDCodec/builders/resid-builder
> > cores/paplayer/SPCCodec
> > xbmc/cores/paplayer/timidity
> > cores/paplayer/vgmstream
> > cores/paplayer/vorbisfile
> > cores/paplayer/WavPackCodec
> > cores/paplayer/YMCodec
> > FileSystem/curl
> > lib/"mostly"* - WTF?! What an unbelievable mess! Even source copies
> > contain other source copies?!?
> > xbmc/screensavers/rsxs-0.9
> > xbmc/visualizations/Goom/goom2k4-0 - Copied & patched
> > xbmc/visualizations/Milkdrop/vis_milkdrop
> > xbmc/visualizations/OpenGLSpectrum
> 
> Yes, xbmc does use third party code, which include libraries. It is possible 
> to use system libraries rather than internal libraries for most of the 
> libraries xbmc ships. I would like to see xbmc use nothing but system 
> libraries where available, but this of course is going to require a lot of 
> work.
> 
> > Illegal sources
> > ===============
> > media/Fonts/arial.ttf
> 
> I suppose these can be replaced with the Liberation fonts.
I think the dejavu-* fonts could be an alternative, too.

> > All precompiled binaries (Some with not so obvious suffixes)
> 
> I don't see how these are illegal, unless you mean they don't have the 
> corresponding source used to compile them, with license. In any case, could 
> you clarify what you meant by this.
I meant that there shouldn't be anything precompiled in
our .orig.tar.*s. Illegal is just some category I made up. Don't rely
too much on the names of my topics ;)

> > Questionable/Useless
> > ====================
> > media/weather.rar
> > credits/credits.mod
> > find -iname "*.bat"
> 
> These are used, though of course the .bat files are not used in linux.
Then I suggest to drop them from the .orig.tar.gz. In some packages I
use scripts to generate .orig.tar.gz files which automatically strip
unwanted files before compressing the sources.

Something like find -iname "*.bat" -exec rm -f "{}" \; should be enough.
Probably also usable for other kind of files (*.exe, *.dll, *.lib,
*.dylib and so on)

> > project/*
> 
> These are used for Windows at least.
Then I suggest to remove that directory from the .orig.tar.gz.

> > xbmc-xrandr.c - WTF?!
> 
> What exactly is the problem with xbmc-xrandr.c?
It looks like a redundant implementation of the xrandr from the
x11-xserver-utils package. If it's just a source copy, then I'd suggest
to remove it.

> > scripts/*.zip
> 
> XBMC can open scripts that are inside a zip file.
Ah, ok. Why not put them extracted into the .orig.tar.gz and build the
ZIP archives at build time? I think that's a bit easier to maintain and
keep track of changes in the scripts.

> > system/asound.conf - WTF?!
> 
> Don't know why that file is there.
It looks as if it's there to replace the usual system configuration. But
for multichannel playback I'd suggest to use pulseaudio anyways. It
already should (if it's not broken ;)) take care of the correct ALSA
channels.

> > system/python/DLLs 'file' says precompiled PE32 binaries for .pyd files
> 
> These are used in windows.
Should be removed from the .orig.tar.gz

> > tools/MingwBuildEnvironment/msys.7z
> > tools/PackageMaker
> > tools/TexturePacker
> > tools/XBMCLive
> > tools/XBMCTex
> > tools/XprPack
> 
> The stuff in tools is basically utilities used by xbmc devs for various 
> purposes. Some of it is sources for precompiled binaries in other parts of the 
> source tree. XBMCLive is where the package xbmc-live comes from.
If it's development only and/or non-free tools I'd suggest to remove
them from the .orig.tar.gz.

If they're free and useful to develop other cool stuff for XBMC I'd
suggest to keep them and build binary packages from them.

> > visualisations - 'file' says there are lots of PE32 binaries with
> > suffix .vis
> 
> These are precompiled binaries for windows.
should be removed from .orig.tar.gz

> > visualisations/Milkdrop/*.zip - ?!?
> > visualisations/projectM - Authors? Copyright? Redistribution? Looks
> > stolen to me
> 
> libprojectm is already in the Debian archive. Also, milkdrop is going to be a 
> part of projectm soon.
Oops, my mistake ;)

> > cores/paplayer/ADPCMCodec
> 
> Comes from adplug which is in the archive.
Isn't it another source code copy then or is it a wrapper around the
codec?

> > cores/paplayer/shn - non-DFSG free
> 
> Yes, there is that nagging "non-commercial" clause. xbmc should probably take 
> advantage of the ffmpeg implementation for shorten instead.
What be great to find something free. On the other hand: if there's no
chance for XBMC to go into main, then, for packaging, it doesn't matter
if it's restricted to non-commercial use.

> > General
> > =======
> > Most GPL source code comment headers have an outdated FSF address
> > Stuff (source or binary doesn't matter) from commercial platforms like
> > Mac OS X and Windows is not our business. Whatever its licences say, it
> > should not be part of an .orig.tar.*
> > ---[ snap ]---
> 
> Will get to fixing the address from the GPL headers at some point.
> 
> About stuff that doesn't seem necessary or useful for Linux (or Debian), there 
> has to be some legitimate reason for removing them from the orig tarball. 
> Also, it has to be some reason other than "it's not useful for Linux/Debian".
Well, at least it makes the sources less clear and harder to review ;)

> The reason for this is simply because, speaking as upstream, we want to 
> _avoid_ having our source tree munged as much as possible.
How complicated is it to separate the XBMC sources from everything else
that's needed for non-uninx platforms? That would make integrating XBMC
into a Linux distribution much, much easier and would avoid license
issues with slipped-in non-free things.

> I personally do not want xbmc to be in a situation similar to how ffmpeg was in 
> Debian.
Then the easy way is to put it into non-free ;)

Eeeh... to make things clear: By my quite negative review I did *NOT*
mean to demotivate you about your work in and for XBMC! It's meant
technical!

Cheers - Fuddl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Dies ist ein digital signierter Nachrichtenteil
URL: <http://lists.alioth.debian.org/pipermail/pkg-multimedia-maintainers/attachments/20091127/3d533196/attachment-0003.pgp>