Code review comment for ~sergiodj/ubuntu/+source/coreutils:coreutils-merge-8.32-2

Revision history for this message
Bryce Harrington (bryce) wrote :

* Changelog:
  - [√] old content and logical tag match as expected
  - [√] changelog entry correct version and targeted codename
  - [√] changelog entries correct
  - [√] update-maintainer has been run

* Actual changes:
  - [√] no upstream changes to consider
  - [√] no further upstream version to consider
  - [√] debian changes look safe

* Old Delta:
  - [√] dropped changes are ok to be dropped
  - [√] nothing else to drop
  - [√] changes forwarded upstream/debian (if appropriate)

* New Delta:
  - [-] no new patches added
  - [√] patches match what was proposed upstream
  - [√] patches correctly included in debian/patches/series
  - [√] patches have correct DEP3 metadata

* Build/Test:
  - [√] build is ok
  - [√] verified PPA package installs/uninstalls
  - [√] autopkgtest against the PPA package passes
  - [-] sanity checks test fine

Built fine in lxc container, autopkgtests passed fine, and install/uninstall had no troubles.

Reviewed and verified the two new patches against upstream. Is there a Ubuntu bug to associate with them? If there is, make sure to mention them; if not no biggie. Only other change I'd suggest for the patches is a bit of wording, but it's a super minor nitpick:

      which is necessary to prevent using SYS_getdents which doesn't exist on
      ARM64.

If you change the second 'which' to 'that', the grammar will read a bit smoother.

The delta appears to have all been accounted for and carried forward. It sounds like you're attempting to push some upstream, which is great. There is one formatting error:

    - debian/rules:
      + Allow crossbuilding
      + Run tests
    - Ignore the cut-huge-range test failure on armhf, only seen on the
      buildds.

That second bullet should be a sub-bullet of the first. I.e.:

    - debian/rules:
      + Allow crossbuilding
      + Run tests
      + Ignore the cut-huge-range test failure on armhf, only seen on the
        buildds.

As a future note, I have generally tried to split up the distinct changes in rules (and control) as separate commits. I figure that might make it simpler to drop rules delta in the future. But since coreutils hasn't been maintained in git-ubuntu before, at least having the changes split out by file will be an improvement. But do fix up the changelog entry formatting at least, to avoid confusion.

review: Needs Fixing

« Back to merge proposal