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

Proposed by Daniel van Vugt on 2011-05-11
Status: Superseded
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
debian/mediatomb-daemon.mediatomb.upstart (+34/-0)
To merge this branch: bzr merge lp:~vanvugt/ubuntu/oneiric/mediatomb/fix-212441
Reviewer Review Type Date Requested Status
Evan Broder (community) Needs Fixing on 2011-05-17
Ubuntu branches 2011-05-11 Pending
Review via email:

This proposal has been superseded by a proposal from 2011-05-25.

Commit message

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

To post a comment you must log in.
Evan Broder (broder) wrote :

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 :

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 :

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 :

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:

Evan Broder (broder) wrote :


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 :
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/' || echo './'`../src/metadata/
../src/metadata/ In function 'void addFfmpegMetadataFields(zmm::Ref<CdsItem>, AVFormatContext*)':
../src/metadata/ error: 'AVFormatContext' has no member named 'title'
../src/metadata/ error: 'AVFormatContext' has no member named 'title'
../src/metadata/ error: 'AVFormatContext' has no member named 'title'
../src/metadata/ error: 'AVFormatContext' has no member named 'author'
../src/metadata/ error: 'AVFormatContext' has no member named 'author'
../src/metadata/ error: 'AVFormatContext' has no member named 'author'
../src/metadata/ error: 'AVFormatContext' has no member named 'album'
../src/metadata/ error: 'AVFormatContext' has no member named 'album'
../src/metadata/ error: 'AVFormatContext' has no member named 'album'
../src/metadata/ error: 'AVFormatContext' has no member named 'year'
../src/metadata/ error: 'AVFormatContext' has no member named 'year'
../src/metadata/ error: 'AVFormatContext' has no member named 'year'
../src/metadata/ error: 'AVFormatContext' has no member named 'genre'
../src/metadata/ error: 'AVFormatContext' has no member named 'genre'
../src/metadata/ error: 'AVFormatContext' has no member named 'genre'
../src/metadata/ error: 'AVFormatContext' has no member named 'comment'
../src/metadata/ error: 'AVFormatContext' has no member named 'comment'
../src/metadata/ error: 'AVFormatContext' has no member named 'comment'
../src/metadata/ error: 'AVFormatContext' has no member named 'track'
../src/metadata/ error: 'AVFormatContext' has no member named 'track'
../src/metadata/ error: 'AVFormatContext' has no member named 'track'
../src/metadata/ In function 'void addFfmpegResourceFields(zmm::Ref<CdsItem>, AVFormatContext*, int*, int*)':
../src/metadata/ error: 'CODEC_TYPE_VIDEO' was not declared in this scope
../src/metadata/ 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) wrote :

(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 :

What a mess. And I can safely say "not my fault". The build failure is caused by this upgrade from a few weeks ago:
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:

Daniel van Vugt (vanvugt) wrote :

OK, the patch for the above build failure is now corrected and tested. Grab the final version from here:

It should go in debian/patches/, series.

20. By Daniel van Vugt on 2011-05-25

Remove confusing and unnecessary changelog edits.

Unmerged revisions

20. By Daniel van Vugt on 2011-05-25

Remove confusing and unnecessary changelog edits.

19. By Daniel van Vugt on 2011-05-19

Revert to revision 17 (the first attempt) which is the one Evan has
chosen to merge.

18. By Daniel van Vugt on 2011-05-15

* Improved Upstart support (LP: #212441):
  - Removed dependency on /etc/default/mediatomb.
  - Automated config upgrading during package upgrade; from
    /etc/default/mediatomb into /etc/init/mediatomb.conf

17. By Daniel van Vugt on 2011-05-11

Introduce Upstart support (fixes LP: #212441)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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:36:33 +0000
4@@ -0,0 +1,34 @@
5+description "MediaTomb UPnP media server"
6+author "Daniel van Vugt <vanvugt in launchpad>"
8+start on (local-filesystems and net-device-up IFACE!=lo)
9+stop on runlevel [!2345]
12+env CONFIGXML=/etc/mediatomb/config.xml
13+env LOGFILE=/var/log/mediatomb.log
14+env DEFAULT=/etc/default/mediatomb
17+ [ -r $DEFAULT ] && . $DEFAULT
18+ [ ! $USER ] && USER=root
19+ [ ! $GROUP ] && GROUP=$USER
20+ if [ -n "$INTERFACE" ]; then
23+ fi
24+ exec mediatomb \
25+ -c $CONFIGXML \
26+ -u $USER \
27+ -g $GROUP \
28+ -l $LOGFILE \
31+end script
33+post-stop script
34+ [ -r $DEFAULT ] && . $DEFAULT
35+ if [ -n "$INTERFACE" ]; then
37+ fi
38+end script


People subscribed via source and target branches