Code review comment for ~ahasenack/ubuntu/+source/isc-kea:focal-isc-kea-bump-1.6.1
Revision history for this message
| Andreas Hasenack (ahasenack) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
> Sorry this review took a while (well from your POV it might have been over-
> night no matter how long it took :-) ).
>
> Generally looks good already, a few requests for info or changes at the bottom
> of this.
>
> Changelog:
> - [√] old content matches Debian
> - [√] changelog entry correct version and targeted codename
> - [√] changelog entries correct
> - [√] update-maintainer has been run
>
> Actual changes:
> - [√] upstream changes look ok
> I have double checked the orig-tarball without an import yet
> I compared your PPA vs upstream vs git and they match.
> - [√] no further upstream version to consider (1.7 is dev, ok to take 1.6)
> - [√] no "new" debian changes to consider as we don't merge but go ahead
>
> Old Delta:
> - [√] dropped changes are ok to be dropped
> - [√] nothing else to drop
> - [√] changes forwarded to debian (I know you are in contact, let that be
> enough for now)
>
> New Delta:
> - [√] I went through the new changes one by one and they seem ok, some
> feedback detail below
>
> Build/Test:
> - [√] build is ok, but x86 only
> - [√] verified PPA package installs/uninstalls
> - [√] autopkgtest against the PPA package passes
> - [√] sanity checks test fine
>
> TODOs:
> - the bugs 1858347 and 1809262 need to be mentioned in the changelog
They were kind of fixed upstream, so I put them next to the "new upstream release" entry.
> - This is "Architecture: any" but I see only amd64 test builds, before a final
> upload probably better also test cross arch build?
Ups, they were in the ppa I was using for testing, just not this one. Fixed.
> - debian/
> header
done
> - for "enable generate messages" the reasoning seems off, when reading
> https:/
> https:/
> It seems a better reason would be "to re-generate docs on build" or
> something like it.
> If we have another reason we should mention that one
When looking for users of the message compiler, I found this bug: https:/
They fixed it another way, but it shows a third party could need it. I thought about putting it in the kea-dev package, but that would introduce a package transition scenario which is a potential delta I wasn't ready to take on, specially if we have to revert it later.
> - --enable-perfdhcp: why is it default off is it known to be broken or bad in
> anyway?
> I found upstream 8ee2c24be138381
> cdc89731ccb2b4a
> projects/
> If you know a reason please add it.
I figure it's not something you would run frequently, that's why it's optional. Maybe we could put it in another package, but that would also introduce a package transition delta which we might not need, depending on what debian will do eventually. I haven't run the tool myself.
> - your PPAs seem to have proposed disabled I see in buildlog libboost-
> system1.67.0 linked but focal proposed has 1.71.0.0ubuntu1. Please make sure
> it builds and tests against the new version as well
Ups, I had that in a previous ppa where I was experimenting with this, but uploaded a bad version to it and didn't want that to accidentally land in the archive, and forgot to enable those things in this new MP ppa. I just did it and will upload a new package anyway, so we will get those results.