I found this quite complicated to review, so to break it down I imported the package into git and then split your debdiff up into multiple git commits and then applied the relevant quilt patches as commits at the end so that I could see what was going on. I've pushed this to https://git.launchpad.net/~racb/ubuntu/+source/percona-xtradb- cluster-5.6/log/?h=niedbalski&id=niedbalski.c59 and I'll refer to commits from there in my review as I disect your debdiff.
1. Whitespace cleanups in 6624138 from your previous upload are fine I suppose, though they did make the debdiff more difficult to review by creating debdiff noise. But if you're going to clean up like this, please could you also take control of your trailing whitespace in debian/changelog in your latest changes in 371f0bc? 2. Quilt noise in 75439ba is fine, though the did make the debdiff more difficult to review by creating debdiff noise. Please configure your quilt according to https://wiki.debian.org/UsingQuilt (-p ab --no- timestamps --no-index) so that quilt patches your generate are always normalised. This will stop this happening and speed up my reviews. With those cleaned up, I could then see through the noise through to the substance of your proposed changes. 3. Commit 18f879d. I thought the purpose of this upload was to stop the warnings being fatal? Why in addition are we fixing up warnings? I thought this patch was cherry-picked from upstream? Is this additional change upstream? And if os_mb is now defined for all architectures, presumably all the calls to os_mb elsewhere in this patch no longer want to be guarded conditionally on architecture? I'm not yet sure what I'm asking for here. I think I need to give the full patch more thorough review (which might impact what's going into the SRU). If the entire patch can demonstrably only affect ppc64el, then I'd be happier. I need to check if this is the case. 4. Commit 6842cbe. I understand the disabling of -Werror. Is adding -fpermissive strictly required or did the build work without? Why is dropping -Wextra required? And in the diff, why are you dropping -Wall, -Wextra, -Wunused, -Wwrite-strings, etc? Won't the build still succeed with those warnings but not being converted to errors with just a drop of -Werror, or am I missing something? 5. Commit 86ca582. I don't understand the purpose of this change and debian/changelog doesn't explain it either. Please advise. 6. Commit cc4a8b7. I don't understand the purpose of this change and debian/changelog doesn't explain it either. Please advise. 7. Commit 371f0bc5. debian/changelog has trailing whitespace. I really don't care when reviewing others' work, but if you also don't care, then why clean up trailing whitespace in your quilt patch, creating extra diff noise and review pain? -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/1657256 Title: Percona crashes when doing a a 'larger' update To manage notifications about this bug go to: https://bugs.launchpad.net/charm-test-infra/+bug/1657256/+subscriptions -- ubuntu-bugs mailing list [email protected] https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
