Bug#469397: quick XBMC review

[Andres Mejia]

On Friday 27 November 2009 07:56:33 Bruno Kleinert wrote:
> 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.

We'll just stick with one replacement, and I would personally choose the 
Liberation fonts as that at least is advertised as compatible with the 
Monotype fonts.

> > > 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 ;)

*OUR* orig.tar.*s? You should be careful with this kind of phrasing. Last I 
checked, Debian is not some distro which includes software which is forked 
from their respective upstream sources (though at times it may seem that way).

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

Like I said in my previous email, I'm not willing to remove files/directories 
from the source tarball simply because they appear useless for Debian/Linux. 
That's why they do not appear in the binary packages.

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

Yes, XBMC has it's own implementation of xrandr. It stays.

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

Yes, I think the contents of the zip files should be extracted and tracked in 
XBMC's svn as well, rather than having the zip files themselves tracked. This 
should wait til' next XBMC release to be fixed.

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

XBMC does use pulseaudio already. I think this file is here because of xbmc-
live, which essentially is a package meant to transform a computer into a set-
top box with very little need for any kind of configuration by a user. I'll 
have to double check, but at least this file isn't installed by the main XBMC 
binary packages.

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

They are not distributed through the binary packages, only the source package, 
thus I want them to stay in the source package.

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

They're free and useful to developers of XBMC, but not particularly useful to 
end users (except for xbmc-live). Only xbmc-live has a binary package.

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

Again, they're not packaged in the binary packages.

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

XBMC makes use of the adplug library.

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

FFmpeg is free.

> > > 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 ;)

As upstream, more changes in a distro package of some upstream source makes 
changes from the distro package 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.

We ultimately want XBMC to be free and platform agnostic. To achieve this, we 
want XBMC to build and run from the same source on many platforms with as few 
changes for different platforms as possible.

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

Very well, I understand. Don't take anything I say as a personal attack on 
you. None of it is meant as some kind of attack on anyone.

So far I've considered to look into the issue with the trademarked logos, the 
zip files at scripts/*.zip, and the inclusion of asound.conf.

For everything else, I'm not convinced that anything else should be changed, 
especially when it comes to removing files/directories simply because they're 
not used in Linux.

-- 
Regards,
Andres