Merge lp:~afrantzis/mir/automate-package-abi-versioning into lp:mir
| Status: | Rejected |
|---|---|
| Rejected by: | Alexandros Frantzis on 2015-03-04 |
| Proposed branch: | lp:~afrantzis/mir/automate-package-abi-versioning |
| Merge into: | lp: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:~afrantzis/mir/automate-package-abi-versioning |
| Related bugs: |
| 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 | continuous-integration | Approve on 2015-03-02 | |
|
Review via email:
|
|||
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?
| Cemil Azizoglu (cemil-azizoglu) wrote : | # |
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?
| Alexandros Frantzis (afrantzis) wrote : | # |
> 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.
| Alan Griffiths (alan-griffiths) wrote : | # |
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?
| Cemil Azizoglu (cemil-azizoglu) wrote : | # |
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.
| Robert Carr (robertcarr) wrote : | # |
>>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
| Chris Halse Rogers (raof) wrote : | # |
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?
| Alexandros Frantzis (afrantzis) wrote : | # |
> 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.
| Colin Watson (cjwatson) wrote : | # |
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.
| Adam Conrad (adconrad) wrote : | # |
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.
| Adam Conrad (adconrad) wrote : | # |
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.
| Alexandros Frantzis (afrantzis) wrote : | # |
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.
Unmerged revisions
- 2353. By Alexandros Frantzis on 2015-03-02
-
debian: Automate creation of packaging files based on the current component ABIs

PASSED: Continuous integration, rev:2353 jenkins. qa.ubuntu. com/job/ mir-ci/ 3102/ jenkins. qa.ubuntu. com/job/ mir-android- vivid-i386- build/1468 jenkins. qa.ubuntu. com/job/ mir-clang- vivid-amd64- build/1467 jenkins. qa.ubuntu. com/job/ mir-mediumtests -vivid- touch/1422 jenkins. qa.ubuntu. com/job/ mir-vivid- amd64-ci/ 1099 jenkins. qa.ubuntu. com/job/ mir-vivid- amd64-ci/ 1099/artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ mir-mediumtests -builder- vivid-armhf/ 1422 jenkins. qa.ubuntu. com/job/ mir-mediumtests -builder- vivid-armhf/ 1422/artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ mir-mediumtests -runner- mako/4443 s-jenkins. ubuntu- ci:8080/ job/touch- flash-device/ 18468
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/mir- ci/3102/ rebuild
http://