[MPlayer-DOCS] letter to distro mplayer packagers?

[Dominik 'Rathann' Mierzejewski]

On Thursday, 01 December 2005 at 23:44, Christian Marillat wrote:
> Dominik 'Rathann' Mierzejewski <dominik at rangers.eu.org> writes:
> > On Thursday, 01 December 2005 at 16:37, Christian Marillat wrote:
[...]
> >> 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?
> 
> As explained in a previous e-mail, the default example.conf doesn't
> work.

As explained in a previous mail, you can comment out all settings
and it should just work. And Diego fixed the 3dfx problem.
Anyway, you can still patch it. I just don't see the point of providing
full multiple config files.

> > * 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.
> 
> I don't intent to package a binary with this sort of warning. The warning
> is clear, don't use this package :
> 
> recompile MPlayer with --disable-runtime-cpudetection

All right, you have a point there. How about removing just the " - WARNING ..."
part _and_ modifying the version string? You see, the "compiled with runtime
cpu detection" message is vital when reading through user-supplied mplayer -v
output. You shouldn't remove it just because you don't like it. ;)

> > * 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.
> 
> The documentation doesn't said the same. From codecs.html :
> 
> ,----
> | If you use an MPlayer release you have libavcodec right in the
> | package, just build as usual. If you use MPlayer from CVS you have to
> | extract libavcodec from the FFmpeg CVS tree as FFmpeg releases are very
> | rare. The CVS is mostly stable and offers the most features. In order to
> | achieve this do:
> `----

Well, these are user docs. Developer/packager docs are in DOCS/tech
and the packaging guidelines clearly state that static linking is
required. If you think it shouldn't be, please say why you think so.

> Anyway this is my problem to check if nothing is broken before
> uploading news packages.

I might have missed it, but are they packaged separately? If not, then
this _should_ be ok, but what's the point of having shared libs if no
other binaries are using them? It's best to link them statically.

> > * sparc and ppc patches: why not submit them to mplayer-dev-eng?
> 
> Sparc patch doesn't exist. On others patches aren't mine.

What can I say to that? Is it possible to contact their authors and
tell them to submit these to us?

> > * 05_heap_overflow.dpatch: looks like a bugfix, please submit to us
> 
> This patch isn't applied.

OK.

> For all Debian stuff read the Debian Reference documentation
> http://qref.sourceforge.net/
> 
> > * 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.
> 
> No, the diff still contains the *.mak, config.h and version.h

Huh? I haven't noticed them and a simple grep doesn't find any config.mak.
I'm looking at:
ftp://ftp.nerim.net/debian-marillat/pool/main/m/mplayer/mplayer_1.0-pre7cvs20051102-0.0.diff.gz
The only reference to config.mak appears when you're substituting
march/mcpu cflags using perl -pe.

> because the
> Makefile is broken and these generated files (by configure) aren't
> removed when doing a make distclean.

It's may be a bug in the buildsystem then. We'll look at it. Thanks for
reporting.

> > 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.
> 
> No, because some options depends on the hardware, like v4l, 3dfx or
> mga

And in most of these cases, I agree. But, there are others: vstream, amr
codecs, freetype, fribidi, xinerama, gl, dxr3, smb, live, v4l(2). Their
detection depends only on the existence of certain libs and headers, not
device files like for fb/directfb, lirc and others.

The only --enable options I'm using in the official RPMs are:
gui, runtime-cpudetection, largefiles, mga, xmga, tdfxfb, directfb, fbdev,
joystick, lirc, menu and xmms. v4l(2) detection doesn't depend on hardware
existence, so if it doesn't get detected despite having kernel support,
it may be a bug and you should report it.

> and yes these options are necessaries because any packaging system
> must support a maximum of hardware/options.

Supporting most options is a matter of philosophy, so I won't argue
with you there. Maybe it's a Debian thing, too.

One more thing: could you explain why some of the configure lines have
--disable-mencoder and some don't? And why provide mplayer-i586 compiled
with 3DNow(Ex)! (in other words, please explain how you are building
non-i386 x86 packages without runtime CPU detection)? Thanks in advance.

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"