On Sat, Aug 5, 2017 at 5:12 AM, Mattia Rizzolo <[email protected]> wrote:
On Wed, Aug 02, 2017 at 06:51:00AM -0700, Olzhas Rakhimov wrote: > > I explicitly listed 'to-be-installed' files in scram.install & > > scram-gui.install. Reuploaded > > Very well, so let's get to the rest of the packaging review: btw, be > aware that I am looking only at what's in the git repository, I haven't > even looked at mentors.d.n. > > * I see the bash completion moved from > /usr/share/bash-completion/completions/scram.sh to > /usr/share/bash-completion/completions/scram/scram.sh - I have no > knowledge of how bash completions should be handled, does it still > work? (it's due a trailing '/scram' in d/scram.install, remember that > dh_install does not behave like cp…) > This script with shell extension was somehow failing on Ubuntu. I wanted to rename and remove the extension. * is d/p/0001-GUI-Fix-the-static-build.patch still needed after > switching to the dynamic lib again? > You are right it is not needed anymore. I removed it. * is d/p/0001-GUI-Update-.desktop-with-URL-and-Keywords.patch upstreamed > or something? > Yes, this is directly from upstream. * The last debian upload was 0.11.5-1, but there are a bunch of other > changelog entries: it's not a real problem (I just need to pass the > correct -v option to dpkg-buildpackage (dpkg-genchanges actually) when > building before upload), but still weird. Consider that you could > have uploaded all of those to experimental instead of keeping them for > now :) > * stadards-version bump is not documented > * new binary is not documented either > * same for d/copyright changes > Probably, I have to learn how to do uploads to experimental. For now, I will squash the changelogs for the pseudo-published releases. * all of this said, why was the manpage removed from the upstream side? > The upstream switched to help2man to generate manpages from help prompts automatically. * last: why -DBUILD_TESTS=OFF ? > This depends on custom (patched) googletest submodule that I cannot seem to pull into builds. I need to download the tarball or with git submodule update --init --recursive with the original repository. I hope to drop that custom dependency someday and switch to standard googletest available on debian; at that time, it should be easier to port the tests as well. > As you can see nothing major, in my opinion: nice work :) > > Thanks for the review. There’s definitely a lot nitty-gritty stuff to learn for me. Regards, Olzhas Rakhimov
