[MPlayer-DOCS] letter to distro mplayer packagers?

[Dominik 'Rathann' Mierzejewski]

On Thursday, 01 December 2005 at 16:37, Christian Marillat wrote:
> Diego Biurrun <diego at biurrun.de> writes:
> 
> > On Wed, Nov 30, 2005 at 03:27:34PM +0100, Christian Marillat wrote:
> >> Diego Biurrun <diego at biurrun.de> writes:
> 
> [...]
> 
> >> > Why you include a skin uuencoded in the packaging patch and not create a
> >> > skin package that the mplayer-gui package depends on remains a mystery
> >> > to me.
> >> 
> >> Why not. May be a good idea :)

Yes, but why uuencoded and in form of a patch? Is it a debian thing?

> > Well, it makes it hard to swap out one skin for another and it makes the
> > packaging patch very bloated and hard to read...
> 
> diffstats is your friend
> 
> > At the very least it's completely unconventional, I haven't seen this in
> > another Debian package.
> 
> If people are providing a mplayer package build with --enable-gui without a skin
> this package is broken.

Not necessarily, if the dependencies are set properly, although it does
require either automatic depsolver like apt or yum, or sufficiently
clueful user to figure out that you need to install both mplayer-gui and
mplayer-skin-default. I did decide to include the skin in the mplayer-gui
package, for different reason though.

> [...]
> 
> > What about the various copies of the config files and the various
> > patches you apply?
> 
> You are talking about what exactly ?

Well, much of the diff you apply to mplayer source comprises various
files and patches. Let me comment on some of them.
* example.conf, which is a sample mplayer.conf: we provide our own example
  file, so why not just use that instead? Maybe patch it if you want, but
  why provide a whole new file? Additionally, what's the point of providing
  more than one? Doesn't it make updating all of them harder?
* help_mp-en.h patch: why you change it like this is beyond me.
  -#define MSGTR_CompiledWithRuntimeDetection "Compiled with runtime CPU detection - WARNING - this is not optimal!\nTo get best performance, recompile MPlayer with --disable-runtime-cpudetection.\n"
  +#define MSGTR_CompiledWithRuntimeDetection "Compiled for Debian\n"
  What's the point of this change? The recommended method is to modify
  MPlayer's version string.
* configure patch: modifying live555 paths is ok, but using shared
  libavcodec and libpostproc is definitely not, because neither of them
  have a stable api. Shared libpostproc support was removed on purpose.
* sparc and ppc patches: why not submit them to mplayer-dev-eng?
* 05_heap_overflow.dpatch: looks like a bugfix, please submit to us
* preinst script can be simplified by removing code duplication

I see you no longer provide config.mak and config.h files (which I do
think was bad, because they are supposed to be generated during build).
This is good.

One more thing: you shouldn't use --enable-option for options that are
autodetected. Just provide the correct -dev packages and let ./configure
do the rest.

We could discuss the list of features you've chosen to enable, but
let's leave that for later.

Best regards,
R.

-- 
MPlayer RPMs maintainer: http://rpm.greysector.net/mplayer/
"I am Grey. I stand between the candle and the star. We are Grey.
 We stand between the darkness ... and the light."
        -- Delenn in Grey Council in Babylon 5:"Babylon Squared"