Comment 28 for bug 2012599

Revision history for this message
Robie Basak (racb) wrote :

Summary: there are too many apparently SRU-inappropriate changes bundled into this upload; please remove them and supply the minimal necessary changes only.

Full review:

This bug has had some unavoidable churn in -proposed due to external factors resulting in multiple releases of tzdata upstream in quick succession. This type of thing usually increases the risk of error in my experience, so I gave this a more thorough SRU review than usual from the perspective of the changes that would be experienced by users. Specifically this is the diff to this upload all the way from what is currently in the -updates pockets.

The pulling in of the actual upstreams (tzdata and icu-data) look fine, assuming that it's OK for the icu-data version to mismatch tzdata as the newest icu-data wasn't available at the time of upload.

I haven't checked the updates to *.pot and *.po but I assume that they are also fine.

The addition and improvement of automated tests are always OK and very welcome.

However, some remaining changes I'm not sure about. These weren't documented in the SRU paperwork, though we (bdrung and I) did discuss them extensively in #ubuntu-release - thank you for explaining them. However I'm still not sure if these changes are appropriate for an SRU.

Generating debconf templates with Python instead of bash seems like a reasonable improvement, but I'd expect this to be done in the development release only. SRU policy says:

> ...the requirements for stable updates are not necessarily the same as those in the development release. When preparing future releases, one of our goals is to construct the most elegant and maintainable system possible, and this often involves fundamental improvements to the system's architecture, rearranging packages to avoid bundled copies of other software so that we only have to maintain it in one place, and so on. However, once we have completed a release, the priority is normally to minimise risk caused by changes not explicitly required to fix qualifying bugs, and this tends to be well-correlated with minimising the size of those changes. As such, the same bug may need to be fixed in different ways in stable and development releases.

I'd therefore not expect this change to be present in the SRU, but instead for the existing mechanism to be used. If the existing mechanism is broken or not practical in some way, then I think this needs a separate SRU bug with a full justification of the reasons so that regression risk can be considered, and a QA plan can be agreed. I appreciate that you added a dep8 test, but it isn't clear to me that the dep8 test covers *all* the previous behaviour. That discussion should happen in that bug.

There are also multiple and extensive changes and refactorings to debian/tzdata.config. Some of these are unnecessary (reordering and normalising of shell syntax) and these mask the remaining changes, making the rest harder to review. Looking past those, there are some additional indirections, but also the removal of at least one indirection (upstream source at [1]) that we discussed. There's also the new use of convert_timezone. If these changes are necessary then they should be covered by separate bugs with their own individual SRU justifications.

I've tried to cover what would be needed if you feel you *must* push these additional changes in an SRU. But please also consider that this consumes a great deal of SRU reviewers' time (which is currently in short supply), and the answer may still be no. I strongly suggest that you simply stop trying to push extensive changes in SRUs except where there's actually a significant and direct benefit to users to do so. I think it'll probably be fine to just update upstream, icu-data and update the templates with the existing bash generator only? This would then be much easier to review and land safely.

Note that I've not actually found anything wrong with your changes here. But it's not enough for the upload to merely be correct. It's also necessary to demonstrate to others why and how it is correct, as well as why and how choices have been made such that the risk of an inadvertent oversight has been minimised. This aspect is currently mostly missing, resulting in a significant review burden on others. This apsect should be taken into consideration when deciding whether to seek an SRU backport of something or not.

[1] https://git.launchpad.net/~ubuntu-core-dev/ubuntu/+source/tzdata/commit/?id=4c2b898ef99c134a9d202405643136d4b0d38048