Ubuntu

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

Proposed by Daniel van Vugt on 2011-05-25
Status: Merged
Merged at revision: 17
Proposed branch: lp:~vanvugt/ubuntu/oneiric/mediatomb/fix-212441
Merge into: lp:ubuntu/oneiric/mediatomb
Diff against target: 38 lines (+34/-0) 1 file modified
To merge this branch: bzr merge lp:~vanvugt/ubuntu/oneiric/mediatomb/fix-212441
Reviewer Review Type Date Requested Status
Luke Yelavich Approve on 2011-06-03
Evan Broder 2011-05-25 Pending
Ubuntu branches 2011-05-25 Pending
Review via email: mp+62263@code.launchpad.net

This proposal supersedes a proposal from 2011-05-11.

Commit Message

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

Description of the Change

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

To post a comment you must log in.
Evan Broder (broder) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

*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) wrote : Posted in a previous version of this proposal
Download full text (3.5 KiB)

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

Read more...

review: Needs Fixing
Evan Broder (broder) wrote : Posted in a previous version of this proposal

(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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote :

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

Preview Diff

1=== added file 'debian/mediatomb-daemon.mediatomb.upstart'
2--- debian/mediatomb-daemon.mediatomb.upstart 1970-01-01 00:00:00 +0000
3+++ debian/mediatomb-daemon.mediatomb.upstart 2011-05-25 09:39:29 +0000
4@@ -0,0 +1,34 @@
5+description "MediaTomb UPnP media server"
6+author "Daniel van Vugt <vanvugt in launchpad>"
7+
8+start on (local-filesystems and net-device-up IFACE!=lo)
9+stop on runlevel [!2345]
10+respawn
11+
12+env CONFIGXML=/etc/mediatomb/config.xml
13+env LOGFILE=/var/log/mediatomb.log
14+env DEFAULT=/etc/default/mediatomb
15+
16+script
17+ [ -r $DEFAULT ] && . $DEFAULT
18+ [ ! $USER ] && USER=root
19+ [ ! $GROUP ] && GROUP=$USER
20+ if [ -n "$INTERFACE" ]; then
21+ INTERFACE_ARG="-e $INTERFACE"
22+ $ROUTE_ADD $INTERFACE
23+ fi
24+ exec mediatomb \
25+ -c $CONFIGXML \
26+ -u $USER \
27+ -g $GROUP \
28+ -l $LOGFILE \
29+ $INTERFACE_ARG \
30+ $OPTIONS
31+end script
32+
33+post-stop script
34+ [ -r $DEFAULT ] && . $DEFAULT
35+ if [ -n "$INTERFACE" ]; then
36+ $ROUTE_DEL $INTERFACE
37+ fi
38+end script

Subscribers

People subscribed via source and target branches