Hello Samuel. Thanks a lot for the feedback!
Now I put the latest upload here: https://salsa.debian.org/dfukui/unhide/-/tree/debian/master-rebase-v2 In addition, the tag was updated accordingly. https://salsa.debian.org/dfukui/unhide/-/tags/debian%2F20210124-2 Take a look at them for details of this upload. Following is what's new for this latest version. >So both the "path" patches should be set to not-needed and the >translation one can be sent upstream (in that case you set the value >to the URL of the PR). It's ok if you don't want to send the patch >upstream, and in that case you can remove the Forwarded key, though I >suggest at least opening up a PR against upstream. Yes, that's what we discussed earlier. I created a pull request regarding the translation issue and added its URL to the header. See the following for the details of the PR: https://github.com/YJesus/Unhide/pull/4 >Regarding the patches filenames, they're a bit different from the >format used by other patches due to the use of uppercase letters. It's >ok if you want to keep it that way, I was just wondering if that's >coming by default from some tool you're using (as the filename also >matches the description)? No need to change this unless you want to. Thanks for pointing this out. I use git-buildpackage or gbp for creating a patch. Interestingly, for some reason, that tool tries to capitalise some letters of a commit title and turns it into a filename. Since there is no functional change, I'll keep the filename as they are. >Great, there's just a small issue with shipping the executable files >in /usr/lib, they should be moved to /usr/libexec. You can have a look >at the lintian finding for more details: >https://lintian.debian.org/tags/executable-in-usr-lib Now executables are located at /usr/libexec. See the following for details: https://salsa.debian.org/dfukui/unhide/-/blob/debian/master-rebase-v2/debian/unhide.install >When installing ToolTip.py and unhideGui.py, you don't need to do an >override in d/rules (and neither d/dirs), you can make use of >d/unhide.install to simplify things. Yes, that sounds simpler than the combination of d/rules and d/dirs. Now the install procedure is merged into unhide.install. Besides, Salsa CI results look fine ;) https://salsa.debian.org/dfukui/unhide/-/pipelines/387697 I hope things are better than the previous one. In addition, as always, I appreciate your feedback. Best, Fukui On Tue, 7 Jun 2022 at 07:41, Samuel Henrique <samuel...@debian.org> wrote: > Hello Daichi, > > > I'd appreciate it if you take a look at my revised changes: > > https://salsa.debian.org/dfukui/unhide/-/commits/debian/master-rebase > > > > To help us understand how things have changed since the last draft, > > I'd like to show how each of the issues you mentioned was addressed as > follows. > > Thanks, this is gonna be helpful, > > > > I recommend changing the "Copyright" part of the patch in favor of a > > > DEP3 header, you can use "Author": > > > https://dep-team.pages.debian.net/deps/dep3/ > > Following this example [0], the header parts of the patches I created > were modified like these: > > > > > https://salsa.debian.org/dfukui/unhide/-/blob/debian/master-rebase/debian/patches/Fix-wrong-path-for-python3-interpreter.patch > > > https://salsa.debian.org/dfukui/unhide/-/blob/debian/master-rebase/debian/patches/Translate-French-texts-into-English.patch > > > https://salsa.debian.org/dfukui/unhide/-/blob/debian/master-rebase/debian/patches/Use-paths-defined-in-unhide.install.patch > > Great, I just have a couple of comments left about the patches: > Forwarded: In the case of the three patches, the default value of > Forwarded is already "no" and the doc says "The field is really > required only if the patch is vendor specific, in that case its value > should be "not-needed" to indicate that the patch must not be > forwarded upstream". > So both the "path" patches should be set to not-needed and the > translation one can be sent upstream (in that case you set the value > to the URL of the PR). It's ok if you don't want to send the patch > upstream, and in that case you can remove the Forwarded key, though I > suggest at least opening up a PR against upstream. > > Regarding the patches filenames, they're a bit different from the > format used by other patches due to the use of uppercase letters. It's > ok if you want to keep it that way, I was just wondering if that's > coming by default from some tool you're using (as the filename also > matches the description)? No need to change this unless you want to. > > > All French texts in the script were translated into English except for > comment lines: > > > https://salsa.debian.org/dfukui/unhide/-/blob/debian/master-rebase/debian/patches/Translate-French-texts-into-English.patch > > > > Hopefully I covered "all" French texts... > > I believe so, it looks good. > > > > > 1) desktop file: > > > For GUI applications it's always a good idea to provide a .desktop > > > file, so users can start it from their DEs. But in the case of unhide, > > > the GUI would need to change to identify when it's not running as root > > > and call polkit (or something similar) to get the privileges. > > Yes, this is a good idea. > > As an experiment, I tried to use pkexec when calling unhide-gui, but > that attempt ended up with the following error. > > > > $ pkexec /usr/sbin/unhide-gui > > Traceback (most recent call last): > > File "/usr/sbin/unhide-gui", line 325, in <module> > > root=Tk(className = "UnhideGUI") > > File "/usr/lib/python3.8/tkinter/__init__.py", line 2270, in __init__ > > self.tk = _tkinter.create(screenName, baseName, className, > interactive, wantobjects, useTk, sync, use) > > _tkinter.TclError: no display name and no $DISPLAY environment variable > > > > According to the manpage of pkexec [1], "pkexec will not allow you to > run X11 applications as another user since the $DISPLAY and $XAUTHORITY > environment variables are not set". > > Thus it looks like pkexec would not be suitable for our use case. > > I'm now searching for an alternative way to implement our idea, but it > may take some time or end up with no luck in the worst case. > > For now, I'd like to skip adding the .desktop file to our package. > > I agree with you then, it's gonna be too much work for us to fix this > so let's ship it without the desktop file. > > > > 2) unhideGui: > > > I think the name of the binary could be improved to try to follow the > > > pattern we usually see on unix tools (and in the other binaries of > > > unhide), maybe unhide-gui (all lowercase)? > > Yes, that would help users find the tool. > > See: > > > https://salsa.debian.org/dfukui/unhide/-/blob/debian/master-rebase/debian/unhide.links > > Great, there's just a small issue with shipping the executable files > in /usr/lib, they should be moved to /usr/libexec. You can have a look > at the lintian finding for more details: > https://lintian.debian.org/tags/executable-in-usr-lib > > When installing ToolTip.py and unhideGui.py, you don't need to do an > override in d/rules (and neither d/dirs), you can make use of > d/unhide.install to simplify things. > > > I really appreciate your comments on my updates and your patience for > my slow response. > > Hopefully these changes make sense and are helpful. > > I'm happy to help :) > > > P.S. > > Salsa CI for unhide fails at random due to this issue: > > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1012035 > > After running the reprotest job several times, it finally passed. > > Sounds good. > > I will redo parts of the changelog entry (to try to follow the > standard I stick to) for this release at the end so don't worry about > the lintians complaining about nmu issues. > > Thanks for contributing! > > -- > Samuel Henrique <samueloph> >
