Mir

Merge lp://qastaging/~afrantzis/mir/automate-package-abi-versioning into lp://qastaging/mir

automate-package-abi-versioning
Merge into development-branch

Proposed by Alexandros Frantzis on 2015-03-02

Status:

Rejected

Rejected by:

Alexandros Frantzis on 2015-03-04

Proposed branch:

lp://qastaging/~afrantzis/mir/automate-package-abi-versioning

Merge into:

lp://qastaging/mir

Diff against target:

426 lines (+115/-96)

16 files modified

debian/control.in (+20/-20)
debian/create_control_and_install_files.sh (+78/-0)
debian/create_postinst_prerm_scripts.sh (+0/-39)
debian/install_ld_so_conf.sh (+0/-26)
debian/libmirclient-debug-extension.install.in (+1/-1)
debian/libmirclient.install.in (+1/-1)
debian/libmircommon.install.in (+1/-1)
debian/libmirplatform.install.in (+1/-1)
debian/libmirprotobuf.install.in (+1/-1)
debian/libmirserver.install.in (+1/-1)
debian/mir-client-platform-android.install.in (+1/-1)
debian/mir-client-platform-mesa.install.in (+1/-1)
debian/mir-platform-graphics-android.install.in (+1/-1)
debian/mir-platform-graphics-mesa.install.in (+1/-1)
debian/rules (+4/-0)
src/protobuf/CMakeLists.txt (+3/-1)

To merge this branch:

bzr merge lp://qastaging/~afrantzis/mir/automate-package-abi-versioning

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Alexandros Frantzis (community)			Disapprove on 2015-03-04
Adam Conrad (community)			Needs Fixing on 2015-03-04
Colin Watson (community)			Needs Fixing on 2015-03-04
Chris Halse Rogers			Abstain on 2015-03-04
Robert Carr (community)			Approve on 2015-03-03
Cemil Azizoglu (community)		2015-03-02	Needs Fixing on 2015-03-03
Alan Griffiths			Abstain on 2015-03-03
Kevin DuBois (community)			Approve on 2015-03-02
PS Jenkins bot (community)	continuous-integration		Approve on 2015-03-02
Review via email: mp+251442@code.qastaging.launchpad.net

Commit message

debian: Automate creation of packaging files based on the current component ABIs

Description of the change

debian: Automate creation of packaging files based on the current component ABIs

With this MP some packaging files (debian/control and various debian/*.install files) are produced at package build time based on the ABIs detected in the source tree (but see "Future improvements" below).

Because during package pre-creation dpkg-source needs preliminary access to the source package fields (i.e. the top fields) of the control file, debian/control is symlinked to debian/control.in. At the beginning of the build process, the symlink is overwritten with a proper control file produced from control.in.

Using a control file produced at build time is a neat and versatile idea. However, there may be issues (e.g. too fragile?) we are not aware of and it may not be acceptable to our debian packaging overlords. If that's the case then we can fall back to having a script that can be invoked manually to update our packaging, plus corresponding tests to ensure that our packaging and source tree (i.e. ABIs) are in sync.

Future improvements:

* Improve how we handle ABIs in the source tree, so that we can detect them more cleanly at package build time (i.e. not grep through all the sources). Perhaps a single file that both CMake and our packaging scripts could read?

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-02:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3102/rebuild

review: Approve (continuous-integration)

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2015-03-02:

Thanks for the quick action and this MP. Since CI is not good at detecting packaging issues, could you mention the testing you've done on this MP?

review: Needs Information

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-03-02:

> Thanks for the quick action and this MP. Since CI is not good at detecting packaging issues,
> could you mention the testing you've done on this MP?

Since the MP changes the packaging process but not the packaging output, I focused on ensuring the new packages matched packages created from trunk. I checked this by locally creating both sets of packages and comparing.

There is also the confidence we get from the successful CI run. Although CI doesn't detect all packaging issues, it can detect a decent amount of them, especially since we enabled installation of all packages in mediumtests last week.

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-03-02:

lgtm

review: Approve

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-03-03:

I like the autogeneration but I worry about it being fragile in ways I do not understand.

Also isn't there a risk of accidentally checking in the generated file that overwrites the link?

review: Abstain

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2015-03-03:

I don't think it's good to have a symlink for debian/control. If the script fails then everything should fail in a spectacular manner, and not act like it has a way to proceed.

review: Needs Fixing

Revision history for this message

Robert Carr (robertcarr) wrote on 2015-03-03:

>>I don't think it's good to have a symlink for debian/control. If the script fails then everything
>> should fail in a spectacular manner, and not act like it has a way to proceed.

It sounds like the symlink is required because dpkg-buildpackage wants to parse a few non ABI (hopefully!) fields from debian/control prior to kicking off the full build (and performing the autogeneration).

LGTM

review: Approve

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-03-04:

I expect this would be NAKed by the distro team.

The ‘traditional’ way this is done is in the clean: (so, override_dh_clean) target; this ensures that it's run before generating the source package. Currently When debian/control autogeneration is done at all it's done to populate a relatively contained field, such as Uploaders:.

Although it's autogenerated we should still have debian/control in source control.

As a distro packager I would not do this, but the “upstreams contain packaging and automatically land in the distro” world is still strange and it seems like this might be reasonable for us to do.

Maybe this is better off as something in tools/ to manually run when necessary?

review: Abstain

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-03-04:

> I expect this would be NAKed by the distro team.
>
> Maybe this is better off as something in tools/ to manually run when necessary?

This matches the fallback I mentioned in the description: "If that's the case then we can fall back to having a script that can be invoked manually to update our packaging, plus corresponding tests to ensure that our packaging and source tree (i.e. ABIs) are in sync."

Let's see what distro says.

Revision history for this message

Colin Watson (cjwatson) wrote on 2015-03-04:

I agree with Chris's comments above. Autogenerating debian/control like this is dangerous and tends to play hell with tools that expect the list of package names to be accurate at the point when the source package is built; as written this will probably result in an incorrect .dsc file in some cases, causing archive problems. Best practice when this stuff is necessary is to do it in the clean rule so that the source package is accurate, or to have a script to update it manually when necessary.

review: Needs Fixing

Revision history for this message

Adam Conrad (adconrad) wrote on 2015-03-04:

02:38 < infinity> alf: I would NACK it if you asked for a review from me. :P
02:39 < infinity> alf: debian/control should be correct, leading to .dsc actually listing the binary packages a source produces. Your control and .dsc contain garbage until build time, which is unacceptably wrong.
02:39 < infinity> alf: Generating that at source package creation time is fine, though. You should just do that.

As an added bonus, using the symlink trick here, your debian/control isn't just inaccurate, but actually syntactically broken, as it contains invalid characters in the Package: fields.

review: Needs Fixing

Revision history for this message

Adam Conrad (adconrad) wrote on 2015-03-04:

Another aside, if you ever intend to get this stuff into Debian, the Debian ftp/build infrastructure is very picky about sources producing the binaries listed in .dsc (I had to fix glibc to stop listing some binaries it didn't produce), so this really should just be done correctly from the get go.

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-03-04:

OK, it seems that, as feared, automagic creation of debian/control is indeed fragile and needs special care. Although there are ways to make it work, the process will become more complicated than what I would like. It's much more straightforward to go for a scripted but manual packaging update and tests to check for inconsistencies between source tree ABIs and packaging. Stay tuned for a new MP.

review: Disapprove