Merge lp:~vanvugt/ubuntu/oneiric/mediatomb/fix-212441 into lp:ubuntu/oneiric/mediatomb

[Daniel van Vugt]

Introduce Upstart support to avoid start-up races (LP: #212441)

I've now simplified this proposal. It is now trivial.

All aforementioned build failures are actually not related to this change, but are fixed here:
lp:~vanvugt/ubuntu/oneiric/mediatomb/fix-770964-784431

[Evan Broder (broder)]

Thanks for contributing this patch! This sort of racy behavior is exactly what Upstart was designed to solve, and I'm glad that people are starting to see it a solution for that class of problem.

I do have a few concerns about the patch - one is a bit more substantial, and the rest are simply small nits to help you understand our processes a bit better.

First off, I would like to see the /etc/default config file eliminated. /etc/default files were introduced to have a short space to contain the values that would be commonly changed, to minimize conflicts when the init.d script changed. Upstart config files are short and simple enough that this is no longer necessary - end users should simply edit the Upstart config file, and resolve conflicts when the come up. I don't think you'd need a "script" block once you've done that - you can move the route changes into a pre-start and then have a single "exec" line.

Now for the smaller stuff. The version number in the changelog is a bit weird - I assume that's a mistake, but that should be 0.12.1-0ubuntu2. You also target the change at maverick - new changes should always be targeted at the release currently under development (which is currently oneiric). Last, when we close bugs in our changelogs, they should always be in the form "(LP: #212411)" (i.e. no "fixes")

While you look into these considerations, I'm going to change the status of this merge proposal to "Work in progress" to help us keep track of which proposed patches still need review. Feel free to set the status back to "Needs review" when you feel you've addressed the concerns I outlined above. And thanks again for your contribution!

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

As you can see I've been using this fix since maverick. But it's trivial to change the version and release.

It is also easy enough to remove the dependency on /etc/default but not ideal to remove all support for /etc/default in this instance...

This patch is designed specifically to be to be trivially compatible with both the upstream Debian package, and the maverick/natty releases. However to remove the /etc/default/* requires a very ugly diff removing the file debian/mediatomb-daemon.mediatomb.default which comes from upstream. It doesn't appear that dh_installinit has any useful options to tell it to ignore the .default file in debian/rules. Is there another way?

I am happy to fix the version number and make support for /etc/default support optional. However removing /etc/default/* completely it seems will break users' configurations when upgrading to oneiric, AND will make future pulling of upstream debian changes much more painful than they would otherwise be. That's because any diff which deletes the file debian/mediatomb-daemon.mediatomb.default will eventually stop being compatible with future upstream changes.

On the other hand, if you have reviewed these potential conflicts and incompatibilities and still suggest complete removal of /etc/default/mediatomb then I'm happy to do that too.

[Daniel van Vugt (vanvugt)]

Ignore that. I think I have found a way to retire /etc/default/mediatomb gracefully and import its settings during an upgrade. However I seem to have discovered a new upgrade bug which is in the way and might take a while to figure out.

[Daniel van Vugt (vanvugt)]

This new proposal (r18) removes all dependencies on /etc/default as requested by Evan. Unfortunately that also makes the diff much uglier having to automate upgrades from systems whose config is stored in /etc/default/mediatomb. But it does work.

Note also, the oneiric build environment (gcc-4.6) is currently failing to build all mediatomb source to due an unrelated bug #770964: https://bugs.launchpad.net/ubuntu/+source/mediatomb/+bug/770964

[Evan Broder (broder)]

*sigh*

I discussed this with some other developers a few minutes agi, and we came to the conclusion that we weren't quite ready to force dropping /etc/default files - especially with all of the transition implications that carries. So...sorry for making you do the extra work - that was my bad.

In any case, I'm going to go ahead and back that change out, merge in your other branch, and then sponsor the fix.

Thanks for your contribution, and for your understanding

review: Approve

[Evan Broder (broder)]

I've pushed my current revision of the branch to lp:~broder/ubuntu/oneiric/mediatomb/fix-212441-and-770964, but it's failing to build with this error:

g++ -DHAVE_CONFIG_H -I. -I.. -I../tombupnp/upnp/inc -I../src -I../tombupnp/ixml/inc -I../tombupnp/threadutil/inc -I../tombupnp/upnp/inc -I.. -I/usr/include/mysql -DBIG_JOINS=1 -fno-strict-aliasing -DUNIV_LINUX -DUNIV_LINUX -I/usr/include/taglib -pthread -Wall -g -O2 -MT libmediatomb_a-ffmpeg_handler.o -MD -MP -MF .deps/libmediatomb_a-ffmpeg_handler.Tpo -c -o libmediatomb_a-ffmpeg_handler.o `test -f '../src/metadata/ffmpeg_handler.cc' || echo './'`../src/metadata/ffmpeg_handler.cc
../src/metadata/ffmpeg_handler.cc: In function 'void addFfmpegMetadataFields(zmm::Ref<CdsItem>, AVFormatContext*)':
../src/metadata/ffmpeg_handler.cc:92:25: error: 'AVFormatContext' has no member named 'title'
../src/metadata/ffmpeg_handler.cc:94:6: error: 'AVFormatContext' has no member named 'title'
../src/metadata/ffmpeg_handler.cc:96:51: error: 'AVFormatContext' has no member named 'title'
../src/metadata/ffmpeg_handler.cc:98:25: error: 'AVFormatContext' has no member named 'author'
../src/metadata/ffmpeg_handler.cc:100:6: error: 'AVFormatContext' has no member named 'author'
../src/metadata/ffmpeg_handler.cc:102:51: error: 'AVFormatContext' has no member named 'author'
../src/metadata/ffmpeg_handler.cc:104:25: error: 'AVFormatContext' has no member named 'album'
../src/metadata/ffmpeg_handler.cc:106:6: error: 'AVFormatContext' has no member named 'album'
../src/metadata/ffmpeg_handler.cc:108:51: error: 'AVFormatContext' has no member named 'album'
../src/metadata/ffmpeg_handler.cc:110:18: error: 'AVFormatContext' has no member named 'year'
../src/metadata/ffmpeg_handler.cc:112:6: error: 'AVFormatContext' has no member named 'year'
../src/metadata/ffmpeg_handler.cc:114:64: error: 'AVFormatContext' has no member named 'year'
../src/metadata/ffmpeg_handler.cc:116:25: error: 'AVFormatContext' has no member named 'genre'
../src/metadata/ffmpeg_handler.cc:118:6: error: 'AVFormatContext' has no member named 'genre'
../src/metadata/ffmpeg_handler.cc:120:51: error: 'AVFormatContext' has no member named 'genre'
../src/metadata/ffmpeg_handler.cc:122:25: error: 'AVFormatContext' has no member named 'comment'
../src/metadata/ffmpeg_handler.cc:124:6: error: 'AVFormatContext' has no member named 'comment'
../src/metadata/ffmpeg_handler.cc:126:51: error: 'AVFormatContext' has no member named 'comment'
../src/metadata/ffmpeg_handler.cc:128:18: error: 'AVFormatContext' has no member named 'track'
../src/metadata/ffmpeg_handler.cc:130:6: error: 'AVFormatContext' has no member named 'track'
../src/metadata/ffmpeg_handler.cc:132:64: error: 'AVFormatContext' has no member named 'track'
../src/metadata/ffmpeg_handler.cc: In function 'void addFfmpegResourceFields(zmm::Ref<CdsItem>, AVFormatContext*, int*, int*)':
../src/metadata/ffmpeg_handler.cc:181:71: error: 'CODEC_TYPE_VIDEO' was not declared in this scope
../src/metadata/ffmpeg_handler.cc:212:31: error: 'CODEC_TYPE_AUDIO' was not declared in this scope
make[3]: *** [libmediatomb_a-ffmpeg_handler.o] Error 1
make[3]: Leaving directory `/build/evan-mediatomb_0.12...

review: Needs Fixing

[Evan Broder (broder)]

(I'm also going to go ahead and change the status of both this MP and the MP for lp:~vanvugt/ubuntu/oneiric/mediatomb/fix-770964 back to "Work in progress" - same deal applies)

[Daniel van Vugt (vanvugt)]

What a mess. And I can safely say "not my fault". The build failure is caused by this upgrade from a few weeks ago:
https://launchpad.net/ubuntu/oneiric/+source/libav
which I didn't yet have in my oneiric pbuilder base. So my changes were building fine when I tested them :)

I have logged the libav compatibility problem and a partially tested patch here:
https://bugs.launchpad.net/ubuntu/+source/mediatomb/+bug/784431

[Daniel van Vugt (vanvugt)]

OK, the patch for the above build failure is now corrected and tested. Grab the final version from here: https://bugs.launchpad.net/ubuntu/+source/mediatomb/+bug/784431

It should go in debian/patches/, series.

[Luke Yelavich (themuso)]

Thanks for your work. The patch from your last comment, as well as changes from your other branch all builds fine here. Thanks to Evan for starting to collect the needed bits together. I'm going to go ahead and upload your work.

Thanks again.

review: Approve

=== added file 'debian/mediatomb-daemon.mediatomb.upstart'
--- debian/mediatomb-daemon.mediatomb.upstart	1970-01-01 00:00:00 +0000
+++ debian/mediatomb-daemon.mediatomb.upstart	2011-05-25 09:39:29 +0000
@@ -0,0 +1,34 @@
+description "MediaTomb UPnP media server"
+author      "Daniel van Vugt <vanvugt in launchpad>"
+
+start on (local-filesystems and net-device-up IFACE!=lo)
+stop on runlevel [!2345]
+respawn
+
+env CONFIGXML=/etc/mediatomb/config.xml
+env LOGFILE=/var/log/mediatomb.log
+env DEFAULT=/etc/default/mediatomb
+
+script
+	[ -r $DEFAULT ] && . $DEFAULT
+	[ ! $USER ] && USER=root
+	[ ! $GROUP ] && GROUP=$USER
+	if [ -n "$INTERFACE" ]; then
+		INTERFACE_ARG="-e $INTERFACE"
+		$ROUTE_ADD $INTERFACE
+	fi
+	exec mediatomb \
+		-c $CONFIGXML \
+		-u $USER \
+		-g $GROUP \
+		-l $LOGFILE \
+		$INTERFACE_ARG \
+		$OPTIONS
+end script
+
+post-stop script
+	[ -r $DEFAULT ] && . $DEFAULT
+	if [ -n "$INTERFACE" ]; then
+		$ROUTE_DEL $INTERFACE
+	fi
+end script