Re: Request to review and upload unhide 20210124-2
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>
Reply to: