Bug#523842: mplayer: Debdiff with the change to create the nogui package, as well as a manpages package

[Reinhard Tartler]

Fabrice Coutadeur <fabricesp at ubuntu.com> writes:

> Package: mplayer
> Followup-For: Bug #523842
> User: ubuntu-devel at lists.ubuntu.com
> Usertags: origin-ubuntu karmic ubuntu-patch
>
> This debdiff creates a -nogui package, but also, to avoid duplicating the manpages
> (1 Mb compressed), it creates a -manpages package.
> mplayer-nogui conflicts and replace mplayer (same executable name), and
> mplayer-manpages is recommended in mplayer and mplayer-nogui.

thank you very much for your patch, here is my review:

> diff -u mplayer-1.0~rc3+svn20090405/debian/rules mplayer-1.0~rc3+svn20090405/debian/rules
> --- mplayer-1.0~rc3+svn20090405/debian/rules
> +++ mplayer-1.0~rc3+svn20090405/debian/rules
> @@ -135,7 +135,6 @@
>  	--enable-sdl \
>  	--enable-ossaudio \
>  	--enable-lirc \
> -	--enable-gui \
>  	--enable-freetype \
>  	--enable-menu \
>  	--enable-largefiles 
> @@ -150,16 +149,34 @@
>  	test -r .svnrevision && cp .svnrevision snapshot_version
>  	# Add commands to configure the package here.
>  	$(CLEAN_ENV) \
> -	./configure $(COMMON_CONFIGURE_FLAGS) $(DEB_BUILD_CONFIGURE)
> +	./configure $(COMMON_CONFIGURE_FLAGS) $(DEB_BUILD_CONFIGURE) --disable-gui
>  	touch configure-arch-stamp


probably OK
 
>  # commands to compile the package
> -build-arch: build-arch-stamp
> -build-arch-stamp: configure-arch-stamp
> +build-gui: build-gui-stamp
> +build-gui-stamp:
>  	dh_testdir
> +	if test -f build-nogui-stamp; then \
> +	  $(MAKE) distclean; \
> +	  rm -f build-nogui-stamp; \
> +	fi
> +	rm -f configure-arch-stamp
> +	$(CLEAN_ENV) \
> +	./configure $(COMMON_CONFIGURE_FLAGS) $(DEB_BUILD_CONFIGURE) --enable-gui
> +	$(CLEAN_ENV) \
> +	$(MAKE)
> +	touch build-gui-stamp
> +
> +build-nogui: build-nogui-stamp
> +build-nogui-stamp: configure-arch-stamp
> +	dh_testdir
> +	if test -f build-gui-stamp; then \
> +	  $(MAKE) distclean; \
> +	  rm -f build-gui-stamp; \
> +	fi
>  	$(CLEAN_ENV) \
>  	$(MAKE)
> -	touch build-arch-stamp
> +	touch build-nogui-stamp

this is taken from marillat and look unbelievably silly, IMO. it
obviously does not support paralell builds and looks like someone tried
to write a shellscript in make.

how about merging the configure and merge and build targets for all
variants? We cannot build in parallel anyway and they are all phony
targets. The only drawback I see is that one cannot call anymore the
configure target directly to just get the working directory in the state
after the configure step. Is that useful to anyone?
 
>  ###### build-indep
>  
> @@ -172,7 +189,7 @@
>  	$(MAKE) -C DOCS/xml html-chunked
>  	touch build-indep-stamp
>  
> -build: build-indep build-arch
> +build: build-indep
>  

This target does not seem to be used from anywhere. Perhaps we should
just drop it?

>  ################ clean
> @@ -187,23 +204,29 @@
>  
>  ##################### install
>  
> -install-arch:  build-arch
> +install-arch:  
> +install-nogui: build-nogui
>  	dh_testdir
>  	dh_prep -a
> -	$(MAKE) install-gui DESTDIR=$(destdir)
> -	$(MAKE) install-mplayer-gui-man DESTDIR=$(destdir)
> +	$(MAKE) install-mplayer DESTDIR=$(CURDIR)/debian/mplayer-nogui
>  
so no manpage for the nogui variant?

> diff -u mplayer-1.0~rc3+svn20090405/debian/control mplayer-1.0~rc3+svn20090405/debian/control
> --- mplayer-1.0~rc3+svn20090405/debian/control
> +++ mplayer-1.0~rc3+svn20090405/debian/control
> @@ -75,6 +75,9 @@
>  	 mplayer-skin,
>  	 ${shlibs:Depends},
>  	 ${misc:Depends}
> +Recommends: mplayer-manpages
> +Conflicts: mplayer-nogui
> +Replaces: mplayer-nogui
>  Description: movie player for Unix-like systems
>   MPlayer plays most MPEG, VOB, AVI, Ogg/OGM, VIVO,
>   ASF/WMA/WMV, QT/MOV/MP4, FLI, RM, NuppelVideo, yuv4mpeg, FILM, RoQ, PVA files,


Yes, I know that this is taken from marillat. While reviewing your
patch, I was think if it wouldn't actually make more sense to not
make these package conflict, but rather do the following instead:

 - in mplayer: install /usr/bin/mplayer.gui
 - in mplayer-nogui: install /usr/bin/mplayer.nogui
 - use update-alternatives(8) in both packages
 - make mplayer depend on mplayer.nogui
 - don't install support files like documentation to gui since it is
   already shipped with the nogui package.

what do you think?

> @@ -93,6 +96,40 @@
>   Not all of the upstream code is distributed in the source tarball.
>   See the README.Debian and copyright files for details.
>  
> +Package: mplayer-nogui
> +Architecture: any
> +Section: video
> +Suggests: mplayer-doc,
> +	  ttf-freefont,
> +	  netselect | fping,
> +	  bzip2,
> +	  fontconfig
> +Depends: debconf | debconf-2.0,
> +	 ${shlibs:Depends},
> +	 ${misc:Depends}
> +Recommends: mplayer-manpages
> +Conflicts: mplayer
> +Replaces: mplayer

With the approach above, we would only need these dependencies in the
nogui package, since the normal package would hard-depend on the no-gui
one.

> +Package: mplayer-manpages
> +Architecture: all
> +Section: doc
> +Recommends: mplayer
> +Depends: ${misc:Depends}
> +Description: documentation for MPlayer
> + This package contains the documentation for MPlayer, a movie player for
> + Unix-like systems.

is that really necessary? I read from your mail you want to save archive
space, but I'm not really comfortable with not shipping the manpage in
the normal package is not really comfortable. How about installing only
the english manpage to the nogui package and all translations in
mplayer-doc?

> only in patch2:
> unchanged:
> +++ mplayer-1.0~rc3+svn20090405/debian/mplayer-nogui.install
> +++ mplayer-1.0~rc3+svn20090405/debian/mplayer-nogui.examples
> +++ mplayer-1.0~rc3+svn20090405/debian/mplayer-nogui.mime
> +++ mplayer-1.0~rc3+svn20090405/debian/mplayer-nogui.preinst

with the approach outlined above, we don't need to duplicate these
files. Duplicating files in the package is certainly not acceptable as
it is too error prone.


-- 
Gruesse/greetings,
Reinhard Tartler, KeyID 945348A4