Code review comment for lp://staging/~gz/ubuntu/raring/juju/0.7

Revision history for this message
James Page (james-page) wrote :

Ignoring the MP as unreadable: had a look at the branch debdiff with current:

1) postinst

Needs to set -e; also you should not set the alternatives manually:

    update-alternatives --set juju $base_dir/juju-$VER/bin/juju

That's something for the end-user to decide; we just decide which is default based on relative priories of juju/go juju binaries.

3) postrm

    rm -Rf /usr/lib/python2.7/dist-packages/juju

That should not be required.

3) rules

Not really the correct way to change the version in the maintainer scripts:

override_dh_builddeb:
    sed -i -e 's/__NEW_VERSION__/$(VER)/' debian/juju/DEBIAN/*
    dh_builddeb

This would be better done by renaming the maintainer scripts <script>.in and having some rules that generate the actual maintainer scripts before builddeb:

debian/juju.postinst: debian/juju.postinst.in
    sed -e "s/__NEW_VERSION__/$(VER)/g' $< > debian/juju.postinst

debian/juju.prerm: debian/juju.prerm.in
    sed -e "s/__NEW_VERSION__/$(VER)/g' $< > debian/juju.prerm

and then override dh_installdeb to get them to generate prior to debhelper installing them:

override_dh_installdeb: debian/juju.postinst debian/juju.prerm
    dh_installdeb

Remember to add cleanup to remove the generated files as well.

4) manpages

Need to be covered by use of alternatives as well.

Should the resulting binary not be called juju-0.7 or something similar? Otherwise when you upgrade to 7.1, you will have to remove old alternatives.

review: Needs Fixing

« Back to merge proposal