Re: Request to review and upload unhide 20210124-1
Hello Samuel. Thanks a lot for the kind feedback and sponsoring the upload! > There is a weird spacing on line 8 of d/copyright, it has more spaces > than needed. Since this doesn't affect anything other than cosmetics, > I will leave it as it is. Oops, sorry for awkward spaces. I should have been more careful. When I am ready to upload unshareGui.py, which we discussed previously, I'll also fix the redundant spaces. > 1) debian/20210124-1: I see you pushed this tag, and it's a good > practice that the person who does the upload creates the tag instead, > since it's that person who is claiming which commit was used for the > upload. I have recreated the tag so you will need to update any clones > of the team's repo. OK, I got it. > 2) Signed tags: You might want to consider signing the tags with your > GPG keys as well. If you're using gbp, the setting is "sign-tags = > True" under the "DEFAULT" section. This helps increase the trust of > your GPG key through Debian contributions (will be handy when applying > to become a DM, for example), besides the other security related > benefits that we get. That sounds important to me in that singing tags helps identify my contributions to Debian. I'll update my gbp.conf. Other than that, I received a notification that the changes were successfully accepted. Again I appreciate your sponsoring and am happy to have contributed to Debian ;) Best regards, Fukui
Re: Request to review and upload unhide 20210124-1
Hello Daichi, > d/copyright was updated as you suggested. > Here is the change. > https://salsa.debian.org/dfukui/unhide/-/commit/92fda0924f6187089b025018d1968306cb845665 There is a weird spacing on line 8 of d/copyright, it has more spaces than needed. Since this doesn't affect anything other than cosmetics, I will leave it as it is. https://salsa.debian.org/dfukui/unhide/-/blob/debian/master/debian/copyright#L8 You could also have added yourself to d/copyright under the debian/* entry, but that's not critical and your name will end up there whenever anybody updates it anyway. > > 2) branches upstream and pristine-tar: > > I believe you forgot to commit those branches, could you do that? > The two missing branches are now available in my repository. > I just forgot to upload them from my local repo. > The branches are as follows. > https://salsa.debian.org/dfukui/unhide/-/tree/upstream > https://salsa.debian.org/dfukui/unhide/-/tree/pristine-tar I forgot to also ask you to push the upstream/ tag, which was created when you imported the new upstream release, but that's ok and I created it myself. I have a couple of comments about tags and I think this is the right place to mention them: 1) debian/20210124-1: I see you pushed this tag, and it's a good practice that the person who does the upload creates the tag instead, since it's that person who is claiming which commit was used for the upload. I have recreated the tag so you will need to update any clones of the team's repo. 2) Signed tags: You might want to consider signing the tags with your GPG keys as well. If you're using gbp, the setting is "sign-tags = True" under the "DEFAULT" section. This helps increase the trust of your GPG key through Debian contributions (will be handy when applying to become a DM, for example), besides the other security related benefits that we get. > > Extra things you could do to improve packaging even more: > > > > 4) Python client: > > I see that unhide now offers a python client, with a GUI: unhideGui.py > > My suggestion is to consider shipping that GUI as well, but it will > > require some testing to make sure it's working fine. > Looks like we need more effort to incorporate the GUI program into our unhide > package. > Allow me to skip trying this improvement for now, but I would like to do this > in the near future. That's ok, > > 8) d/unhide.manpages: > > I'm not sure this file is needed as debhelper should be smart enough > > to catch it, but I didn't double check. > I tried to remove unhide.manpages, but noticed that unhide.8.gz is missing in > the package withtout the .manpages file. > Thus I decided to keep the file as it is. Alright, debhelper is probably not smart enough on this regard then, > Hope the changes above make sense. > If the issues are resolved, let's go to the next step. Perfect, I have done a few changes on d/changelog, so please have a look to see what I would suggest (both for d/changelog and for the commit message). I appreciate your replies pointing to the commits which made the changes. I have pushed your commits to the repo and sponsored the upload, thanks for contributing! Cheers, -- Samuel Henrique
Re: Request to review and upload unhide 20210124-1
Hello Samuel, Thank you for the feedback! That's very helpful. Kindly find how I updated the first version of the patch below. On Wed, 13 Apr 2022 at 06:56, Samuel Henrique wrote: > > Hello Daichi, > > Here's my review, the first 3 items are more important, the other ones > are suggestions on further improvements you can do if you have the > time: > > 1) d/copyright: > I can see upstream has updated the copyright years and assignments on > a few files by looking at the following commit: > https://salsa.debian.org/dfukui/unhide/-/commit/ce1d2cac78695f8373fe1407e666869662ffd38e > > So we need to update the definitions under d/copyright. You could > simplify that file by stating the following: > Files: * > Copyright: 2005-2021 Yago Jesus > 2010-2021 Patrick Gouin > License: GPL-3+ > > And removing the entry for the "sanity" files. > But of course you can always be more precise and declare each file as it is. d/copyright was updated as you suggested. Here is the change. https://salsa.debian.org/dfukui/unhide/-/commit/92fda0924f6187089b025018d1968306cb845665 > 2) branches upstream and pristine-tar: > I believe you forgot to commit those branches, could you do that? The two missing branches are now available in my repository. I just forgot to upload them from my local repo. The branches are as follows. https://salsa.debian.org/dfukui/unhide/-/tree/upstream https://salsa.debian.org/dfukui/unhide/-/tree/pristine-tar > 3) d/control: > We're missing a dependency on procps, and probably on other packages > too (as mentioned in "Require" section of the README). Missing dependencies were added to d/control, following the "Require" section in README. The control file was updated like this: https://salsa.debian.org/dfukui/unhide/-/commit/133273f23c2b4cc2b4cbc8cdda81e24d1547e96c > Due to the lack of procps, I can see autopkgtests are failing. > Let me know if by any chance autopkgtest is passing on your > environment so I can try to figure out why. > And if you're not running that, you can either take advantage of > salsa-ci or make sure your build tools run it (sbuild supports > autopkgtest), let me know if you'd like me to expand on that. Autopkgtest was also failing on my environment, but I didn't notice that for some reason. Now that procfs was added as a dependency, the test passes without a hitch. > Extra things you could do to improve packaging even more: > > 4) Python client: > I see that unhide now offers a python client, with a GUI: unhideGui.py > My suggestion is to consider shipping that GUI as well, but it will > require some testing to make sure it's working fine. Looks like we need more effort to incorporate the GUI program into our unhide package. Allow me to skip trying this improvement for now, but I would like to do this in the near future. > 5) DH bump to 13: > debhelper-compat could be bumper to 13 in d/control DH compat is now bumped to 13. See the following patch for details. https://salsa.debian.org/dfukui/unhide/-/commit/40369939802d4454d681cc4331653b7171f6ac7c > 6) d/control: Standards-Version: > You could go through the upgrade checklist of the Debian policy and > check if it could be bumped to 4.6.0: > https://www.debian.org/doc/debian-policy/upgrading-checklist.html Following the checklist you referred to, I reviewed the source to tell if Std-Ver can be bumped. After looking at the source with the checklist in mind, I concluded that the field should be updated. The field was incremented like this. https://salsa.debian.org/dfukui/unhide/-/commit/778fc0f94ea19629f19ac554a98cb0f41e202d50 > 7) d/unhide.postinst: > This file can be removed since unhide 20110113-3 was superseded 10 years ago. Yes, obviously the .postinst file no longer makes sense. The file was removed as follows. https://salsa.debian.org/dfukui/unhide/-/commit/d4d0b790b965fb0abc7c5ccd6e2b487da76c7994 > 8) d/unhide.manpages: > I'm not sure this file is needed as debhelper should be smart enough > to catch it, but I didn't double check. I tried to remove unhide.manpages, but noticed that unhide.8.gz is missing in the package withtout the .manpages file. Thus I decided to keep the file as it is. Hope the changes above make sense. If the issues are resolved, let's go to the next step. > Thank you for working on this, I'm sure this will benefit a lot of users :) > > > -- > Samuel Henrique Best regards, Fukui Daichi
Re: Request to review and upload unhide 20210124-1
Hello Daichi, Here's my review, the first 3 items are more important, the other ones are suggestions on further improvements you can do if you have the time: 1) d/copyright: I can see upstream has updated the copyright years and assignments on a few files by looking at the following commit: https://salsa.debian.org/dfukui/unhide/-/commit/ce1d2cac78695f8373fe1407e666869662ffd38e So we need to update the definitions under d/copyright. You could simplify that file by stating the following: Files: * Copyright: 2005-2021 Yago Jesus 2010-2021 Patrick Gouin License: GPL-3+ And removing the entry for the "sanity" files. But of course you can always be more precise and declare each file as it is. 2) branches upstream and pristine-tar: I believe you forgot to commit those branches, could you do that? 3) d/control: We're missing a dependency on procps, and probably on other packages too (as mentioned in "Require" section of the README). Due to the lack of procps, I can see autopkgtests are failing. Let me know if by any chance autopkgtest is passing on your environment so I can try to figure out why. And if you're not running that, you can either take advantage of salsa-ci or make sure your build tools run it (sbuild supports autopkgtest), let me know if you'd like me to expand on that. Extra things you could do to improve packaging even more: 4) Python client: I see that unhide now offers a python client, with a GUI: unhideGui.py My suggestion is to consider shipping that GUI as well, but it will require some testing to make sure it's working fine. 5) DH bump to 13: debhelper-compat could be bumper to 13 in d/control 6) d/control: Standards-Version: You could go through the upgrade checklist of the Debian policy and check if it could be bumped to 4.6.0: https://www.debian.org/doc/debian-policy/upgrading-checklist.html 7) d/unhide.postinst: This file can be removed since unhide 20110113-3 was superseded 10 years ago. 8) d/unhide.manpages: I'm not sure this file is needed as debhelper should be smart enough to catch it, but I didn't double check. Thank you for working on this, I'm sure this will benefit a lot of users :) -- Samuel Henrique
Re: Request to review and upload unhide 20210124-1
Hello Daichi, > It's been almost a week since the last post. > I would appreciate it if you consider applying this patch and replying to me. Sorry for the late reply, I wanted to review your package and I try to do reviews within 7 days of the initial request. I would have missed this as I lost track of it so thanks for the ping. I'm gonna review it in the next couple of days. Regards, -- Samuel Henrique
Re: Request to review and upload unhide 20210124-1
> > Hello team, It's been almost a week since the last post. I would appreciate it if you consider applying this patch and replying to me. Best regards, Fukui
